Overridden menu links lose parent, expanded and enabled (are disabled) status on cache clear

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

Let's see if the test fails.

Yeah 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

taking a look

Tentatively tagging beta target, since I believe it blocks the closing of #2341575: [meta] Provide a beta to beta/rc upgrade path.

I'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.

So the issue is the \Drupal\Core\Menu\MenuLinkDefault::updateLink passes on only the changed keys and the menu name.

e.g.

array(
  'weight' => -10,
  'menu_name => 'main,
);

and then in \Drupal\Core\Menu\StaticMenuLinkOverrides::saveOverride we do this

$expected = array(
      'menu_name' => '',
      'parent' => '',
      'weight' => 0,
      'expanded' => FALSE,
      'enabled' => FALSE,
    );
    // Filter the overrides to only those that are expected.
    $definition = array_intersect_key($definition, $expected);
    // Ensure all values are set.
    $definition = $definition + $expected;

So 'enabled' FALSE, 'expanded' FALSE, 'parent' '' all get added to the stored values.

Let's see what else breaks

New title, as its not just status

This is ready.

+++ b/core/lib/Drupal/Core/Menu/MenuLinkDefault.php
@@ -96,7 +96,7 @@ public function updateLink(array $new_definition_values, $persist) {
-      $this->staticOverride->saveOverride($this->getPluginId(), $overrides);
+      $this->staticOverride->saveOverride($this->getPluginId(), $this->pluginDefinition);

Nice fix.

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

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

* @return array
* The plugin definition incorporating any allowed changes.

Just checked the other implementations and they are fine.

I'm sorry but I could not resist to add a unit test

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

@dawehner Interesting, OK. Well if it is a separate issue then I guess they'll get credit if/when that lands :)

Committed 663848e and pushed to 8.0.x. Thanks!

This issue did not fixed #2598488: Views Page display menu expanded option is not included and gets destroyed by cache-rebuild