Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The Standard profile adds a "Home" link through its standard.links.menu.yml
. When this menu item is re-ordered (the weight changes to a non 0 value) and saved gets disabled with the next clearing of caches.
I created a test to demonstrate the issue, it can be moved or merged to another test once it is more clear where it should go.
It is very easy to reproduce:
- Install standard.
- Add a menu item to the main menu (through a node or any other way, this is not even necessary)
- Reorder the menu items (if you do it with js you can also reorder any item, as this results assigning weights to all items)
- Save the reordered list.
- Enjoy the menu as you configured it.
- Clear all caches
- Find the Home item disabled.
The only work around is to manually set the weight of "Home" to 0 as then it survives clearing caches.
Comment | File | Size | Author |
---|---|---|---|
#14 | interdiff.txt | 2.23 KB | dawehner |
#14 | 2548009-13.patch | 5.5 KB | dawehner |
#10 | menu-weight-2548009.8.patch | 3.27 KB | larowlan |
#10 | interdiff.txt | 719 bytes | larowlan |
menu-link-reorder-TEST-ONLY.patch | 2.57 KB | bircher | |
Comments
Comment #2
bircherComment #3
catchSo we're not noticing this in HEAD because people aren't developing on core by customizing menu links much.
But in actual site building, setting up your main/secondary/user menu how you want it, then those links getting disabled every cache clear is a horrible horrible issue, in a similar way to #2505989: Controllers render caching at the top level and setting a custom page title lose the title on render cache hits except it looks like this additionally involves data loss.
Tagging D8 upgrade path because this was re-discovered in #2341575: [meta] Provide a beta to beta/rc upgrade path and even if it's not the upgrade path being broken, it certainly looked like it from plach's testing - if someone hadn't already known about this issue we could have lost hours of debugging time.
The combination of customizing menu links being quite basic functionality, cache clearing being non-optional and data loss makes this critical I think.
Comment #4
tim.plunkettLet's see if the test fails.
Comment #5
dawehnerYeah its a pretty damn problem, on the other hand I have seen sites where things actually work, they also had a working update path, so this seems to be a edge case triggered by something. I guess the overridden part is the key indeed. Will have a look at that tomorrow morning
Comment #7
larowlantaking a look
Comment #8
webchickTentatively tagging beta target, since I believe it blocks the closing of #2341575: [meta] Provide a beta to beta/rc upgrade path.
Comment #9
catchI'm on the fence about that. On the one hand it's not actually an upgrade path issue. On the other, it's impossible to distinguish between this and an upgrade gone wrong, and represents data loss on new sites as well as ones upgrading from betas. So it probably should be yes.
Comment #10
larowlanSo the issue is the
\Drupal\Core\Menu\MenuLinkDefault::updateLink
passes on only the changed keys and the menu name.e.g.
and then in
\Drupal\Core\Menu\StaticMenuLinkOverrides::saveOverride
we do thisSo 'enabled' FALSE, 'expanded' FALSE, 'parent' '' all get added to the stored values.
Let's see what else breaks
Comment #11
larowlanNew title, as its not just status
Comment #12
jibranThis is ready.
Nice fix.
Comment #13
rootworkPresumably this would also close #2468713: Internal/custom menu links force-reset to the top of their menus on cache rebuild -- there's a patch there too, but this one is much more recent.
Not sure if this patch pulled from that at all, but as I _have_ been developing on D8 and (clients) use custom links, I deeply appreciated the work that StryKaizer, paulmckibben and jhedstrom put into that patch, in case it seems worth adding some credit for them.
Comment #14
dawehnerOh nice catch. You know, this is another classical example of a problem of using arrays ... If you look at the documentation, it clearly says what you should return.
Just checked the other implementations and they are fine.
I'm sorry but I could not resist to add a unit test
Comment #15
dawehner@rootwork
If I read the other issue correctly, this is a different issue, especially see the comment of paulmckibben #2468713-33: Internal/custom menu links force-reset to the top of their menus on cache rebuild
Sadly d.o. requires people to make a comment to give them credit at the moment.
Comment #16
rootwork@dawehner Interesting, OK. Well if it is a separate issue then I guess they'll get credit if/when that lands :)
Comment #17
alexpottCommitted 663848e and pushed to 8.0.x. Thanks!
Comment #20
dawehnerThis issue did not fixed #2598488: Views Page display menu expanded option is not included and gets destroyed by cache-rebuild