Remove or replace usages of Shortcut from themes

Let's get a review of this

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Okay from slack seems like this approach is the least disruptive.

This needs more detail. This should be a summary of the discussion and who participated. A link to the discussion is fine, but we can't rely on a Slack link for documenting our issue discussions.

I think I found a couple of bad image file paths. But I also tried to help figure out what needs to be done with those third party settings. See what you think.

wonder if #3402885: olivero.settings config schema is wrong addresses the olivero.schema concerns?

I'd completely forgotten about that issue, despite having worked on it.

After doing additional analysis my answer is "No." But, it brings additional problems with this MR to light. Ultimately, I think that other issue needs to be closed as a duplicate since there's no point to enacting the change now. Some of the code from its MR needs to be copied here before that.

• This MR 15667 has no update function for existing Olivero settings that contain the erroneous third-party setting. I suggest copying the update function and its test from MR 5481 over here. We're in the awkward position of having to introduce new code related to a module that's being removed from Core, but that's a consequence of these improperly created settings. We may need to add a note in the update function docblock to say something to the effect of "Don't delete this update function when removing Shortcut. @see [THIS URL]." so no one has to wonder why it's there. If there's another option, please suggest it.
• That bad third-party setting was the only way to make Olivero display shortcut actions. After applying MR 15667, the setting does not get configured either on install or upon saving Olivero's settings (manually tested this evening by me). This makes sense because I couldn't find any code to do it. As noted on the MR, Claro has the setting configured via hooks. This is the only way it happens for any theme now. Those hooks need to be updated to work for Olivero in addition to Claro.
• I have triple-checked default_admin to see how Shortcut affects it like it does Claro and Olivero. I can't find shortcut action links on the front end. I can't find anything in the code. It doesn't help that "shortcut" is a three-times overloaded term in default_admin; referring to the favicon, keyboard shortcuts for expanding the navigation or toolbar, and the actual Shortcut module via the styles and icons. Are these dead styles and icons we're moving? Or am I completely missing something?

Shortcuts are available in both Claro and Default Admin when Navigation is enabled, although when you have no shortcuts it is not obvious how to set up the first one, unlike with Toolbar which showed it up front. Steps to reproduce:

1. Install from main
2. (optional) Install Default Admin and set as admin theme
3. Install Shortcut
4. Go to Configuration > User interface > Shortcuts
5. Go to List links on the default set
6. Add a shortcut and save

Your shortcut set now appears at the top of the navigation panel, under Shortcuts.

Regarding the star that appears in the page title; by default this appears in Claro and not Default Admin. This is because of the theme setting third_party_settings.shortcut.module_link.

ShortcutHooks::themesInstalled() explicitly handles Claro only:

  /**
   * Implements hook_themes_installed().
   */
  #[Hook('themes_installed')]
  public function themesInstalled($theme_list): void {
    // Theme settings are not configuration entities and cannot depend on
    // modules so to set a module-specific setting, we need to set it with
    // logic.
    if (in_array('claro', $theme_list, TRUE)) {
      \Drupal::configFactory()->getEditable("claro.settings")->set('third_party_settings.shortcut.module_link', TRUE)->save();
    }
  }

If we add default_admin here:

    if (in_array('default_admin', $theme_list, TRUE)) {
      \Drupal::configFactory()->getEditable("default_admin.settings")->set('third_party_settings.shortcut.module_link', TRUE)->save();
    }

then reinstall Default Admin, the shortcut star does show up and works, but the placement is off (the text is shown because I hovered over the star while taking the screenshot):

@smustgrave asked me to comment on this issue. I haven't looked at any of the MRs on this issue yet, just read the comments and done some manual testing and investigation into the codebase.

Currently, both Olivero and Claro support Shortcut via third_party_settings.shortcut.module_link. For reasons I haven't dug into Olivero sets this in core/themes/olivero/config/install/olivero.settings.yml whereas Claro has this set by Shortcut itself in hook_install and hook_themes_installed.

In order to decouple Shortcut from Olivero I think we should take the same approach as Claro does: Shortcut should set the setting by itself in hook_install and hook_themes_installed. shortcut_uninstall should also take care of removing the setting again. This should mean that it's self contained in Shortcut module; when upgrading to Drupal 12 users will either need to download Shortcut from contrib, or uninstall it first.

Regarding the CSS I think any Shortcut CSS from Olivero or Claro will need to move to libraries in Shortcut and be attached as necessary by Shortcut itself.

Per #24 I think we can drop Shortcut support from Default Admin immediately, assuming that Navigation is purely responsible for displaying shortcuts and there is no explicit theme support there; we might need another issue to decouple Shortcut from Navigation? The only way to enable the star feature is by manually configuring the third party setting; when Shortcut is in contrib it can add support for Default Admin then if anyone wants to do the work. Similarly Shortcut in contrib can figure out a way of adding support to other themes, if anyone wants that feature.

If we want to reduce scope and perhaps make things easier we could break this into one issue per theme?

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I think this is pretty close, I'm not sure the best way to test this. Do we pull it down and search the theme for shortcut? Then enable shortcut and confirm they work correctly?

Ok I pulled this down and searched in themes.

There are no references to shortcut I can find except in stable9 which isn't part of this issue.

I then installed drupal fresh, no shortcut as expected.
I then installed shorcut and tested that the icons show up in claro and olivero.

Clicking the star adds them to shortcuts and clicking again removes them, I think this is ready!

Hook and css moves look right.
The backport folder is a bit strange, but this is moving to contrib so that can be cleaned up.

Not entirely sure about adding olivero settings in shortcut, but it does show up in the page as expected.

Scratch that, I think shortcut needs an uninstall hook to remove olivero.settings.third_party_settings.shortcut.module_link right?

Yes, that pulls olivero's instance out, but we set it on shortcut install, so now we need to delete it when shortcut is uninstalled right?

Looks great to me!

Committed/pushed to main and cherry-picked to 11.x and 11.4.x, thanks!