Menu link overrides are lost after cache rebuild

The issue summary here doesn't make sense. Views links should never be tracked by the config core.menu.static_menu_link_overrides

So either the issue summary is wrong, or I think you misidentified the bug. Possibly we forgot to flag the Views links as discovered?

The issue summary here doesn't make sense. Views links should never be tracked by the config core.menu.static_menu_link_overrides

Well yeah, we figured that out in the meantime. If you look at the patch it does something different, it enables the support for adding a status flag storage in views itself.

+++ b/core/modules/views/tests/src/ViewsMenuLinkTest.php
@@ -0,0 +1,103 @@
+ */
+class ViewsMenuLinkTest extends WebTestBase {
+
...
+    $storage = $this->entityManger->getStorage('view');
+
+    $data = \Symfony\Component\Yaml\Yaml::parse(drupal_get_path('module', 'views_test_config') . '/test_views/views.view.test_page_display_menu.yml');
+    $view = $storage->createFromStorageRecord($data);
+    $view->save();
+

Is there a reason we can't use the ViewTestBase and the static $testViews mechanism?

Here's a reroll of the patch with some changes. I added testing of the expanded key (which i also added in the schema of the page display menu link).

I removed metadata as i didn't find any possibility to edit/set this from the UI, so i don't see a reason to keep this. Please correct me if i'm wrong here.

Removed some left over that was accidentally included in the last patch. I wasn't sure whether it really makes sense to add the expanded flag also to the page displays link form. In fact i also doubt it makes sense to add the enabled option there as nobody would configure a link in views and then disable it. IMHO it is enough if that is possible from the menu configuration. Correct me if i'm wrong here.

Anyway i kept the enabled option form element in the patch for now. I can yet remove that quickly, if you guys agree.

1. +++ b/core/modules/views/src/Plugin/views/display/Page.php
   @@ -132,6 +132,7 @@ protected function defineOptions() {
            'title' => array('default' => ''),
            'description' => array('default' => ''),
            'weight' => array('default' => 0),
   ...
            'menu_name' => array('default' => 'main'),
            'parent' => array('default' => ''),
            'context' => array('default' => ''),
   
   +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -324,6 +324,10 @@ public function getMenuLinks() {
   +        if (isset($menu['expanded'])) {
   +          $links[$menu_link_id]['expanded'] = $menu['expanded'];
   +        }
   

   IMHO we should also add a default for expanded

2. +++ b/core/modules/views/src/Plugin/views/display/Page.php
   @@ -132,6 +132,7 @@ protected function defineOptions() {
   +        'enabled' => array('default' => 1),
   

   should be boolean false I think

3. +++ b/core/modules/views/src/Tests/ViewsMenuLinkTest.php
   @@ -0,0 +1,106 @@
   +
   +  protected function setUp() {
   +    parent::setUp();
   

   {@inheritdoc} missing

4. +++ b/core/modules/views/src/Tests/ViewsMenuLinkTest.php
   @@ -0,0 +1,106 @@
   +    /**
   +     * Link from views module.
   +     */
   ...
   +
   +    /**
   +     * Link from user module.
   +     */
   

   Let's use // instead.

Made the suggested changes.

I checked the view ui again now and i really don't agree with the way it is now.

First thing is that If you want to add a menu link now you will have to actively enable the checkbox for the "enabled" option of the menu link, which has a default value of false. So that is one unnecessary click more which is not very nice usability wise, because when you intend to add your view to a menu, you will in most cases want the link to be active already after submitting the form in views.

Besides that, if adding the enabled option to that form, the expanded option should be there as well to stay consistent for all menu link options. Obviously adding the expanded option to the UI makes even less sense than the enabled option.

Please consider this when reviewing the patches.

+++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -132,7 +132,8 @@ protected function defineOptions() {
+        'enabled' => array('default' => FALSE),

This is odd, why do you think it should be disabled by default?

A related bug also exists for menu items that come from yaml files.
#2548009: Overridden menu links lose parent, expanded and enabled (are disabled) status on cache clear
The test patch from there unsurprisingly also fails with this patch, but the underlying problem is probably related.

From a UX perspective, what's the reason to expose the enabled and expanded options in the views UI at all? IMHO the views UI will hardly ever be used to toggle those options, most people will prefer using the normal menu ui. So we would just have to assure that views won't change existing settings for those options.

I agree with @drubb to be honest, exposing that in the UI is wrong. Adding it on the data level is kinda needed though still.
Let's drop it,

The fact that we don't save something aka. something gets destroyed on cache rebuild is sort of a critical for me, to be honest.

@dawehner are you thinking of removing this from the UI in this issue or a follow-up?

@catch
I forgot to attach an interdiff, but this most recent patch already doesn't add a UI for it.

Oh sorry I get it. There's no UI for it now, and there's still no UI with the most recent patch.

I made a minimal update to the issue summary to reflect current state. This looks good to me so RTBC. Also tagging RC target.

I think we need an upgrade path here because the config will change on a cache rebuild.

I'm not sure it's possible to provide a useful update:

1. If the cache has already been rebuilt, the information is already lost and can't be recovered.

2. The case where the cache hasn't been rebuilt, then we run the update before there is one, seems extremely unlikely and not worth trying to account for.

If I'm wrong about this, then yes we need an update, but this is why I didn't ask for one.

I'd agree, it seems pretty unlikely that we'd manage to salvage something before it gets lost.

My point was not really about salvaging anything - but we've new keys in the file so I think we need to add these as part of a hook_update_N so the file doesn't change unexpected on a cache rebuild.

I have came across same issue on production site, in my case main menu was not getting rendered after feature and cache rebuild

I have written theme hook in template.php
/**
* for adding main menu content
* @param type $variables
* @return type
*/
function theme_links__system_main_menu($variables) {
variable_set('menu_main_links_source', 'main-menu');
$pid = variable_get('menu_main_links_source', 'main-menu');
$tree = menu_tree($pid);
return drupal_render($tree);
}

This issue only applies to 8.x, the code sample suggests 7.x, and it's most likely a bug in features menu integration which has never been great.

However looking at the code sample the following stuck out:

*/
function theme_links__system_main_menu($variables) {
variable_set('menu_main_links_source', 'main-menu');

It looks like this variable_set() will run on every request, which is going to be very bad for your site's performance - also can't see why the variable_set()/variable_get() is necessary in there.

My point was not really about salvaging anything - but we've new keys in the file so I think we need to add these as part of a hook_update_N so the file doesn't change unexpected on a cache rebuild.

Nothing in views at least requires new items to be part of files, but I see your point.

for #31

We can add isset for variable_set('menu_main_links_source') isn't an issue, The reason I have added variable set, was after clearing cache this variable becomes unset, and issue was specific to particular instance.

@alexpott
Do you want to see something like this, otherwise please clarify what you think is needed here.

@dawehner yes something like that.

Thank you alex!

Manually tested this and bug still occurs in current HEAD, added steps to reproduce in issue summary.

Feedback from @alexpott in #26 was addressed

+++ b/core/modules/views/tests/src/Kernel/Plugin/Display/ViewsMenuLinkTest.php
@@ -0,0 +1,98 @@
+   * The menu link overides.

Missing r in overrides, fix on commit?

Other then that the patch looks good to go to me.

+++ b/core/modules/views/src/Plugin/Menu/ViewsMenuLink.php
@@ -26,7 +26,6 @@ class ViewsMenuLink extends MenuLinkBase implements ContainerFactoryPluginInterf
-    'metadata' => 1,

This change looks out-of-scope here. I think it needs to be looked at separately.

@alexpott since that line causes an invalid schema fail in the tests, either it has to go or we need to provide a valid schema for it (or rewrite the test to avoid it).

So, now with a valid schema (well, one that passes the test locally anyway) for metadata. Plus some clean up stuff.

Well metadata is definitely part of the MenuLinkContent plugin - see \Drupal\menu_link_content\Plugin\Menu\MenuLinkContent

    if (!empty($this->pluginDefinition['metadata']['entity_id'])) {
      $entity_id = $this->pluginDefinition['metadata']['entity_id'];
      // Builds a list of entity IDs to take advantage of the more efficient
      // EntityStorageInterface::loadMultiple() in getEntity() at render time.
      static::$entityIdsToLoad[$entity_id] = $entity_id;
    }

So I think allowing it to be configured in views at least leaves options open.

@alexpott see #23-25 - there's no UI for this now, and the patch doesn't add a new UI. I think we can look at that, but don't think it's worth doing to fix this critical - feels like follow-up issue which can then have usability review etc.

Discussed this with @alexpott, @catch, @xjm, and @webchick, and we agree it's Critical, since it's data loss.

+++ b/core/modules/views/config/schema/views.display.schema.yml
@@ -34,6 +34,18 @@ views.display.page:
+        metadata:
+          type: sequence
+          label: 'Metadata'
+          sequence:
+            type: string
+            label: 'Metadata'

We should remove the schema as well...

re #43 and #45 - by configured in views - I meant from the API / view configuration object level not the UI. I think the patch in #41 is good to go.

I'm personally not sure about it. If we support metadata the view object should also store it, which the patch currently doesn't.

talked to @alexpott about this at dev days and he agreed that the metadata handling can be moved to a followup issue, so we go back to #38.

Bit of a cleanup

1. +++ b/core/modules/views/src/Plugin/views/display/Page.php
   @@ -124,6 +124,8 @@ protected function defineOptions() {
   +        'expanded' => array('default' => FALSE),
   

   was already getting set a couple of rows further on, so no need to add this.

2. +++ b/core/modules/views/views.install
   @@ -363,3 +363,34 @@ function views_update_8005() {
   +function views_update_8006() {
   

   set this into the 8.1 updates

Good catch!

Committed 495a46a and pushed to 8.1.x and 8.2.x. Thanks!

Sorry for holding this issue up.

Are we going to add the upgrade path test in followup?