Rename menu ID to use hyphens #340
Conversation
|
Here's what we need to update |
|
I've updated block configuration here, there are some code changes to Localgov_demo that will need to be carried out and merged at the same time if we decide to go with this. I couldn't find any references in localgov_base. If I've missed something @markconroy it would be good to check the theme can still work with this change. I haven't made an update hook. Is that something we require for this change or are we happy it's new sites only. Presumably most who have run into the issue have done what BHCC did and create our own menu a long time ago so it's not relevant. |
|
It appears as long as the menu name is |
|
@andybroomfield I think I'd like an update hook for this (maybe as a follow up if we wish) but it definitely seems to be a bug that we have that we should fix across the board. |
|
Some thoughts on supporting existing sites. I’m not sure we can do update hooks because we won’t know the naming for the active theme block config? Is it possible that theming overrides can depend on the underscores too? One idea is a snippet to bypass Drupal core’s validation for that specific machine name? |
… link content This will - Create the new menu, `localgov-services-menu` - Update menu_link_content entities (user created menu items) to set the menu name to `localgov-services-menu` from `localgov_services_menu` - Delete the old menu `localgov_services_menu`
|
@markconroy @tonypaulbarker I've pushed an update hook for review. I only think I can update the machine name of the menu by creating a new one and moving the items over. But any other configuration I'm not so sure about in terms of how we go about it. Would we have to discover each block and then try and update them, and would it effect a theme. |
|
I'm also testing. |
|
Just fixed some coding standards so the rest of the tests can run. |
|
Update hook works nicely for me @andybroomfield Before: after: |
|
Discussing in Merge Tuesday: For existing sites block placements is an issue, attempting to delete the old menu will remove the block placement. Andy suggests we create the menu, copy links to the new menu but don's delete the old menu. Add rerelease notes to be clear that developers need to:
|
In order to prevent existing sites breaking - Check if an existing `localgov-services-menu` exists, and abort if so. - Duplicate menu content items instead of moving them. - Relabel the existing `localgov_services_menu` indicating it should be deleted once the blocks have been replaced.
AWearring
left a comment
AWearring
left a comment
There was a problem hiding this comment.
Tested on the West Lindsey build where we had already "fixed" the issue by renaming the menu via a patch. localgov_services happily updated and the update hook gets skipped because of if ($existing).
Will try testing on a new LGD vanilla build later
AWearring
left a comment
AWearring
left a comment
There was a problem hiding this comment.
Have no tested on an old LGD vanilla build I had locally and the update hook correctly copies the Services Menu and updates the existing menu to make it obvious that it should be removed
|
This PR has been recreated on Drupal.org |
|
Should we close the PR here as merged on Drupal.org? |
|
This is merged on drupal.org https://git.drupalcode.org/project/localgov_services/-/merge_requests/3 Cosing here |
Fix #307
Fix #140
What does this change?
Rename the menu
localgov_services_menuto use hypens as inlocalgov-services-menuNeed to check if this is actually used in places apart from config and if other modules (localgov_demo) or themes (localgov_base) need to be updated to account for the change.
How to test
Go to /admin/structure/menu/manage/localgov_services_menu and save the menu.
(Requires fresh install).
How can we measure success?
No more errors when trying to edit the services menu with
Have we considered potential risks?
Menu name is used in places independently (custom themes).
Images
n/a
Accessibility
n/a