Problem/Motivation
Menu links map very well to the entity system, from CRUD operations to access control and rendering. As a huge bonus, we get fieldability support (not in this initial patch though) and the ability to deprecate *a lot* of contrib modules from the menu_* namespace.
Proposed resolution
Convert menu links to the entity system. In order to keep the initial conversion patch at a reasonable size, make the entity class implement ArrayAccess so we can keep most of the current code intact.
Remaining tasks
Review the latest patch.
User interface changes
None.
API changes
hook_menu_link_alter()
is replaced byhook_menu_link_presave()
hook_translated_menu_link_alter()
is replaced byhook_menu_link_load()
Probably others that don't come to mind right now.
Original report by Dave Reid
Modules need to be able to load menu links without worry about access (XML sitemap, token/pathauto). My initial workaround for this is in #333590-13: Node menu token fails (node grants not saved yet, menu access checking, oh my!) but I think menu links map very well to the entity system seeing as they could take advantage of entity caching and loaders (menu item = entity, menu that the item belongs to = bundle). We can do other stuff like abstracting the access checking of a menu link from its loading, just like we do with other entities.
Followup issues:
#1842850: Untangle menu_link access control from _menu_link_translate() and friends
#1842852: Move tree related code from menu.inc to the new Menu link module
#1842854: Move system menu block to core
#1842858: [PP-1] Convert menu links to the new Entity Field API
#1835902: Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)
#1805054: Cache localized, access filtered, URL resolved, and rendered menu trees
Related issues:
#1814916: Convert menus into entities
#1497380: Convert shortcut sets to ConfigEntity
Comment | File | Size | Author |
---|---|---|---|
#179 | menu-rebuild-performance-916388-179.patch | 2.25 KB | Berdir |
#165 | 916388-menu_link-165.patch | 189.62 KB | amateescu |
#165 | interdiff.txt | 1.99 KB | amateescu |
#163 | 916388-menu_link-163.patch | 189.11 KB | amateescu |
#163 | interdiff.txt | 16.25 KB | amateescu |
Comments
Comment #1
Dave ReidHere's some initial entity code I played with for D7:
Comment #2
Dave ReidTagging
Comment #3
Dave ReidPlan:
1. Make a 'menu_link' and 'menu' entity types, the former provided by system module as it should always be available, the latter provided by menu.module.
2. Remove access control from load operations and convert it to menu_link_access()
3. Provide the full cruds associated with entities, including adding a menu_link_load_multiple() and menu_link_delete_multiple().
4. Provide tokens for menu links and menus in the respective system and menu modules.
Comment #4
Dave ReidComment #5
mattyoung CreditAttribution: mattyoung commentedWould this make it possible to have "non-secret" restrict access menu? Eg: I want to show the menu link to all users, but require them to have acces right to actually gain access.
We should have two permission: one the current one. Another to control visibility of the menu.
Comment #6
Dave ReidThat would fall under #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter().
Comment #7
joachim CreditAttribution: joachim commentedSounds good, and would simplify things like adding icons to menus.
> (menu item = entity, menu that the item belongs to = bundle)
How would moving a menu item from one menu to another work with that?
Comment #8
dropcube CreditAttribution: dropcube commentedSubscribing.
Would like to see how this could work with http://drupal.org/project/entity_translation to make menu links entities translatable (i18n_menu module of http://drupal.org/project/i18n package).
Comment #9
casey CreditAttribution: casey commented+1
Comment #10
gaspaio CreditAttribution: gaspaio commented+1
Comment #11
catchSubscribing, this keeps coming up in conversation, I think it makes sense but it will also be tricky.
Comment #12
jherencia CreditAttribution: jherencia commentedSubscribing...
Comment #13
Gábor HojtsyTagging for Drupal 8 multilingual initiative. Re: http://groups.drupal.org/node/155634
Comment #14
plachComment #15
mrsinguyen CreditAttribution: mrsinguyen commentedSubscribing
Comment #16
zkday CreditAttribution: zkday commentedSubscribing
Comment #17
dtarc CreditAttribution: dtarc commentedsubscribing
Comment #18
sunWhile I agree the idea makes sense, this will (vastly) decrease performance. Tough challenge.
Comment #19
catchYeah menu links are already a serious performance issue - we already have a per-page cache per menu, and separate lookup keys to avoid duplicate cache entries. Cache misses are three queries plus two cache_set() per menu as well.
However if we were able to simplify that somehow to at least cache some information site-wide instead of per page (haven't figured out how yet), it might outweigh the change here.
Comment #20
omercioglu CreditAttribution: omercioglu commentedSubscribing
Comment #21
marcvangendOn a side note: If menu links become entities, would that mean that we can make them fieldable and create a more flexible alternative for modules like Menu attributes?
Comment #22
Gábor HojtsyAny takers?
Comment #23
XanoIn response to #3: we have menu router items (mapping URLs to callbacks, access control, etc) and menu links (the actual clickable links. They overlap, but not fully. Which one do you suggest we convert to an entity type?
Comment #24
sunMenu router items are not entities.
Aside from that, I'd suggest splitting either menu links or menus into an own issue. The latter can be implemented much more easily than the former.
Comment #25
casey CreditAttribution: casey commentedI have some code in my sandbox splitting menu.inc into three parts moving anything related to menus into the menu module: http://drupal.org/sandbox/casey/1322008
Far from working though.
Comment #26
Gábor Hojtsy@casey: Great! What about starting with converting menus themselves to entities? That sounds like would be the easier one, instead of trying to tackle both parallel.
Comment #27
Dave ReidIt would probably be best to start with converting menu links. I'm not so sure we should convert menus themselves into entities as they're actually bundles with machine names.
Comment #28
Gábor HojtsyFeedback from my blog post on menu translation (from Quentin DELANCE):
Comment #29
Gábor HojtsyTagging for the content handling leg of D8MI.
Comment #30
gddI just want to throw my hat in the ring of people that agree that menu links should probably become entities, but that we should leave everything else as is. We should probably get crell in on this issue, since he is planning all sorts of changes to the menu system as a part of the WSCCI initiative.
Comment #31
Crell CreditAttribution: Crell commentedTagging. I'll be posting a writeup of our plans for the routing layer coming out of DrupalCon very shortly, which I'll then link here with more commentary. Short version: Router items are config, not entities, and WSCCI is moving them out of hook_menu. Links are probably entities, although I don't know if they're fieldable. The other things hook_menu does just suck. :-)
Comment #32
andypostMenu links could be fieldable to make items into rich-snippets of content like menu_icons module does.
Comment #33
Gábor HojtsyIf menu items are neither converted to entities neither to config, they will not be translatable in Drupal 8.
Comment #34
rudiedirkx CreditAttribution: rudiedirkx commentedI agree. More important (or at least more useful) to have fieldable and translatable menu items. Obviously they have to be translatable in D8. More easily than in D7 even.
I wouldn't know where to start though... Clone D8 and rewrite menu.module?
Comment #35
Gábor Hojtsy@rudiedirkx: theoretically just making the menu item data entities could be possible in smaller steps, with converting description, etc. to fields as next steps.
Comment #36
pounardField API is today quite slow, even in read operations, aren't you afraid that menu could suffer from this? I really don't know just asking here, a lot of data of the menu API is actually cached, but maybe it could become very heavy when, for example, building menu blocks?
Comment #37
skwashd CreditAttribution: skwashd commentedOne of my colleagues has worked on this as a contrib module for D7. His sandbox is at https://drupal.org/sandbox/jamesharv/1749006 Unfortunately he forgot to push his code to it. I have a github repo for the code at https://github.com/skwashd/entity_menu_links so it can be reviewed.
Comment #38
pwolanin CreditAttribution: pwolanin commentedFrom discussion at Drupalcon:
Simply convert the existing menu links table into the base table for an entity - keep the hierarchy implementation contained within the entity - need to figure out how to handle the book code.
Step 2 would be to split out the hierarchy handling into a separate subsystem - through that interface you should be able to load a set of entity IDs for a particular hierarchy. Note that their are some gotcha's here around the "expanded" functionality in the current menu links.
donquixote and ronald_istos said they would work on this, but they need to to connect up.
Comment #39
donquixote CreditAttribution: donquixote commentedThat's the difficult part :(
I would love to hear his input and status.
So, here is some more thoughts, after talking with Crell, Dave Reid, and Peter Wolanin:
I think the strongest argument is in #33, making them translatable (although I was not aware that w/o entity or conf we can't translate anything). This requires the title to be a field or a translatable property (as I understand, it is still quite vague what entity "property" will mean vs "field")
Why?
So, the plan would be, to have one default bundle that can be used for all items until someone wants a different item. We could even leave it to contrib to define other bundle types.
Unclear: Where to use field API
Talking with Crell, it seems there are three types of data on an entity in D8, but this is not carved in stone yet:
This is what I was able to pull from conversations. Apparently this stuff is still being worked on.
For the title we'd probably use (1) or (2).
The link usually does not need to be translated, but it could still be interesting to have it use field API.
The parent item reference should probably not be translated, and performce is a big deal here.
So far the general idea is to start with everything hardcoded, and decide later if we move any of that to field API.
Any information on this is welcome.
Remotely related stuff:
Comment #40
nevergone CreditAttribution: nevergone commentedIn my opinion, the menu and menu items is entities. That possible both connent data, and menu items properties the menu name. The optional data is field, becasuse good extendable and scalable.
Comment #41
Crell CreditAttribution: Crell commentedOne correction to #39: There is no "hard coded non-field data" on Entities in the current entity roadmap. None. Any and all such data is unsupported and should not be used, ever.
I believe as fago is currently using the terms:
Property: A compound data element on an Entity (basically what we call "Fields" in Drupal 7).
Field: A special case of a Property that is user-added rather than defined by the module that declares the Entity type.
Regarding incremental rebuild, there's a related issue: #1446808: Let plugin managers announce changed definitions
Comment #42
ronald_istos CreditAttribution: ronald_istos commentedHere is a summary + possible actions moving forward to provide some focus.
1. Everyone agrees menu links should be entities. Jury is out on Menus => Let us do menu links to start with and that will inform subsequent thought. We can, nevertheless, say that the original idea of menus as bundles of menu links is off the table.
2. There is some code to look at - probably not directly usable but a start. #25 and #37 talk about that.
3. #19 mentions performance concerns - that we really need to figure out.
Next step for me will be to look at existing code and outline a strategy with some sort of patch about how to simply turn menu links into entities. To do that I will need to:
1. Figure out how to built Entities in Drupal 8 - i.e. what has changed from 7 (Crell's comment on Properties and Fields above is *very* confusing).
2. Do a very straightforward conversion of menu links into Entities and progressively enhance that.
Comment #43
gddNote that this has impact on CMI, because if menu links don't become entities then they need to come into CMI, otherwise they won't be deployable.
Comment #44
Crell CreditAttribution: Crell commentedRonald: Go for it. The terminology that fago is using is a bit different than what we discussed back in Paris, and I don't know why he switched. :-) I agree, it's confusing. Check with him if you're still confused. He has a large-ish patch in a sandbox branch that is trying to implement the new model we designed. I'd jump straight to that API if you can.
Comment #45
donquixote CreditAttribution: donquixote commentedThe consequence of #41 would be that all storage-related stuff has to be defined as a "field type" (or "property type", *) first, and only then wired up with the entity type (but be equally available for any other entity type).
The question would be, if the "field type" only defines the basic storage, or also all logic associated with it. E.g. there would be a field type "tree parent reference, (un)ordered", with all the tree loading logic, additional storage for materialized path etc, which would then be available for any entity type.
When I suggested that on DrupalCon Munich, the response was that we rather want to hardcode this logic into the entity type as a first step (I think it was Peter who said that).
(*) If we distinguish "field type" and "property type", then the difference would be that for a "property type" we can skip the field instance configuration UI.
Comment #46
Crell CreditAttribution: Crell commentedThis is important for both WSCCI and i18n initiatives, and is a blocker for eliminating hook_menu(). Bumping to critical per discussion with Shannon.
amateescu is working on this, but hasn't posted anything yet. Andrei, can you at least post an interim patch so we know things are happening? :-)
Comment #47
tim.plunkettTagging.
Comment #48
andypostYeah it really interesting how this goes - I'd recommend to make this issue as META and made a follow-ups
- Convert menu to Config entity like imageStyle now
- Convert menu item to Entity like File and User now
Working on #1552396: Convert vocabularies into configuration & #1588422: Convert contact categories to configuration system I suggest to make separate issues for
- Conversion to entity, tests
- Moving forms to controllers, tests
- Implementing Entity List Controllers, tests
This would help to make patches more reviewable and probably could speed-up process
Comment #49
andypostAlso module shortcut needs a some changes to work with new menu-links #1304486-17: Completely remove the ability to limit the number of shortcuts per set
EDIT Related #1801304: Add Entity reference field
Comment #50
amateescu CreditAttribution: amateescu commentedFirst of all, sorry for stashing this work in the wscci sandbox for so long :/
I tried to keep this patch as small as possible but it's still a huge change to review because menu links are *everywhere*.
This is the list of things that I left out from this patch and would like to keep for follow-ups:
And the list of things still to do in the current patch:
Now, on to the patch(es):
Let's keep this issue focused only on menu links and open a new one for converting menus to config.
Fingers crossed for finding reviewers :D
Comment #52
chx CreditAttribution: chx commentedFirst: amazing work. Really, I am astonished how far you got. Mostly small problems:
Can we add a comment on why don't we use url() here? (because it'd add base_path)
Hrm, why is this commented? Can we remove it or is something missing?
{"p$depth"} is very hard to read IMO, can we
$property = "p$depth"; $entity->$property
instead?Even if this happens to work with the current EFQ (I am btw shocked if it does) you never should re-execute() the same query twice, clone before count()->execute() thanks.
*blink* multiple lines? Loops? The whole tree buildup here is very hard to read.
This needs a use Drupal\User\User. We only do the \foo syntax for non-namespaced classes.
I know this comes from the old code but can we slap a TODO here to make this a render array? I dislike inline theme() calls :/ I know render arrays are on their way out but still, structured output and all that. If this is not feasible with a block content then nevermind.
label_a and label_b perhaps?
Is this still a @todo? I saw localized options below albeit it was marked a "hack".
Can we put
p$i
this in a variable? Please? My eyes are bleeding.This can be put into a module_invoke('menu_link', 'maintain'.... it will do nothing if menu_link does not exist.
Small semantics and it doesn't change the test but it's not menu_link it's menu_links because you are loading multiple ones. Important only if someone uses this as an example.
I would love to see a TODO / followup to study whether it'd be possible to move these p1,p2... columns into a single array property.
So many public properties. I understand that' because of the straight port, of course. Followup perhaps to make them protected, move more logic in here from menu.inc etc?
You want
get_object_vars()
here. I am surprised array casting even works. It's absolutely not guaranteed it will work on future PHP versions.Comment #53
andypostShortcut part probably would have conflicts with #1497380-7: Convert shortcut sets to ConfigEntity
Comment #54
chx CreditAttribution: chx commentedOne more thing which I partially touched already: I see #50 mentioning that later we will let the D8 APIs deal with localized options which is fine but please do not leave in commented queries. We have git to get old code out from if necessary.
Comment #55
ardas CreditAttribution: ardas commentedThis is a great and correct idea - we need a powerful mechanism to extend menu item with different fields. No need to make menu items NOT fieldable... I don't understand why they made fieldable = FALSE !!!
Drupal core should handle all data extending, so that the same storage mechanism is utilised ... and there will be no need to develop different modules to extend menu items.
Fields will also solve a problem of menu item icons... just add an image field and override menu item theme function - it is DONE!
+ 1000000 for fieldable=TRUE
Comment #56
podaroklooks like we need a lot of speed testing here
#55 agree with that, but if we make menu links as fildable possib;y it will be a huge performance impact
Comment #57
pounardNot necessarily as long as entity can be cached. Right now they are not until #1596472: Replace hard coded static cache of entities with cache backends happens, but we have to consider this a solution if menu item fields cause problems.
Comment #58
podarok#57 cached entities can be performance impact toocause if we have 20+ K menu links
(bundles as in this patch)unserialize and serialize from/to cache may be a trouble too like in #1040790: _field_info_collate_fields() memory usageComment #59
catchSee #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees for caching. Short version, we need to move from caching menu tree arrays, to caching rendered links. If we can do that then the bulk of requests will have (much) better performance than they do now.
Comment #60
podarok#59 thanks... following
Comment #61
andypostFiled #1814916: Convert menus into entities
updated summary with related links
Comment #62
amateescu CreditAttribution: amateescu commentedAdressed all the comments from #52 and fixed a few more tests.
About the remaining failures:
- book.module is out of this world, I don't know what we're going to do with it
- I have a feeling that TrailTest is going to give me a very hard time, probably I will end up converting access, href and all the new public properties to protected. Actually, this is what I did initially but converted them to public in order to keep the patch size down :(
Comment #64
markhalliwellJust saw this issue, but also referencing #1287610: Create a Menu Item Type API.
I really like the idea of converting menu links into entities. Although, I also feel there should be a terminology shift from "menu links" into "menu items". Rendered menu items could technically be anything: link (menu, which would use the menu router), plain text or separator (special_menu_items), or a view (menu_views).
Comment #65
fagoI'm wondering whether it makes sense to have that in resetCache() ? That way we'd have a single place that clears all caches related to the entities.
Comment #66
amateescu CreditAttribution: amateescu commentedI'm not sure, but caching of menu links will need to be revisited anyway so I'd prefer to leave that investigation for a follow-up.
It's time for an update here :) I was right about TrailTest, it did gave a very hard time (a few days worth). Investigating that led me from one rabbit hole to another because every usage of menu links is so tied up (leaked-in router item properties, breadcrumbs, active links, etc..) that I was actually starting to turn the current (old) router system to OO.
So I decided to take another approach for now, use ArrayAccess and deal with cleanups properly in followup issues. This reduced the patch size by 100K and hopefully made it a lot easier to review.
Comment #67
amateescu CreditAttribution: amateescu commentedComment #69
amateescu CreditAttribution: amateescu commentedLet's try to move the registration of entity.query to bootstrap.inc, as suggested by @tim.plunkett.
Comment #71
nevergone CreditAttribution: nevergone commented#69: menu-links-entities-69.patch queued for re-testing.
Comment #73
Gábor HojtsyThis issue is still a huge enabler for the multilingual initiative to make menu items translatable. Keep up the great work! :)
Comment #74
amateescu CreditAttribution: amateescu commentedFixed the remaining test failures, I just needed to add the entity query factories to the container used by update.php :)
Comment #75
BerdirYay, green!
Some random feedback....
We discussed this before :) Just thinking out loud...
- using modules in an includes file is really not what we want
- but the plan is that menu.inc will be completely replaced by menu_link and the new router system and menu links will eventually become an optional module then, right?
- Not sure if that will happen for 8.x, though.
- Now that entity types are plugins, it's actually possible to define entity types as a component, I think.
- It might be a possibility to keep the functional stuff in a menu_link.inc for the moment and move it to a module once we're that far.
Wondering if we can shorten these classes a bit. For example, the taxonomy_term classes are just called Term*, as they're within the taxonomy namespace.
Link* on the other hand is rather misleading, so MenuLink probably does make sense.
Is a menu link really content? I don't think any more than a user and a user is currently not :)
What would happen if you were to disable menu_link?
Sounds to me like people could really mess up their site by identically disabling this. Just like we currently have a hardcoded fallback if you disable block.module to make the main block still available.
The plan is to remove most of the per-entity-type specific wrapper functions and directly use entity_*(). Maybe we can leave them out from the beginning here?
This one for example is certainly useless :)
Right now in 7.x, this is added by i18n_menu (I think?). Because core then doesn't know about it, the preferred link/breadcrumb generation doesn't the language into account, so i18n_menu() needs to implement hook_init() and inject the static cache, se http://api.drupalize.me/api/drupal/function/i18n_menu_init/7. I *think* there is a hook now, that allows easier access to this, but you still have to duplicate the whole query.
Did you already take care of that in this issue, should we do it here or in a follow-up issue (would be totally fine with me but should probably be major as we should really really do that).
Is this actually necessary?
Right now, I think the serialize flag does not actually have any effect on the database side, it's only used by drupal_write_record() to automatically serialize().
Comment #76
moshe weitzman CreditAttribution: moshe weitzman commentedJust want to reemphasize that we have a 'needs benchmarks' tag on this issue and we should remember to follow through on it. If this patch is only a small regression, maybe we say it is OK if we also get a patch to disable menu link building entirely (donquixote has worked on this).
Comment #76.0
moshe weitzman CreditAttribution: moshe weitzman commentedUpdated issue summary.
Comment #77
andypostConverting menus itself in #1814916: Convert menus into entities
I came to similar dilemma - the menu.module should manage custom menus and probably their items.
Menu items also used all over a core (mostly their forms to add/edit as sub-forms).
Shortcut module creates a menu-items with own "menu_name-bundle" so just manages same custom menus just prefixing it's machine name and provides a block.
So it seems that Menu and MenuItem should be defined in system.module as Core entities and menu module should be just a UI module - to manage what?
Comment #78
Crell CreditAttribution: Crell commentedPlease don't define anything in system module if you can possibly avoid it. :-/ That thing is an abomination of circular dependencies that needs to die. That it wasn't executed this dev cycle is a major regret.
Comment #79
tim.plunkettAnnotated classes (plugins) need not be provided by a module. They can just live in \Drupal\Core\Plugin\Core\Entity
Comment #80
amateescu CreditAttribution: amateescu commentedRe #76: Wouldn't have done it otherwise :) Here's some performance testing done on my local machine.
Anonymous user
Logged in as uid 1
All tests were done with the standard profile and a warm cache.
Comment #81
BerdirLooked into this a bit with xhprof.
First: I installed this and HEAD in parallel and the installation with this patch seems to have taken a considerable longer amount. Comparing the test run time of this and patches on the same bot confirmed this. This patch took 1h10m, compared to ~47min of other patches. That's a *huge* difference.
On pages with a warm cache, the differences vary but there aren't many large ones. Some things that I noticed:
- 14 additional calls to the classloader, which is actually the biggest difference. My guess is that this is actually because we're using EFQ on the frontpage? Some sort of caching might help here.
- Looks like a considerable amount of the additional time and especially those function calls are spent in the offset*() methods for the BC layer. That should just go away, so this should be come irrelevant
- ~3 more queries which are executed currently.
Tried to get some comparable results on a first call to a cold cache (truncated cache, cache_bootstrap, cache_config and cache_menu and deleted the dumped container). The total runtimes were relatively close (580 to 600ms), so I might be missing something, not sure what needs to be emptied as well, maybe a menu rebuild? Some things that I noticed:
- ~7 more queries, 9 more Select::__toString() (subquery?)
- Some more Tokenizer parser calls due to the new entity type
- A bunch of smaller things, mostly related to the two above (more queries and more annotation parsing)
Ok, so let's check menu_router_rebuild(). And now it gets interesting. I'm seeing a *huge* difference there. The 800ms (HEAD) vs 7s kind of difference. So something is obviously going wrong here.
- 900 vs 5000 queries.
- 50% of the page request is spent within Drupal\Core\Entity\DatabaseStorageController::load(). See attached screenshot.
- Those loads are coming from loadByProperties(), updateParentalStatus, entity_load_unchanged() and findParent() and a few entity_load_multiple().
- Additionally, there are 580 EFQ queries
- Also interesting: 300 calls to menu_cache_clear(), each of them resulting in a invalidateTags()/Merge Query
You can reproduce this easily by adding a call to menu_router_rebuild(). I'm not sure what exactly will become of this once the existing menu system is replaced completely, I assume things might get better as router rebuild won't mean menu item rebuild, but I have no idea what exactly the plan is with menu items. But we need to improve this quite a bit I think, to improve testrun time, if nothing else.
Comment #82
BerdirForgot the screenshot.
Also, corrected something above. It's a token parser, not a reflection parser, it's just the class that's called StaticReflectionParser.
Comment #83
donquixote CreditAttribution: donquixote commentedAbout menu_router_rebuild() / _menu_navigation_links_rebuild():
I am working on a solution to speed that up.
#1830274: Support a parent_path setting in hook_menu(). / hook_menu_link_alter() should not touch the plid.
This is all based on the traditional menu links system, and I would like to get that in first, to make it backportable to D7.
However, the idea should work as well for a new system.
Idea of the algorithm:
- No expensive calls to menu_link_save() and friends, and use dedicated methods instead.
- One controlled flow, where everything is touched only once, starting at root level, and moving down from there.
This requires the API change mentioned in the linked issue title, but I think that would be quite acceptable for D8 at least.
(and besides mentioning it here, I am ok if you guys leave me tinkering on that one until I have something presentable)
Comment #84
donquixote CreditAttribution: donquixote commented#76 (Moshe)
That would be this one, I guess
#1170278: Introduce hook_menu_router_update()
Comment #85
amateescu CreditAttribution: amateescu commentedHere's a patch that restores some sanity to _menu_navigation_links_rebuild(). It's still ~3x slower than HEAD but ~7x better than #74 :)
Note to self: I still have to respond to the review from #75.
Comment #86
David_Rothstein CreditAttribution: David_Rothstein commentedThis is an important issue, but I don't believe it's a critical task (see http://drupal.org/node/45111 and http://drupal.org/node/1181250). Specifically, there are ways to translate menus without this, and removing hook_menu() in favor of other methods is not something that must be done to release Drupal 8...
If you disagree and feel this must still be marked critical, please provide a very specific justification and keep in mind that every critical issue significantly prevents other features that people are working hard on from getting into Drupal core, due to the issue queue threshold policy (until/unless #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw is addressed, at any rate). Note that I am not singling out this issue for particular attention but rather (slowly) trying to go through the issue queue and do this in many places so that other features still have a chance to get into Drupal core soon without the thresholds getting in the way. I am trying my best to be impartial, and of course, just because an issue isn't marked "critical" certainly doesn't mean it's unimportant to work on. You can follow my progress via the tag I've just added to this issue :)
Comment #88
amateescu CreditAttribution: amateescu commentedThis should fix the exceptions.
Comment #89
catchI'm fairly sure that hook_menu() won't even work once the router-ish bits have been moved to the new router system - not without significant refactoring, of which this patch is the first step towards properly separating them out. We might want a critical meta issueto track the various pieces of this and have this as a sub-issue though.
Comment #90
Crell CreditAttribution: Crell commentedRelated, for the other bits of hook_menu that we haven't found a new home for yet: #1835902: Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)
Comment #91
amateescu CreditAttribution: amateescu commentedAnd finally a response to #75.
About creating a new module vs menu_link.inc, this was discussed on IRC and I remember the conclusion was that a new module (required for now but optional in the end) is cleaner because we also need to provide the form and storage controllers.
Anyway, as I mentioned in #50 and catch said in #89, this is just the first step, there are many things left to do to properly separate menu links from old router items.
The end goal is to make them fieldable, so yes, content entity for now. If we decide otherwise until code freeze, it's a trivial change :)
Can you disable a required module? :-O I guess it would certainly break your site, though not more than disabling system.module...
Or if you meant disabling it *after* it becomes optional, well, that's a different story. You can do that either because you have a custom module that provides navigation-like functionality, or you just don't need it because you are using Drupal only for REST purposes.
I agree with that and I have no love for those two wrappers, but I do need
menu_link_delete()
because it has some functionality that can't be moved to the delete() method because of the extra parameters.I didn't do anything related to that and it is in the list of followups from #50. Implement the new field api and cleanup localized_title and localized_options.
That's the only reason I added the serialize property, we are using drupal_write_record() in the entity save controller.
Now, on to the performance review.
_menu_navigation_links_rebuild()
has been improved a lot in #85 but it's only temporary anyway because we want to get rid of it soon.From ~4K new function calls on a warm cache, these make up for almost 70-75% of them and, unfortunately, won't go away, they will transform into calls to __get(), __set() and friends, the magic methods from the new entity field api. The plan is to switch to caching rendered entities and this is how we can get rid of them :)
Comment #92
catchI haven't looked all the way through this in depth yet, but I've been following the issue and have looked through the patch a couple of times.
Yes to a separate required module for now. Required modules aren't great, but they're better than stuffing things into system module and looks like there's a chance this will eventually be optional.
Also fine with this being the 'straight conversion' patch and handling the deeper issues of what to do about hook_menu() and link rebuilding etc. in follow-ups.
I asked amateescu to create follow-up issues and link to them from the issue summary, as well as to the existing follow-ups that already exist. Won't mark this RTBC yet because I've not fully reviewed it (and the module_invoke() in aggregator looked a bit odd, why not module_exists() for that case?) but seems like it's close.
Comment #93
donquixote CreditAttribution: donquixote commentedJust a quick note: It seems that hook_menu_link_alter() is dead with this patch? Replaced with entity hooks?
(I don't mind it being removed)
Comment #94
amateescu CreditAttribution: amateescu commentedYep,
hook_menu_link_alter()
is replaced byhook_menu_link_presave()
andhook_translated_menu_link_alter()
is replaced byhook_menu_link_load()
. This will be written in the change notice.Comment #94.0
amateescu CreditAttribution: amateescu commentedAdded link to shortcuts conversion
Comment #95
amateescu CreditAttribution: amateescu commentedThe issue summary has been rewritten :)
Comment #96
Crell CreditAttribution: Crell commented#88: menu-links-entities-88.patch queued for re-testing.
Comment #98
Crell CreditAttribution: Crell commentedI thought as much. :-( amateescu, can you reroll, and specify what sort of review/backup/support you need here? Both WSCCI and D8MI are looking forward to this. :-)
Comment #99
amateescu CreditAttribution: amateescu commentedLet's see if I managed to fix all the conflicts :/
The only thing needed is just a regular patch review, as I removed all the large architectural changes (access and translation handling) from this patch since #66.
Comment #101
andyceo CreditAttribution: andyceo commented#99: menu-links-entities-99.patch queued for re-testing.
Comment #103
disasm CreditAttribution: disasm commentedattached is a reroll.
Comment #104
disasm CreditAttribution: disasm commentedComment #106
disasm CreditAttribution: disasm commented#103: drupal-menu_links_entities-916388-103.patch queued for re-testing.
Comment #108
disasm CreditAttribution: disasm commentedThe current testbot failure is due to the run-tests.sh script behaving differently than running tests from the UI. I have successfully ran the first 30 tests in the UI on my local, but get this as the first failure in running the script:
Comment #109
disasm CreditAttribution: disasm commentedComment #110
amateescu CreditAttribution: amateescu commentedLet's try this reroll.
Comment #112
aspilicious CreditAttribution: aspilicious commentedHad a merge conflict? ;)
How is the performance of this last reroll?
Any idea how to improve this in the future?
What is needed for this patch to get in?
Any other maintainer is going to be added to this patch? Can we remove the todo?
Comment #113
andypostI think we need to get rid of menu.inc at all. Working on #1814916: Convert menus into entities I'd like to decouple this classes into
lib/Drupal/Core/Menu/Entity/
folder to allow menu and menu_item modules to stay optional. Ideally we can move some abstracted functionality into Component folderComment #114
BerdirThat's not the goal of this issue.
This is just about menu *links* functionality, which builds on top of menu router functionality, which makes up the other half of menu.inc. The remaining stuff in menu.inc we'll hopefully get rid of when everything has been ported to the new router system. Whenever that will happen :)
Comment #115
tim.plunkett#113 would be a follow-up, likely postponed on #1847002: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity or something like it. Let's just finish this issue first.
Comment #116
amateescu CreditAttribution: amateescu commentedFixed that merge conflict and here's another re-roll to keep up with HEAD.
@#112
I don't trust my local dev environment for profiling so consider this a call for help on this area :)
Depends on the output of the above. What I know for sure that can be improved is removing the use of ArrayAccess.
Some more reviews I guess.
I don't think we can remove it, I surely can't be the sole maintainer of this critical piece of functionality.
Edit: Interdiffs can be found here: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/9163...
Comment #117
chx CreditAttribution: chx commentedI can help maintaining this.
Comment #118
amateescu CreditAttribution: amateescu commentedMerged HEAD and added @chx as a maintainer, thanks for that :)
Comment #120
andypostsomething went wrong with merge again
Comment #121
aspilicious CreditAttribution: aspilicious commentedmenu's are entities now (objects)
"+function menu_link_add(array $menu) {"
is going nuts
Comment #122
amateescu CreditAttribution: amateescu commentedOk then, let's try again.
Comment #123
andypostPlayed a lot with this patch and I think that only benchmarks delays RTBC, actually I found no visual effect with performance.
... a bit related #1783964: Allow entity types to provide menu items
Comment #124
Anonymous (not verified) CreditAttribution: Anonymous commentedtag update.
Comment #125
podaroklooks like needs reroll
Comment #126
podarok#122: menu-links-entities-122.patch queued for re-testing.
Comment #128
podarokreroll
Comment #130
podarokyet one
Comment #132
podarokwoops
yet one
Comment #133
podarokto MenuLink array conversion test fix
Comment #134
podaroksome profiling info for the admin/structure/menu/manage/admin page
without patch #133
with patch #133
6,5% regression
Comment #135
podarokand here are top calls output
Comment #136
podarokremoving tags
Comment #138
podarokreroll
Comment #139
podarokbot?
Comment #141
dawehnerThis just fixes some of the issues, if noone took it over, I will try to fix them later during the day.
Comment #142
dawehnerSo the current problem is the following:
One possible solution solution could be to just store the menu item IDs in the config file and load them on the fly.
Do you agree with that?
In general though it feels wrong/bad to have content entities as part of config entities. The other way round sounds better.
Comment #143
tim.plunkettIf we store content entities in a config entity, we'd have to use the UUID. Not sure if that's a good thing or not, but not sure what the other options are.
Comment #144
andypostShortcut sets does not save links within itself but probably could save a set of UUIDs of links
The suggestion was to make links exportable with shortcuts #1497380-37: Convert shortcut sets to ConfigEntity
Also I filed follow-up #1895938: Add label() method to ShortcutSet where we can clean-up/refactor a logic behind a shortcuts
Comment #145
podarok#141 needs review
Comment #147
aturetta CreditAttribution: aturetta commentedBut first it needs a re-roll...
Comment #148
dawehnerThis is far from a real rereole as shortcut is still not working fine, but at least the shortcuts in the standard.install get's created.
I will work on that tomorrow.
One strategy to reduce the amount of changes could be to a) just save the uuids but load all menu items during runtime.
Comment #149
dawehnerPuh, finally got it working.
Comment #151
dawehnerHere is the right version.
Comment #153
andypost#151: drupal-916388-151.patch queued for re-testing.
Comment #154
andypostFixed deprecated entity_get_controller() and menu_link_add() to use Menu entity
Comment #155
andypostUpgrade for shortcut was wrong, so filed #1899682: Add upgrade path tests for shortcut module
Tests should pass now
Comment #156
andypostGreen again!!!
Comment #157
dawehnerWhat a huge amount of work done here, hopefully I didn't went too much into details with that review (stopped after some time, so that's certainly not complete).
Thanks andypost to make it green again!
I guess we should use the drupal_container() to factor us a query now.
As the entity query already does $id => $id, can't we just set $parents += $result?
I'm wondering whether we could still make this configurable by putting that has a service into the container, like menu.link_storage.fallback ?
Another question: can we actually rely on that class, as it's part of the menu_link module?
Should we try to keep these kind of changes out of the patch, so it's smaller? I don't think this is worthwhile to remove again.
What about putting a @todo here and open a follow-up?
Maybe we could put that into a method on the link controller, so it can be replaced if needed?
Whether you put that into aggregator menu_link, it always feels wrong.
What about $node->book->save() instead?
Shouldn't we at least try to log something?
We could also get the controller from the plugin.manager.entity, but I'm not sure whether that is the "standard" now.
It seems to make sense to verify that the new title was saved.
I guess we should use entity_load directly as much as possible?
Let's use $this->container->get('entity.query')->get('menu_link')
I'm wondering whether we should put such default values into the storage controller?
Another place where it could make sense to default mlid to 0?
Couldn't we try to use just a single entity query, as IN is actually not required and the rest of the query is the same?
Missing "\" at the front.
What about using $delta = max($delta, 50)?
I think 'path.alias_manager.cached' is the proper service to use here.
Don't we want to cast it to a boolean?
Wouldn't cause this to loose some data?
Needs documentation.
In order to be able to override better use static::
Do we use array_diff for such cases beside of http://drupal.org/node/1525176 ?
static
Same as before :(
Maybe it would help to have a general description what we do in that function?
Shouldn't we use an entity query instead?
Can't we cast it to int directly?
I would say so.
In general I like to provide API methods but using entity_query at the end.
According to http://drupal.org/node/1354 it should be int instead of integer
Some of those could get some documentation :)
What about adding a @deprecate here, so people don't start to use it?
What about adding a @depcreated here as well?
I would love to have such a documentation standard in core, see #1772420: [policy only] Documentation standard for arrays of a certain type (param/return types)
No need to reference it, as it's already an object.
Comment #158
andypost@dawehner thanx a lot for deep review
Patch addressed most of all except documentation of variables and changes to controllers
Also #1899682: Add upgrade path tests for shortcut module is commited so no this hunk
Comment #160
andypostReverted book changes. Seems arrayAccess still needed
Comment #161
dawehnerDid some more review coming from the bottom, so maybe read it like that as well.
Missing docs.
MenuLink is one of these examples where replacing the storage controller could be a real usecase, so we should implement an interface in order to tell people about new exposed methods. Could be another follow up.
Another odd comment :)
Should we document all this vars in this issue, or open an follow up?
Maybe there should be a comment, why the plid is removed when duplicating.
At some point we should rename these functions as they don't actually get the router but the menu links back.
I think we still want to expose the full namespace, but I'm not sure what the current "standard" tells us to do.
... or false if it was not possible to be loaded.
This function is not used anywhere, I think we should drop it. In general the name is quite confusing as it does something on multiple items.
This function can be removed entirely as the logic is handled in the storage controller now.
Added a follow up to convert them: #1903774: Replace entity_load_multiple_by_properties() uses in menu module/menu tests with an entity_query
Opened a follow up for that: #1903768: Fix coding standards in MenuTreeTest
Shouldn't we use entity_create here instead, because you might want to replace the menu_link with another implementation?
This maybe throws a notice, as menu_links is not defined.
See #1887058: Remove usages of entity_load_multiple_by_properties() and EntityStorageController::loadByProperties() so probably also worth for another follow up?
It feels kinda wrong to let this function return menu link entities? In general at least we should maybe add a follow up to fix the documentation of that function.
Opened a follow up for that #1903750: Convert menu_link hooks to use Type-Hinting
We should maybe add at least a @todo or try to avoid it in general, as this could add a cost to most of the tests.
Comment #162
tim.plunkettDefinitely needs work for removing the dependency in the testing profile.
Comment #163
amateescu CreditAttribution: amateescu commentedImplemented all the changes suggested in #161 except the following:
Actually, I plan to take everything out of the storage controller and move this responsability to a new Tree field module, so most of the storage controller code from here is temporary.
The current standard is that when you are in a namespaced file, you can leave out the full namespace.
Even better, I converted that test to DUTB :P
The plan is to move those system_* blocks to menu_link.module, here's the followup issue: #1842854: Move system menu block to core
Menu link is a *required* module for now (what can you expect from something coming from system.module and menu.inc :P) and I just wanted to explicitely show the new dependency for all profiles. Anyway, I removed the explicit dependency by popular request :)
Also, I had to start a new branch for this, starting with the patch from #160 because of some re-rolls that didn't contain an interdiff :(
Comment #165
amateescu CreditAttribution: amateescu commentedI've no idea who broke
user_menu_link_presave()
and turned it intouser_menu_link_alter()
:/Comment #166
mgiffordThis patch applies nicely. I didn't see if it changed things to entities, but menus still seem to function fine with my brief testing.
I was looking at a somewhat related issue #1543750: Allow menu items without path and had hoped that it might have been fixed here. Unfortunately, you still can't seem to be able to add a menu item without a path:
Error message - The path '#bottom' is either invalid or you do not have access to it.
Comment #167
pwolanin CreditAttribution: pwolanin commentedLooking at this patch, I'm not sure I see how the LEFT JOIN to menu_router is replaced? After this patch is access to a link no longer checked? Or that's fully postponed to a follow-up issue?
Comment #168
amateescu CreditAttribution: amateescu commentedThe LEFT JOIN is not replaced, just moved to
MenuLinkStorageController::buildQuery()
.No change is done regarding access checks, they work just like now thanks to the (temporary) ArrayAccess implementation of the menu link entity. The followup issue is to actually move that access logic to the entity controller.
Comment #169
catchOK I just read through again and couldn't find anything to complain about.
There's a very significant performance regression here, and this is going to need to be added to #1744302: [meta] Resolve known performance regressions in Drupal 8 once it's in.
On the other hand, there is #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees which for cache hits would be a significant performance improvement compared to Drupal 7 menu link caching (since we'd not be loading the menu links at all for most requests).
Going to bump this to RTBC since it blocks a lot of work, but I'll give it at least 72 hours before commit in case there's any last minute concerns.
Comment #170
larowlanAdded #1911080: Replace menu node form additions with entity reference field
Comment #171
webchickBy Australia time, it's now been more than 48 hours. :) I'm going to call that good enough, so we can make progress here at the sprint. Hope that's ok, catch.
Committed and pushed to 8.x (adjusting the update function number). Thanks!
This will definitely need a change notice methinks. :)
Comment #172
dman CreditAttribution: dman commentedhook_translated_menu_link_alter() has gone away.
hook_menu_link_alter() has gone away.
Obviously this is a major (good) change that invalidates a few pending patches, like This big documentation cleanup :-(
Yes, a change notice about that would have been useful.
NVM, the guys at the table here pointed out that api.drupal.org was now out of date with the truth... :-B
Comment #173
fagoIs there an issue for EntityNG conversion of menu links yet?
Comment #174
amateescu CreditAttribution: amateescu commentedYep, it's in the OP. Working on a change notice now.
Comment #175
amateescu CreditAttribution: amateescu commentedChange notice created: http://drupal.org/node/1914008
Also added this issue to #1744302: [meta] Resolve known performance regressions in Drupal 8.
Comment #176
andypostSuppose change notice should describe dropped menu_link related functions and mentioning about access checks must tell hiw to get access-checked menu-link
Comment #177
amateescu CreditAttribution: amateescu commentedI haven't seen change notices to document the removal of functions, so only hook updates are mentioned for now.
Added a note about getting access-checked menu links under the load example code.
Comment #178
BerdirHm.
This still slows down menu rebuilds enough to considerably slow down test runs. Did a lock test with a randomly picked test class ("Drupal\user\Tests\UserAccountLinksTests"), took 11s before this patch and 15s now.
This results in a lot of aborted test runs as they now take too long and a huge backlog.
Will have a look to see if there's something we can do...
Comment #179
BerdirHave been profiling this a bit.
Made two small changes that gets the execution time of my example test back to 13 seconds but there's a lot of things that could be improved.
What I did:
- Remove the count query in findParent() and instead count() the results. That is rather pointless if we, in most cases, expected to find exactly one result, which seems to be the case. This did get the amount of queries from 7xx to 4xx.
- Use a single db_update() in updateParentalStatus() instead of load() & save(). While not nice, that is *considerably* faster.
Some other things that I noticed/some thoughts:
- We're doing hundreds of tag cache clears, for every single saved menu link. Every single saved menu link also calls through the reset function and does a full resetCache(). We should probably inline the (static) cache clears that we actually need and avoid multiple static clears during a single rebuild.
- Seeing 3 calls to dfac() for every test method, twice in the installer and once after all modules were enabled. Wondering if we can improve that. For testing purposes, I commented out the one in resetAll() and things worked just fine. And it saved another 2 seconds. We might want to open an issue to try and get rid of that call there. And maybe look at those in the installer and try to do more specific cache clears there as well instead of using the sledgehammer-method :) This could more than revert the performance regression during tests if we can get that right.
- 1200 calls to menu_get_ancestors() and therefore 1200 uncached calls to the state keyvalue store. I need to get back to that caching issue.
- findParent() is still quite slow.
- Wondering if we can improve updateParentalStatus(), maybe collect plid's and do a single check after we saved all the menu links? It is recalculating many plid's multiple times.
- 2k calls to Drupal\Core\Entity\DatabaseStorageController::create, 90% of that is spent in the (new) create hook, the only implementation seems to be field but a lot of the time actually seems to be spent in rebuilding/calculating the existing hooks, as far as I can see.
Comment #180
sunThose relatively simple measures look sensible to me.
That said, I still did not look at the originally committed patch, but I'll file follow-up issues if necessary and link to them from here.
Comment #181
plachAny idea of how to make this work with storage-agnostic entities? Are we requiring menu links to be always stored on a SQL database?
Comment #182
chx CreditAttribution: chx commentedI will work on #1893772: Move entity-type specific storage logic into entity classes and then other storrage engines can override the specific storage controller.
Comment #183
plachCan we put the lines above in a method of the
MenuLinksStorageController
so that we can provide different implementations depending on the storage without needing to rewrite the whole method?Comment #184
plach@chx:
(crosspost) Yes, I didn't notice the lines were already part of the storage controller.
Comment #185
BerdirI guess something like that will need to happen in #1893772: Move entity-type specific storage logic into entity classes, doesn't make much sense to create a method now and then have to move it over to a new class IMHO.
For the record, my patch in #179 was more a work in progress and still took 1h30 on that testbot, that one seems to vary between that and up to 1h45m at the moment, so I'm not sure how much my patch actually improves the situtation. it's likely that the difference was bigger on my local system. But it should improve the situation at least a bit and I'me fine with committing that and then open additional separate follow-up tasks.
Comment #186
plachOk, works for me.
Comment #187
catchThanks Berdir. Those changes look great and I've committed/pushed to 8.x, leaving open for the change notice.
Comment #188
amateescu CreditAttribution: amateescu commentedWell, the change notice exists already.. is there anything else we'd like to see in it?
http://drupal.org/node/1914008
Comment #189
dcrocks CreditAttribution: dcrocks commentedShouldn't the " * @ todo The paragraph above is lying! This hasn't been (re)implemented yet." be removed from menu_link.api.php for hook_menu_link_presave?
Comment #190
amateescu CreditAttribution: amateescu commentedNope, not yet. We have to figure out if we want to condition firing entity load hooks for menu links based on the old
options['alter']
mechanism. And this @todo is onhook_menu_link_load()
, not presave.Comment #191
dcrocks CreditAttribution: dcrocks commentedThis is probably the cause of #1912806: Regression: 'User Account' displayed on front page for anonymous users. What should I look at?
Comment #192
podarokchange notice looks good
Comment #193
amateescu CreditAttribution: amateescu commentedReverting the priority as well.
Comment #194
amateescu CreditAttribution: amateescu commented.. and the title :/
Comment #195
holtzermann17 CreditAttribution: holtzermann17 commentedLooking at the API docs for menu_link and menu, I'm not seeing what seems like a straightforward Organic Groups-like "win" -- the ability to straightforwardly delete the membership of a link in a given menu. Did I miss something? In D7 there's menu_link_delete, which, of course, gets rid of the entire link, and menu_link_save which allows for a complicated way to do revisions (the
$existing_item
). Maybe converting links into entities will make that easier? (Sorry if I've missed something "obvious"!)Comment #196
tstoecklerRe @holtzermann17:
Converting menu links into entities allows for
$menu_link->save()
and$menu_link->delete()
. Menu links are not revisionable in core but in theory that is overridable in contrib, although that will probably be non-trivial. It's only "obvious" if you are up to speed with the new entity API, so no harm done asking :-)Comment #197
holtzermann17 CreditAttribution: holtzermann17 commented@tstoeckler: Thanks.
After looking a bit further I see that the D8 entity has something called MenuLink::$menu_name -- changing that would presumably change what menu the link is assigned to, or one could copy-and-modify to make two similar links in different menus. This isn't exactly the "OG model" I was thinking about but it does seem potentially less awkward than the D7
menu_link_save
method.Comment #199
Gábor HojtsySo long as the menu items are not in bundles per menu, language settings per menu are impossible as-is right now. Language settings for entities are bundle based, so eg. (for nodes) you can make your press release bundle type translatable while your blog posts are not. This makes them behave pretty differently in terms of UIs showing up for translation, etc. If there is only one bundle for menus or the bundles are cross-menu or even different depending on the level the item is in the menu, the language settings are impossible. I opened #1966298: Introduce menu link bundles per menus to discuss this (again).
Comment #200
andypostFiled follow-up #1966330: Move all menu_link related code to its home
Comment #200.0
andypostRewritten the issue summary.
Comment #201
jnicola CreditAttribution: jnicola commentedI upgraded D8 alpha 6 to Alpha 7, and this is the error I am seeing now:
Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("Some mandatory parameters are missing ("menu_link") to generate a URL for route "/admin/config/user-interface/shortcut/link/{menu_link}/delete".") in "core/themes/seven/templates/page.html.twig" at line 74. in Twig_Template->displayWithErrorHandling() (line 279 of /var/sites/jessenicola.dev-vm/core/vendor/twig/twig/lib/Twig/Template.php).
It happens when accessing /#overlay=admin/content, and going to directly to admin/content causes the same issue.
Is there an issue for this already, or worthy of opening a new issue?
Comment #202
effulgentsia CreditAttribution: effulgentsia commentedFolks here might be interested in / have input for #2178723: [meta-2] Finalize module-facing API of declaring menu links, especially comments #39 and later.