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
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
Comment | File | Size | Author |
---|---|---|---|
#48 | 2464077-48.patch | 7.68 KB | Lendude |
#48 | interdiff-2464077-38-48.txt | 2.03 KB | Lendude |
#41 | 2464077-41.patch | 7.77 KB | Lendude |
#41 | interdiff-2464077-38-41.txt | 2.13 KB | Lendude |
#38 | interdiff.txt | 1.56 KB | dawehner |
Comments
Comment #1
webflo CreditAttribution: webflo commentedThe patch shows the different behaviour of a views menu link (ViewsMenuLink class) and a default menu link form user module (MenuLinkDefault class).
Comment #2
webflo CreditAttribution: webflo commentedComment #4
webflo CreditAttribution: webflo commentedThe enabled key was missing in config schema.
Comment #5
webflo CreditAttribution: webflo commentedExposed the config setting to views ui.
Comment #8
pwolanin CreditAttribution: pwolanin commentedThe 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?
Comment #9
dawehnerWell 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.
Is there a reason we can't use the ViewTestBase and the
static $testViews
mechanism?Comment #10
s_leu CreditAttribution: s_leu at MD Systems GmbH commentedHere'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.
Comment #11
s_leu CreditAttribution: s_leu at MD Systems GmbH commentedRemoved 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.
Comment #12
dawehnerIMHO we should also add a default for expanded
should be boolean false I think
{@inheritdoc} missing
Let's use // instead.
Comment #13
BerdirComment #14
s_leu CreditAttribution: s_leu at MD Systems GmbH commentedMade 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.
Comment #15
dawehnerThis is odd, why do you think it should be disabled by default?
Comment #17
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedBecause 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.
Comment #18
bircherA 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.
Comment #19
drubbFrom 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.
Comment #20
dawehnerI 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,
Comment #22
dawehnerThe fact that we don't save something aka. something gets destroyed on cache rebuild is sort of a critical for me, to be honest.
Comment #23
catch@dawehner are you thinking of removing this from the UI in this issue or a follow-up?
Comment #24
dawehner@catch
I forgot to attach an interdiff, but this most recent patch already doesn't add a UI for it.
Comment #25
catchOh 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.
Comment #26
alexpottI think we need an upgrade path here because the config will change on a cache rebuild.
Comment #27
catchI'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.
Comment #28
BerdirI'd agree, it seems pretty unlikely that we'd manage to salvage something before it gets lost.
Comment #29
alexpottMy 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.
Comment #30
chaitanya17 CreditAttribution: chaitanya17 at Cybage Software Pvt Ltd. commentedI 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);
}
Comment #31
catchThis 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:
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.
Comment #32
dawehnerNothing in views at least requires new items to be part of files, but I see your point.
Comment #33
chaitanya17 CreditAttribution: chaitanya17 at Cybage Software Pvt Ltd. commentedfor #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.
Comment #34
dawehner@alexpott
Do you want to see something like this, otherwise please clarify what you think is needed here.
Comment #37
alexpott@dawehner yes something like that.
Comment #38
dawehnerThank you alex!
Comment #39
LendudeManually tested this and bug still occurs in current HEAD, added steps to reproduce in issue summary.
Feedback from @alexpott in #26 was addressed
Missing r in overrides, fix on commit?
Other then that the patch looks good to go to me.
Comment #40
alexpottThis change looks out-of-scope here. I think it needs to be looked at separately.
Comment #41
Lendude@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.
Comment #42
alexpottWell metadata is definitely part of the MenuLinkContent plugin - see \Drupal\menu_link_content\Plugin\Menu\MenuLinkContent
So I think allowing it to be configured in views at least leaves options open.
Comment #43
catch@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.
Comment #44
effulgentsia CreditAttribution: effulgentsia at Acquia commentedDiscussed this with @alexpott, @catch, @xjm, and @webchick, and we agree it's Critical, since it's data loss.
Comment #45
dawehnerWe should remove the schema as well...
Comment #46
alexpottre #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.
Comment #47
dawehnerI'm personally not sure about it. If we support metadata the view object should also store it, which the patch currently doesn't.
Comment #48
Lendudetalked 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
was already getting set a couple of rows further on, so no need to add this.
set this into the 8.1 updates
Comment #49
dawehnerGood catch!
Comment #52
alexpottCommitted 495a46a and pushed to 8.1.x and 8.2.x. Thanks!
Sorry for holding this issue up.
Comment #53
jibranAre we going to add the upgrade path test in followup?