Problem/Motivation

$ git grep -li shortcut core/modules/navigation/
core/modules/navigation/config/install/navigation.block_layout.yml
core/modules/navigation/src/Hook/NavigationHooks.php
core/modules/navigation/src/NavigationServiceProvider.php
core/modules/navigation/src/Plugin/Block/NavigationShortcutsBlock.php
core/modules/navigation/src/ShortcutLazyBuilder.php
core/modules/navigation/tests/navigation_test/src/Hook/NavigationTestHooks.php
core/modules/navigation/tests/src/Functional/NavigationSafeBlockDefinitionTest.php

Steps to reproduce

Proposed resolution

Includes removing Shortcut from ConfigImportAllTest.php because Navigation uses simple config incorrectly. See #13

Remaining tasks

Review
Review the change record

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3581816

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

quietone created an issue. See original summary.

quietone opened merge request !15231

smustgrave’s picture

smustgrave commented
Assigned: Unassigned » smustgrave

Going to try and help this weekend

smustgrave’s picture

smustgrave commented
Status: Active » Needs review
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Needs work

NW per MR review.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Feedback should be addressed.

dcam’s picture

dcam commented
Status: Needs review » Needs work

I found a few things and added comments to the MR.

smustgrave’s picture

smustgrave commented

I can't resolve threads but can someone do that not sure what's left for this one.

dcam’s picture

dcam commented

@smustgrave, the unresolved action items are:

quietone’s picture

quietone commented

I've yet to see why ConfigImportAllTest.php is failing on 'search_help'.

Drupal\Tests\config\Functional\ConfigImportAllTest::testInstallUninstall
Module "search_help" is enabled.
Failed asserting that false matches expected true.

core/modules/system/tests/src/Functional/Module/ModuleTestBase.php:149
core/modules/config/tests/src/Functional/ConfigImportAllTest.php:167

https://git.drupalcode.org/project/drupal/-/pipelines/804483/test_report...

quietone’s picture

quietone commented

From the browser output it is failing when re-installing navigation.

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for navigation.block_layout with the following errors: 0 [sections.0.components.00b005e5-d42b-4685-9dc7-272438fbec6b.configuration.provider] Module 'shortcut' is not installed. in Drupal\Core\Config\Development\ConfigSchemaChecker->onConfigSave() (line 98 of core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php).

quietone’s picture

quietone commented
Issue summary: View changes

The Shortcut module now uses hook_navigation_defaults to define default content for the Navigation bar. This used to be defined in the navigation module. So, when the configuration is exported in ConfigImportAllTest the navigation.block_layout contains config that requires the Shortcut module. The test installs everything and exports config. It then uninstalls everything and imports the config. It is the config import that fails with the error shown in the previous comment, that Shortcut is not installed. So, by using hook_navigation_defaults it has navigation now has a dependency on Shortcut. At least that is what I see. How to resolve this?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

There is no clean solution to this. The fact that navigation module store plugin configuration with dependencies in a simple config object is fundamentally wrong and not supported. Simple config can not depend on other modules/config. Similar problems would happen if you place a menu block there and then remove that menu.

Since this is about shortcut being removed, I'd go with a quickfix, that is, exclude shortcut from this test, so we don't run into this situation for this test. AFAIK we do that anyway with deprecated modules, so we're just doing it a bit early in this case.

The only proper fix would be to refactor how navigation module stores it's configuration, for example by putting each block placement into it's own config entity, not unlike block.module handles placed blocks. hook_navigation_defaults() wouldn't even need to exist anymore then, shortcut.module could just provide a default block. And it would also automatically handle dependency removal, removing that block when you uninstall shortcut.

quietone’s picture

quietone commented
Assigned: smustgrave » Unassigned
Issue summary: View changes
Status: Needs work » Needs review

@berdir thanks! I made an issue in Navigation, #3587499: Navigation stores plugin configuration with dependencies in a simple config object

dcam’s picture

dcam commented
Status: Needs review » Needs work
Issue tags: +Needs change record

There are a couple of items to take care of that I left comments about in the MR. There is a loss of test coverage that needs to be ported to Shortcut. And the CR URLs in the deprecation notices are still placeholders. Also, we'll need a change record.

quietone’s picture

quietone commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs change record

@dcam, thanks again. Yes, I put zeros in the change record URL and intend to get back to fix it. But too often I forget to do that.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

Yes, I put zeros in the change record URL and intend to get back to fix it. But too often I forget to do that.

That's fine. I wasn't 100% sure if that was intentional because you were waiting on some feedback or for some other reason. But I figure it's better to document a finding than not. That way there's less chance that someone else comes along to review, misses the placeholder, and then RTBCs the MR. Thank you for considering my feedback.

Speaking of which, my feedback has been addressed. I think this is ready to send up.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review

I'm not sure about one of the comments in the MR. It's pointing to a closed issue - so if it is fixed do we need this code - or is it pointing to the wrong issue?

quietone’s picture

quietone commented

I had a go at improving the comment.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

I think @alexpott's concerns from #18 have been addressed. The comment's point is clearer to me now than it was before.

quietone’s picture

quietone commented
Title: Moves uses of Shortcut from Navigation to Shortcut » Move uses of Shortcut from Navigation to Shortcut
godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

Some comments on the MR.

Couple more thoughts:

  • I think we can use moved_classes service parameters to move NavigationShortcutsBlock and the navigation ShortcutLazyBuilder. See https://www.drupal.org/node/3509577
  • There needs to an update path if an existing site has Navigation installed and Shortcut uninstalled. Any navigation_shortcut_block component needs to be removed from navigation.block_layout config, because now that block plugin won't exist anymore
  • Mentioned this on the MR, but there's also some complexity on install/uninstall. With the changes, if a site has Shortcut already installed, and Navigation uninstalled, then if Navigation is installed, the navigation_defaults hook is invoked as expected, and the shortcut block is added. OTOH, if Navigation is already installed and Shortcut uninstalled, then if Shortcut is installed the navigation_defaults hook is not invoked, so the the shortcut block does not get added. I think maybe there needs to be code in shortcut_install() to add it, but there may be problems trying to add to navigation.block_layout in the situation where navigation and shortcut are both uninstalled, but are installed in the same request. See MR comment.
smustgrave’s picture

smustgrave commented

addressed the first 2 bullets but not sure what to do about the third.

smustgrave’s picture

smustgrave commented

test failure seems relevant to the change in shortcut.install now.

godotislate’s picture

godotislate
he/him
commented

test failure seems relevant to the change in shortcut.install now.

Looking at the test browser output, looks like there are two instances of the shortcut block now. My mistake, hook_navigation_defaults is invoked both on navigation_install (hook_install) AND NavigationHooks::modulesInstalled (hook_modules_installed), so the change to shortcut_install is not needed.

Still, we should manually test the following scenarios
1. Install minimal profile. Then install navigation and shortcut together
2. Install minimal profile. Then install navigation. Then install shortcut
3. Install minimal profile. Then install shortcut. Then install navigation

And make sure the shortcuts block appears in navigation as expected.

smustgrave’s picture

smustgrave commented

Since this is the last blocker for shortcut to get removed, asked Claude for some help and it added a check to see if shortcut was already in the navigation. Lets see if that turns green.

smustgrave’s picture

smustgrave commented

All green what do we think or do we need to add more coverage per 25

godotislate’s picture

godotislate
he/him
commented

All green what do we think or do we need to add more coverage per 25

Unless I missed it, I don't see an update test in the MR, so we do need that.

I think confirmation of #25 with manual tests is probably fine.