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

Is the manual testing in #25 complete? See the limited results in #29.
Add UpdateTest
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.

quietone’s picture

quietone commented
Issue summary: View changes
Issue tags: +11.5.0 release priority

I did the manual testing outlined in #25 and didn't encounter any errors. In all cases I was able to create a shortcut and edit the name of the set. However, #25 doesn't state what is to be tested. So, I have no idea if that is complete or not.

I rebased the MR and updated the versions in the change record.

A reminder to use tags and update the issue summary. If this had been tagged for ' needs manual testing' I would have found this a few weeks ago.

quietone’s picture

quietone commented
Status: Needs work » Needs review

I had a go at an update test.

smustgrave’s picture

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

Reviewed https://git.drupalcode.org/project/drupal/-/merge_requests/15231/diffs?c... and worked for me (as in ran). This one good to go?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Some questions about the moved_classes setup - I thought the point of that was that we didn't need to leave the old class in place at all, but also what happens if the target module is not installed?

Untangling this sort of thing is super tricky so thanks for the work so far, most of it looks good - this is not easy at all.

quietone’s picture

quietone commented
Status: Needs work » Needs review

#22 suggested using moved_class for NavigationShortcutsBlock and the navigation ShortcutLazyBuilder. NavigationShortcutsBlock was done but it left the 'old' class with the typical deprecation. Which method should be use for NavigationShortcutsBlock? ShortcutLazyBuilder was not done. Should it?

Changing status to get opinions.

godotislate’s picture

godotislate
he/him
commented

Re: #33, yes if moved_classes is used, then the original classes should be deleted, and the moved_classes functionality should provide the deprecation warnings automatically.

Or if we don't use moved_classes, then the original classes should still exist but with manual deprecation.

quietone’s picture

quietone commented

OK, then I have removed the moved_classes simply to keep the style of all I've used in other extension deprecations.

smustgrave’s picture

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

Believe feedback about not doing moved_classes has been addressed. Lets land this one hopefully :)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

Left some MR comments

quietone’s picture

quietone commented
Status: Needs work » Needs review

@larowlan, thanks for the review!

I've updated the review based on the feedback. The failing test is unrelated.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

I checked the feedback was resolved and closed the remaining thread.

On manual code review I was surprised that there was no update to the "new block name", then realized that even though its a new block class and moved to the Shortcut module the block name is still namespaced/named as navigation_shortcuts. I can see the appeal of keeping the name even though we would not name it like that were it a new block in shortcut module. (I assume it would be shortcut_navigation or something?) I don't think its a big deal to keep the old name and it does simplify things, so let's not block on that further IMHO.

Otherwise looks good to me.

  • catch committed ab8745cf on main 
    task: #3581816 Move uses of Shortcut from Navigation to Shortcut

By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

This looks good to me. I wasn't sure about the extra shortcuts/navigation test fixture vs adding that to the 11.3.0 dump, but then I realised that fixture will go away when we remove the update, so it makes sense to do it like that.

Committed/pushed to main, thanks!

Will need a backport MR for 11.x

quietone opened merge request !16710

quietone’s picture

quietone commented
Status: Patch (to be ported) » Needs work

Grrr! This should be reverted. Just like #3614949: Move tests to Olivero, part 1 there is a namespace error.

There were conflicts were in:

  • \Drupal\Tests\config\Functional\ConfigImportAllTest::testInstallUninstall
  • \Drupal\Tests\navigation\Kernel\ConfigAction\AddNavigationBlockConfigActionTest::testAddBlockToNavigation

  • catch committed ac2adde1 on main 
    Revert "task: #3581816 Move uses of Shortcut from Navigation to Shortcut...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » main

Pushed the revert.

quietone reopened merge request !15231

quietone’s picture

quietone commented
Version: main » 11.x-dev
Status: Needs work » Needs review

The test failure on 11.x is Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTagsTest::testFullHtml, a known random failure.

quietone’s picture

quietone commented
Version: 11.x-dev » main
smustgrave’s picture

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

Since failure was around namespace https://git.drupalcode.org/project/drupal/-/merge_requests/15231/diffs?c... looks to fix it. Restoring RTBC.

  • catch committed 27ae1229 on main 
    task: #3581816 Move uses of Shortcut from Navigation to Shortcut

By:...

  • catch committed 7926fc32 on 11.x 
    task: #3581816 Move uses of Shortcut from Navigation to Shortcut

By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main, thanks!

I also committed/pushed to 11.x but composer is failing on 11.x for me today, so had to use --no-verify on the commit.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

quietone’s picture

quietone commented

Published the change record

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.