When switching from Gin to Claro the header becomes empty

[rpkoller]

If you switch from Gin to Claro on 'admin/appearance' the header is becoming empty. I've also tried to uninstall the gin toolbar but it made no difference. either way with or without the gin toolbar the header remains empty with claro as the active admin theme.

Screenshot

i tried to install gin on a none starshot instance and there i am able to switch between claro and gin.

[TravisCarden]

Thanks, @rpkoller. This is because there are no blocks placed in the "Block layout" for Claro at admin/structure/block/list/claro.

@phenaproxima, what do you think of this? Is it a question of an unexpected use case or unexpected behavior. I would have thought the Claro theme would install its own blocks when it was installed (which it is, because Gin depends on it), but apparently not in our case. I noticed there's a relevant Core recipe core_recommended_admin_theme, but it conflicts with our config (and might be broken anyway). What do you think?

[boulaffasae]

Maybe we should just use Claro, then

1/ either let the end user decide to install gin 2/ add a script to the recipe to install it

[phenaproxima]

I think this is a bug and should be addressed as a bug. I think the prototype should use Gin, that's a good, strong opinion for us to have. If the user decides to switch to Claro and something is wrong when they do, that's a bug somewhere and we should fix it.

[boulaffasae]

Yes sir 💯

Any idea where to start so that I can fix the issue ?

Changing recipes/starshot/recipe.yml doesn't modify the config after running ddev quick-start. I couldn't understand why :/ ? ** I tried replacing the Gin recipe with core recommended admin theme and it changed nothing at the end of the build (I won't replace the recipe, I was just testing)

[phenaproxima]

I'm not entirely certain the bug is in any of the recipes. It might be in core somewhere. Hard to say without me actually sitting down and tracing into this behavior with the debugger.

I notice that Claro ships several blocks in its config/optional directory, two of which are assigned to the header region. I'd start by switching from Gin to Claro using /admin/appearance, and figuring out:

1. Do those blocks exist in the database now? (drush cex would tell me that.)
2. If they don't exist, why weren't they created when Claro was installed? If they do exist, why aren't they visible? Maybe they are disabled (status: false) or hidden in some other way?

[boulaffasae]

No they don't exist (drush cex show only olivero and gin)

and it's not just claro, stark also have the same issue - it get installed, with no blocks

[phenaproxima]

Then yeah, I think the question is why the hell they weren't created when Claro was installed. As far as I can tell, they should be! I guess I'd probably start tracing into \Drupal\Core\Config\ConfigInstaller::installOptionalConfig(). If that's not getting called, I'd move the trace to \Drupal\Core\Extension\ThemeInstaller::install(), which should definitely be getting when Claro is installed.

[gitressa]

I tried replacing Gin and Navigation with Claro and Admin Toolbar, and found a few things. There could be overlap with this issue ...

• #154

[boulaffasae]

Hi !

I was able to track the issue and I came down to the below, it seems that we are disabling configuration from being installed so we can use custom ones instead.

I will continue checking, to find a solution to have both custom/default configurations working.

** removing the code that disable the config solve the issue - only \Drupal::service('theme_installer')->install([$theme]); is needed

  /**
   * Installs a theme for a recipe.
   *
   * @param string $theme
   *   The name of the theme to install.
   * @param \Drupal\Core\Config\StorageInterface|\Drupal\Core\Recipe\Recipe $recipeConfigStorage
   *   The recipe or recipe's config storage.
   * @param array<mixed>|null $context
   *   The batch context if called by a batch.
   */
  public static function installTheme(string $theme, StorageInterface|Recipe $recipeConfigStorage, ?array &$context = NULL): void {
    if ($recipeConfigStorage instanceof Recipe) {
      $recipeConfigStorage = $recipeConfigStorage->config->getConfigStorage();
    }
    // Disable configuration entity install.
    \Drupal::service('config.installer')->setSyncing(TRUE);
    $default_install_path = \Drupal::service('extension.list.theme')->get($theme)->getPath() . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;
    // Allow the recipe to override simple configuration from the theme.
    $storage = new RecipeOverrideConfigStorage(
      $recipeConfigStorage,
      new FileStorage($default_install_path, StorageInterface::DEFAULT_COLLECTION)
    );
    \Drupal::service('config.installer')->setSourceStorage($storage);

    \Drupal::service('theme_installer')->install([$theme]);
    \Drupal::service('config.installer')->setSyncing(FALSE);
    $context['message'] = t('Installed %theme theme.', ['%theme' => \Drupal::service('extension.list.theme')->getName($theme)]);
    $context['results']['theme'][] = $theme;
  }

[phenaproxima]

That code is there for a reason. :) Recipes don't install any config entities automatically; they expect the recipe to explicitly list the config they want. That's an intentional difference between normal theme installation, and installing a theme via a recipe.

