Problem/Motivation

Views provided menu links are lost after cache clear/rebuild because the changed config is not stored in core.menu.static_menu_link_overrides. I wrote a test case to explain the issue.

Steps to reproduce:
- Create a View with a 'page' dispaly
- Add a menu link
- Disable the menu link in the Menu UI
- Clear caches

Expected result:
Menu link is disabled.

Actual result:
Menu link is enabled.

Proposed resolution

Add options for expanded/enabled to the config schema so that their status gets stored.

Don't provide a UI though - this is just to preserve them.

Remaining tasks

User interface changes

API changes

Comments

webflo’s picture

FileSize
3.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,714 pass(es), 1 fail(s), and 0 exception(s). View

The patch shows the different behaviour of a views menu link (ViewsMenuLink class) and a default menu link form user module (MenuLinkDefault class).

webflo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: 2464077.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
4.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,708 pass(es), 2 fail(s), and 1 exception(s). View

The enabled key was missing in config schema.

webflo’s picture

FileSize
6.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,710 pass(es), 2 fail(s), and 1 exception(s). View

Exposed the config setting to views ui.

The last submitted patch, 4: 2464077-4.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: 2464077-5.patch, failed testing.

pwolanin’s picture

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?

dawehner’s picture

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?

s_leu’s picture

Status: Needs work » Needs review
FileSize
9.79 KB
7.96 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,790 pass(es). View

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.

s_leu’s picture

FileSize
9.48 KB
7.65 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,771 pass(es). View

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.

dawehner’s picture

  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.

Berdir’s picture

Status: Needs review » Needs work
s_leu’s picture

Status: Needs work » Needs review
FileSize
3.18 KB
7.71 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,895 pass(es), 5 fail(s), and 2 exception(s). View

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.

dawehner’s picture

+++ 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?

Status: Needs review » Needs work

The last submitted patch, 14: menu_links_overrides_lost-2464077-14.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
2.34 KB
7.47 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,878 pass(es). View

Because your previous review in #12 proposed to set it to FALSE ;). Nevermind is think we should use the same defaults as for normal menu links.

bircher’s picture

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.

drubb’s picture

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.

dawehner’s picture

Status: Needs review » Needs work

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,

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.

dawehner’s picture

Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
6.64 KB

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.

catch’s picture

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

dawehner’s picture

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

catch’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update +rc target

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

catch’s picture

Status: Needs work » Needs review

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.

Berdir’s picture

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

alexpott’s picture

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.

chaitanya17’s picture

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);
}

catch’s picture

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.

dawehner’s picture

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.

chaitanya17’s picture

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.

dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, 34: 2464077-34.patch, failed testing.

The last submitted patch, 34: 2464077-34.patch, failed testing.

alexpott’s picture

@dawehner yes something like that.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.82 KB
1.56 KB

Thank you alex!

Lendude’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

Lendude’s picture

Status: Needs work » Needs review
FileSize
2.13 KB
7.77 KB

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

alexpott’s picture

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.

catch’s picture

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

effulgentsia’s picture

Issue tags: +Triaged D8 critical

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

dawehner’s picture

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

alexpott’s picture

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.

dawehner’s picture

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

Lendude’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Good catch!

  • alexpott committed fa52948 on 8.2.x
    Issue #2464077 by webflo, s_leu, dawehner, Lendude: Menu link overrides...

  • alexpott committed 495a46a on 8.1.x
    Issue #2464077 by webflo, s_leu, dawehner, Lendude: Menu link overrides...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Sorry for holding this issue up.

jibran’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.