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.Problem/Motivation
In \Drupal\Core\Menu\StaticMenuLinkOverrides::saveOverride) we encode the $id but the id is also encoded in StaticMenuLinkOverrides::loadOverride() which means that the configuration is not merged as expected because loadOverride does not find an override.
Proposed resolution
Fix double encoding and sort the config before saving it
Remaining tasks
Add a test to insure the overrides remain sorted
Original report by @alexpott
In StaticMenuLinkOverrides::save() we encode the $id but the id is also encoded in StaticMenuLinkOverrides::loadOverride() which means that the configuration is not merged as expected because loadOverride does not find an override.
$id = static::encodeId($id);
$all_overrides = $this->getConfig()->get('definitions');
// Combine with any existing data.
$all_overrides[$id] = $definition + $this->loadOverride($id);
$this->getConfig()->set('definitions', $all_overrides)->save();
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | increment.txt | 928 bytes | pwolanin |
| #6 | 2403389-6.patch | 6.42 KB | pwolanin |
| #1 | 2403389.1.patch | 6.26 KB | alexpott |
| #1 | 2403389.1-test-only.patch | 5.37 KB | alexpott |
Comments
Comment #1
alexpottI've refactored the test in
StaticMenuLinkOverridesTestto make it a bit easier to follow along.Comment #2
dawehnerGood catch!
I also like to split up the test coverage in two test methods.
Comment #4
alexpottSo there is also a problem that the order the merged keys will be saved in is completely dependent on the order that one does the saves in. This is problematic since configuration that is same wrt to how it affects the system is not the same in the store leading to potentially incorrect reports of differences on the config sync screen.
Comment #5
pwolanin CreditAttribution: pwolanin commentedSo, we should sort by the ID and sort the config within each override?
Comment #6
pwolanin CreditAttribution: pwolanin commentedComment #7
dawehnerCan we please add a test to ensure that this doesn't happen? It is quite incredible annoying if this happens.
Comment #8
pwolanin CreditAttribution: pwolanin commentedTo clarify from IRC - needs a test written to insure that the keys and values stay ordered when new items are added or removed.
Comment #9
pwolanin CreditAttribution: pwolanin commentedComment #12
catchComment #13
catchTrying again adding that related issue.
#2548009: Overridden menu links lose parent, expanded and enabled (are disabled) status on cache clear.
#2464077: Menu link overrides are lost after cache rebuild.
Comment #16
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedChecking the code this is evidently still a bug