But that also seems beside the point. If I'm understanding this issue correctly, the blocks are gone if you install Claro normally, after having installed a recipe. In such a case, the recipe runner has absolutely nothing to do with installing Claro, so their config should be installed normally.

Unless it was somehow installed previously by a recipe, and merely reactivated later, as https://github.com/phenaproxima/starshot-prototype/issues/154#issuecomment-2260799518 would seem to suggest. That said, none of the Starshot recipes, to my knowledge, install Claro explicitly.

So this warrants further debugging. Removing that code is (probably) not the answer.

[boulaffasae]

Hi @phenaproxima

I still think this have something to do with that RecipeOverrideConfigStorage (I just can't prove it :p)

Issue was gone just by modifying web/recipes/starshot_admin_theme/recipe.yml (I was changing the wrong file the whole time -_-)

name: Admin Theme
type: Starshot
description: Sets up a nice administrative theme and navigation.
install:
  - coffee
  - gin
  - gin_toolbar
  - navigation
config:
  import:
+   claro: '*'
    gin: '*'
    navigation: '*'
  actions:
    gin.settings:
      simple_config_update:
        high_contrast_mode: true
    system.theme:
      simple_config_update:
        admin: gin

Core have a different method of importing Claro configuration. I think it would much better to use the same, what do you think @phenaproxima (I tested it)

name: 'Admin theme'
description: 'Sets up Claro as the administrative (backend) theme.'
type: 'Themes'
install:
  - claro
  - block
config:
  import:
    system:
      - system.menu.account
      - system.menu.main
      - system.theme
    claro:
      - block.block.claro_breadcrumbs
      - block.block.claro_content
      - block.block.claro_local_actions
      - block.block.claro_messages
      - block.block.claro_page_title
      - block.block.claro_primary_local_tasks
      - block.block.claro_secondary_local_tasks
  actions:
    system.theme:
      simpleConfigUpdate:
        admin: claro

[phenaproxima]

No, we'll stick with Gin. I don't think having starshot_admin_theme install Claro is the proper fix.

Whatever's going on here is interesting and we need to figure out the real root cause, not just guessing and bypassing code that seems to fix the problem. Sorry to sound strict here, but from an engineering perspective I'm not comfortable (at least, not in this case) just trying to work it.

I have a theory -- I think (could be wrong here) that the Drupal core installer uses Claro by default. Maybe that's what's happening here? Perhaps the correct fix is to have the install profile (starshot_installer) use Gin too. In any event, Claro and Stark should not be installed once the site has been fully installed.

[boulaffasae]

Hmmmmm !

I don't understand

1/ Claro being installed is not the real issue, the issue is having it's configuration not being imported 2/ Gin will install Claro, because Claro is a dependency for Gin theme

and I think the issue is clear now, RecipeRunner disable the default config import from running. And he only import the configuration that were declared in config.import else why do we have config.import.gin: '*' let's remove that too so we all can be comfortable (well surely we will be after having an admin page that look's like a PDF)

** :) I'm happy, keep sounding strict - I too wanna build the right thing, not just a working thing ** sorry for naming starshot a thing :p ** pleaase tell me that you know that Gin use Claro, why do you hate Claro this much. People worked on that together <3 without it Gin won't be here today

[phenaproxima]

Gin will install Claro, because Claro is a dependency for Gin theme

Ooooh, that changes things! I did not know that! I stand corrected. If Gin has a hard dependency on Claro, then I agree, the fix here is for starshot_admin_theme to import Claro's config, despite using Gin as the default.

Also, I do not remotely hate Claro. I think Claro is beautiful, but I also think Gin is a more modern and useful experience, for a bunch of reasons. That said, the final decision about whether Starshot will use Gin or Claro for its admin theme is not my decision to make. :)

[boulaffasae]

WHAAAAAAAAAT :neutral_face: :neutral_face:

from an engineering perspective BLA BLA BLA BLA

I DON'T KNOW WHO YOU ARE BUT I WILL FIND YOU AND YOU WILL ~~KILL~~ HUG YOU :heart:

Don't hate me to we Moroccan are born like this

[phenaproxima]

Don't hate me :) I'm glad we found the cause! I apologize for my intransigence and lack of understanding.

Regarding the recipe system's config installer -- now that I see the nature of the problem (thanks for being patient with me!), this raises an interesting issue, which is that a recipe needs to, in a sense, import the config for all of the dependencies of the dependencies to avoid anomalies like this. That's a troublesome implication, but it's beyond my pay grade. I'd have to run this one past @alexpott to see if this was intentional, or a bug that needs fixing in core.

[boulaffasae]

NAH! It's me who should apologize to you, you guided me throught the whole process ❤️

Thank you very much for your help and patience, Master.

** "pay grade" wow this one is new to me, I will use it with my uppers 😉