Added tests.
is there a simple way to add to our test coverage?
Missed this. Moving back to NW.
tl;dr: not sure yet
Back to NR.
I pushed two commits to the issue branch. Thanks for the head start @scotwhith1t! I ended up starting from a blank slate based on the 3.0.x branch, but I used your patch as inspiration.
The two commits:
- Add an input to configured a theme hook suggestion
- Set the menu depth to be relative to the active menu item
I propose any remaining functionality from the menu_block module be broken out into their own feature request issues to keep the scope of changes to a minimum. I probably should have broken the two commits above apart.
gabesullice → changed the visibility of the branch 3366930-theme-suggestion-and-dynamic-level to hidden.
gabesullice → made their first commit to this issue’s fork.
gabesullice → made their first commit to this issue’s fork.
Converted your patch to a merge request ^
I didn't make any changes. Let's let a maintainer review this.
gabesullice → created an issue.
gabesullice → created an issue.
Took a swing at this and updated the IS
gabesullice → created an issue.
gabesullice → created an issue.
gabesullice → made their first commit to this issue’s fork.
For those looking for a simple workaround to fix their kernel tests, the following additions to my kernel test class suppressed the exception.
I imported Drupal\Tests\user\Traits\UserCreationTrait and added that trait to my kernel test class with use UserCreationTest. Then I added $this->setUpCurrentUser() in my class's setUp method. In my case, I didn't need to call that method any optional arguments.
effulgentsia → credited gabesullice → .
Thanks @zrpnr!
tim.plunkett → credited gabesullice → .
A gaunt hand breaks through the damp earth of the cemetery grounds, once dead, the zombie (issue) rises to torment us again.
heh, jokes aside. I stumbled in here because I came across a similar need. @tim.plunkett, you're right that you can make this work with the form system as-is, but one needs to customize the validateForm method and call setValueForElement() method. So yes, it's already possible, but only if you have a deep enough understanding of the form API to understand what's going on in FieldStorageAddForm and are willing to set data during the validation step.
I think it's a reasonable feature request to make this part of the machine name form element API via a #value_prefix property or similar. Perhaps as a #process callback. I guess that's why this issue hasn't ever been closed.
Maybe this bump will breath new life into the idea ;)
Thank you so so so much for all the heart and soul you've poured into our little internet outpost. We're all better for it and we'll honestly never be able to repay it entirely.
Thank you from the bottom of my heart.
We'll always love ya and we'll never forget ya!
#12++
I'm trying to do that in
MenuLinkTree, but I'm running into a hiccup because, at some point in the process of building a menu link tree, a particular item may not have any visible children even though one will be added later. Thus, my current code is removing items that it shouldn't because it doesn't "know" about the child that doesn't exist yet. I think I just have to spend more time with Xdebug to figure out where these missing child links are added.
To follow up on this: the issue was in the toolbar module. It does a quirky thing where it only loads the top level of the admin menu, then it loads the subtree of each of those top level items separately, rather than loading the whole tree at once. This explains why I wasn't seeing any children when I was step-debugging the first load of the admin menu (it was only loading a shallow depth).
Needs work to update the toolbar tests, which makes sense.
To clarify: it makes sense because I'm fiddling with the menu and the toolbar tests probably make some assumptions that don't hold any longer.
Needs work to update the toolbar tests, which makes sense.
Okay, I added code to update the database schema on existing sites. I was expecting to write an update hook to do this, but it seems like the default menu tree storage service handles its own schema changes, so I made my changes in there instead. This probably needs special attention.
Needs review for tests.
Needs work for an update hook to add the transitive column to existing sites.
I spent some time trying to get this working by adding an additional access check to all of the routes which use the SystemController::systemAdminMenuBlockPage controller, but I don't think that's going to be a fruitful approach. It's difficult to check access to menu links from that controller and after digging in, it felt a little hacky.
Instead, I think it might be more useful and generally useful to add a new flag to menu items to indicate whether it should or shouldn't be shown if doesn't have children. Tentatively, I'm naming this flag transitive with a default value of FALSE. The idea is that menu links like People and Development can be marked "transitive" since their only purpose is to transit users from the root page to a child page. Thus, if they have no visible child links, they can be safely elided.
I'm trying to do that in MenuLinkTree but I'm running into a hiccup because at some point in the process of building a menu link tree, a particular item may not have any children but will have one added later. So, my current code is removing items that it shouldn't because it doesn't have a way to "know" about the child that doesn't exist yet. I think I just have to spend more time with Xdebug.
The drawback of the menu link tree based approach is that the system menu block pages would still be accessible, even without any links, if a user visited the right URL directly. That feels like a separate issue that could be handled in a follow-up though. All we'd need to do would be to throw a 403 or 404 HTTP exception in the controller instead of printing out the message.
Failing test.
I was able to reproduce this on the tip of the 9.5.x branch and I updated the issue description with steps to reproduce this bug from a clean installation.
+1 to UpperCamel casing for enum names.
The current approach is not workable because it is a significant feature regression.
@xjm, I think there's a legitimate bug here, but the issue went a little off track by mixing in performance and scalability concerns.
What's the bug?
In short, if I'm on the /admin/structure/menu/manage/main/add route (note the main route param), I shouldn't be able to create a new menu link in the footer menu, but I can.
To expand on that: imagine if you're editing the and you follow the button. Now also imagine that you probably have a menu link with the same name in both the and menu. On the create form, you accidentally choose the link in the menu because you (understandably) assumed that you're adding a link to the . Worse, after you click save, you'll be redirected back to the overview and you'll be presented with a green status message saying "The menu link has been saved," however you will not see your newly created menu link since it was actually added to the menu.b
gabesullice → made their first commit to this issue’s fork.
Thank you @Wim Leers! I'm going to go ahead and make you a maintainer since I know you have some critical deps on this module. This change looks good to me!