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();
Files: 
CommentFileSizeAuthor
#6 increment.txt928 bytespwolanin
#6 2403389-6.patch6.42 KBpwolanin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2403389-6.patch. Unable to apply patch. See the log in the details link for more information. View
#1 2403389.1.patch6.26 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,445 pass(es). View
#1 2403389.1-test-only.patch5.37 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,444 pass(es), 1 fail(s), and 0 exception(s). View

Comments

alexpott’s picture

Status: Active » Needs review
Issue tags: +Configuration system
FileSize
5.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,444 pass(es), 1 fail(s), and 0 exception(s). View
6.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,445 pass(es). View

I've refactored the test in StaticMenuLinkOverridesTest to make it a bit easier to follow along.

dawehner’s picture

Good catch!

I also like to split up the test coverage in two test methods.

The last submitted patch, 1: 2403389.1-test-only.patch, failed testing.

alexpott’s picture

So 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.

pwolanin’s picture

So, we should sort by the ID and sort the config within each override?

pwolanin’s picture

FileSize
6.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2403389-6.patch. Unable to apply patch. See the log in the details link for more information. View
928 bytes
dawehner’s picture

Can we please add a test to ensure that this doesn't happen? It is quite incredible annoying if this happens.

pwolanin’s picture

To clarify from IRC - needs a test written to insure that the keys and values stay ordered when new items are added or removed.

pwolanin’s picture

Issue summary: View changes
Issue tags: +Needs tests

mgifford queued 6: 2403389-6.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2403389-6.patch, failed testing.

catch’s picture

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pwolanin’s picture

Version: 8.2.x-dev » 8.4.x-dev
Issue summary: View changes
Issue tags: +DrupalCampNJ2017, +Triaged D8 major

Checking the code this is evidently still a bug