Problem/Motivation
see: #2256497: [meta] Menu Links - New Plan for the Homestretch
The architecture and general implementation is being reviewed in #2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins. Comments on those aspects of the patch should be posted to that issue.
This issue is to focus on implementation details, tests, and code and comment polish necessary to get the patch committed.
Proposed resolution
This issue to to build to a patch that will replace almost entirely the system in HEAD with a new one with minimal user-facing changes but substantial new features under the hood and performance gains.
- Define an extensible framework for different types of links, with a plugin type as a common facade
- The developer-facing APi is a plugin manager that also handles building trees into render arrays and access checks. The tree storage is a separate service that hides the implementation details of efficient hierarchy handing
- Manage links defined in YAML and links defined by views
- Full l10n support for localizing the D8 admin interface
- i18n support via config translation for translating the links provided by Views.
- A NG content entity whose base table stores the plugin definition
- The custom menu link entity is defined as being fieldable
- Link title is the (translatable) entity label
- Menu link content entity can be added/edited via the node form (substituting for existing functionality)
Remaining tasks
- #2301239: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage
- #2301273: MenuLinkNG part2 (no UI or conversions): MenuLinkTree API and unit tests
- #2301313: MenuLinkNG part3 (no conversions): MenuLinkContent UI
- #2301317: MenuLinkNG part4: Conversion
- #2301319: MenuLinkNG part5: Remove dead code; and party!
User interface changes
Minimal. Module-defined menu links will have static title, description, and path that are not editable via the UI.
API changes
A new menu link system.
For reviewers there are 3 very notable changes:
- All menu links operate behind the facade of the \Drupal\Core\Menu\MenuLinkInterface. This means that multiple different implementations can operate together in the same tree. In addition, since we consider them as plugins, the details of the optimized tree storage are not exposed. The MenuTreeStorageInterface takes only a plugin definition and returns it again, while the details of the implementation (e.g. the p1-p9 columns) are not accessible or part of the API.
- The methods for loading and rendering menu link trees have been completely consolidated and made consistent in just 3 methods on the MenuLinkTreeInterface. A lot of previously crufty code that was forward-ported from 6 and 7 has been removed, aided by the decoupling of breadcrumbs form menu links
- The code for performing these steps has been broken up into multiple services, each with a clear interface so that in the cases where the behavior needs to be customized for a site or for a different tree storage (e.g. noSQL) only a limited amount of logic needs to be re-implemented to affect the change.
Patch map
This patch completely removes the menu_link module. It moves some of that module's former logic into \Drupal\Core\Menu, some into a new menu_link_content module, and removes some former logic. Details:
- hook_translated_menu_link_alter(): removed. Can override plugin class instead.
- hook_menu_link_CRUD_OP(): removed. Menu links are no longer entities, but for custom links, there's now hook_menu_link_content_CRUD_OP().
- hook_menu_link_defaults_alter() -> hook_menu_links_discovered_alter().
- menu_link_schema() -> MenuTreeStorage::schemaDefinition()
- fields changed:
- uuid + machine_name -> id
- plid -> parent
- link_path -> url OR route_name and route_parameters. url is only populated for external links
- langcode + link_title -> title + title_arguments + title_context
- module -> provider (in line with all other plugins)
- fields removed:
- customized -> (replaced with MenuLinkInterface::isResetable())
- external (replaced by check if url is populated or Url::isExternal() after getting the Url object)
- updated (this was outdated for D5 -> D6 update)
- fields added:
- description
- class
- metadata
- form_class
- private fields (in the schema of MenuTreeStorage, but not accessibly via any API):
- route_key
- discovered
- p1..p9 (these were public in HEAD, but are made private by the patch)
- fields changed:
- menu_link_help(): removed. Do we need a menu_link_content_help()?
- menu_link_uri() -> MenuLinkInterface::getUrlObject()
- menu_link_CRUD_OP(): removed as menu links are no longer entities. Interact with MenuLinkManager instead (e.g., $menu_link_manager->getInstance(), $menu_link_manager->updateLink(), etc.).
- menu_link_maintain(): removed. See above.
- menu_link_system_breadcrumb_alter() -> menu_ui_system_breadcrumb_alter().
- Entity\MenuLink ->
- Storage-related functionality of link hierarchy and rebuild moved to MenuTreeStorage.
- Storage-related functionality of custom links moved to MenuLinkContent.
- MenuLinkAccessController ->
- For routes (e.g., menu_ui.link_edit) that only need to check "administer menu" permission, check just uses
_permission
. - Access control of custom links moved to MenuLinkContentAccessController.
- Access control of menu_ui.link_reset moved to MenuLinkResetForm::linkIsResetable().
- For routes (e.g., menu_ui.link_edit) that only need to check "administer menu" permission, check just uses
- MenuLinkDeleteForm -> MenuLinkContentDeleteForm
- MenuLinkForm -> MenuLinkFormInterface (with MenuLinkDefaultForm and MenuLinkContentForm implementations)
- MenuLinkInterface: removed as obsolete. The new MenuLinkInterface contains different methods.
- MenuLinkStorage ->
- Tree-related storage moved to MenuTreeStorage
- Entity related storage removed as obsolete, because MenuLinkContent can use regular content entity storage.
- MenuTree -> split into various new classes in \Drupal\Core\Menu.
- StaticMenuLinks: removed. Static link discovery embedded into MenuLinkManager.
This patch also moves a bunch of procedural code from menu.inc to appropriate classes and removes code made dead by the new architecture. Details:
- _menu_item_localize(): removed. MenuLinkInterface::getTitle() and other getter methods return localized strings.
- _menu_link_translate() ->
- unserializing moved to MenuTreeStorage::prepareLink()
- access checking moved to a "menu tree manipulator" (DefaultMenuLinkTreeManipulators)
- title/URL/etc. expansion moved to MenuLinkInterface getter methods.
- menu_(get|set)_active_menu_names(): removed. Was obsolete code even before this patch due to refactored breadcrumbs already in HEAD; this patch just surfaced that.
- menu_link_get_preferred() -> MenuActiveTrail::getActiveLink().
- menu_reset_static_cache(): removed, because that static cache is removed.
- _menu_link_save_recursive() -> MenuTreeStorage::saveRecursive().
- menu_link_rebuild_defaults() -> MenuLinkManager::rebuild() which passes the discovered definitions into MenuTreeStorage::rebuild().
- menu_load_links() -> MenuTreeStorage::loadByProperties().
- menu_delete_links() -> MenuLinkManager::deleteLinksInMenu().
- _menu_(update|set)_expanded_menus(): removed. This optimization was not carried into the new architecture for simplicity. This might need profiling to determine the performance cost, but once we render cache menus, it might change the value of this optimization.
Comment | File | Size | Author |
---|---|---|---|
#144 | Screenshot 2014-07-11 08.08.17.png | 152.83 KB | larowlan |
#144 | Screenshot 2014-07-11 07.38.26.png | 69.77 KB | larowlan |
#144 | Screenshot 2014-07-11 07.38.15.png | 47.53 KB | larowlan |
#144 | Screenshot 2014-07-11 07.37.53.png | 19.5 KB | larowlan |
#144 | 17vyt0zgrb5o8jpg.jpg | 17.84 KB | larowlan |
Comments
Comment #1
larowlanWilling to help here
Comment #2
pwolanin CreditAttribution: pwolanin commentedComment #3
xjmSetting status per #2256497-28: [meta] Menu Links - New Plan for the Homestretch, and postponing on step 1.
Comment #4
pwolanin CreditAttribution: pwolanin commentedI still consider this a beta target, and plan to get it done.
Comment #5
pwolanin CreditAttribution: pwolanin commentedComment #6
pwolanin CreditAttribution: pwolanin commentedThis issue will solve translation of menu links, so it's critical as a replacement for this other critical issue: #1966398: [PP-1] Refactor menu link properties to multilingual
Also related: #1848552: Toolbar icons disappear with translated menu Though marked closed/fixed, the current code would break again as we make the title translatable, so we need to address that problem as part of this patch.
however, we make be blocked with PHP warnings in tests until this is resolved: #2143291: Clarify handling of field translatability
Comment #7
sunThis functionality should not live in a separate module.
There's no need for that level of pluggability. A separate module only harms UX as well as installation and test performance for no benefit in return.
Comment #8
pwolanin CreditAttribution: pwolanin commented@sun - which functionality should not be separate? The content entity and the menu UI?
Comment #9
pwolanin CreditAttribution: pwolanin commentedComment #10
pwolanin CreditAttribution: pwolanin commentedComment #11
effulgentsia CreditAttribution: effulgentsia commentedResetting priority to match the parent issue. For details, see #2256497-36: [meta] Menu Links - New Plan for the Homestretch. This could still be raised to critical in the future if warranted (e.g., if a core maintainer decides that performance of admin/config warrants critical priority or if critical bugs are discovered in HEAD that require this).
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedFrom #2227441-94: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins:
Not my call, but I disagree with the above. In #2256497-28: [meta] Menu Links - New Plan for the Homestretch, Dries requested no interim critical regressions. Core doesn't ship with any nodes either, but performance of node rendering is considered critical. Similarly, I think the performance of front-end menus is more critical than performance of admin menus/pages. Especially since this issue implements custom menu links as EntityNG relative to HEAD's non-NG, I think even with multiloading differences aside, we need to know whether it's a performance regression relative to HEAD or not prior to commit, but to measure that, we need the patch to implement multiloading so at least that part is equivalent to HEAD. That said, I have an idea for how to do that, which I'll post here after there's an updated patch that applies against HEAD.
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedHere's a rough multiload implementation. Interdiff relative to #2227441-97: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins. Further refinement of it should be discussed in #2275297: Implement multiload for menu_link_content, not here.
Comment #15
pwolanin CreditAttribution: pwolanin commentedThe current code is in a new sandbox branch: http://cgit.drupalcode.org/sandbox-dereine-2031809/log/?h=2256521
Will post a patch here one I'm sure things work after the PSR-4 move, the multi-load fix, and some other changes.
Comment #16
victor-shelepen CreditAttribution: victor-shelepen commentedShould we move the patch from this branch to the sandbox?
Comment #17
pwolanin CreditAttribution: pwolanin commented@linkin - not sure what you mean. All the most current patch code is in that branch in the sandbox.
Working from current HEAD (34c6f66988fcbd0ef1222e5653be38d8f0eeb9a3) with a clean install or the patch on top of that (attached), I get a pretty dramatic difference in performance for /admin/config.
It's a bit hard to read in into the changes per method, since many of them were renamed or moved.
This comparison was made with a warm cache (page reloaded a few times before collecting the xhprof run). The actual xhprof data is attached. The run1 in the diff is HEAD, while run2 is with the patch, so the 9% reduction in function calls is consistent with what I saw before.
NOTE: the memory difference seems to have been inflated by opcode cache being full - going to re-run.
Comment #18
pwolanin CreditAttribution: pwolanin commentedOk, re-running it, still the same improvement in # function calls and time, but the difference in memory use is trivial.
An example of a single method that shows a big change is Drupal\system\SystemManager::getAdminBlock, where HEAD and the patch have the same number of function calls, but the patch code is much more efficient.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedAttached is a patch that contains a tester module I wrote that generates a menu with 100 custom links on install, and renders it on /menu-bench. On my machine, running
ab -c1 -n100
:HEAD: 295ms
Patch (#17): 417ms
That's a pretty serious regression. I think we need to get to the bottom of what's causing it: is it all due to EntityNG, and can it be optimized?
Comment #20
pwolanin CreditAttribution: pwolanin commented@effulgentsia -Are all 100 showing on one page? It would seem so since they are all using that route.
menu links were not converted to NG entities previously because of the performance issues it caused. So we should dig in, but this result doesn't surprise me. We are providing user-created links as NG content entities for the features like translation support and field-ability which have overhead.
As far as performance optimizations, dawehner and I have discussed with berdir making the several of the base table fields into one field like is proposed for shortcut as a follow-up, though I'm not sure how much that matter if you just load the entity to get the title and description.
Also, our phase 3 proposal of a link field would also potentially eliminate need to load a menu link content entity for links connected to nodes, etc.
I'm sure we'll want to get some general entity performance improvements in core regardless.
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedOne difference I'm seeing is that in HEAD, the "tree-data" cache stores the MenuLink entity objects, meaning they do not need to go through the whole loadMultiple() flow on every request. However, with the patch, that cache stores the plugin definition arrays only, so the plugin instance objects need to be factory created and the MenuLinkContent entities need to be loaded on every request. The latter part might get better with #597236: Add entity caching to core.
I think we'll need more discussion with catch and Dries on how much of a perf regression is acceptable, especially considering #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees will end up caching the entire rendered output anyway, and also considering various follow ups that could optimize things. However, a concern that amateescu and others raised from the beginning was that the plugin architecture would add overhead, and I think we have here some early indication that it is, though possibly we can still improve that.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedImprovements to the tester module. #19 had all 100 links going to the route that was being displayed, causing all 100 to be loaded by menu_link_get_preferred() despite the tree-data caching mentioned in #21. This one points those links elsewhere to avoid that.
With this test, here are the numbers with xdebug disabled (the numbers in #19 were with it enabled):
HEAD: 137ms
Patch (#17): 217ms
Yes. The point of this is to benchmark a not-all-that-rare case of a site with a large site (non-admin) menu.
Comment #23
pwolanin CreditAttribution: pwolanin commented@effulgentsia - if HEAD is actually caching (serializing) the entities that sounds pretty broken to me, or at least a lot more fragile and prone to getting stale depending on how the entities are updated.
I'd rather rely on caching by the entity system: #597236: Add entity caching to core
I'd be surprised here if the plugin element is adding any significant overhead - there is very little code or logic in it.
In IRC, berdir confirms that content entities are known to be slower that base entities. So again, I see this approach as allowing for a faster OOTB admin experience and toolbar, adds critical i18n features, and gives us a lot of flexibility.
It would be easy in contrib using this plugin facade to add a non-entity custom link module for mono-lingual sites. You could just rely on the tree storage, so this would be fast and simple. BUT you don't get translations or fields. The flexibility of the plugins will enable a lot of contrib innovation, and is another benefit.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedHow about this: what if for this issue's scope, we change the MenuLinkContent plugin to not forward getTitle() and getDescription() to the content entity, but instead have it return what's in the plugin definition (and on createLink(), updateLink(), etc. have it populate that definition with the monolingual value)? That would not be a functional regression compared to HEAD. Then, we can have a followup issue to make multilingual titles/descriptions work. The benefit of doing it that way is it completely removes the entity loading and TypedData overhead from the render path, bringing the number from #22 down to 158ms: still slower than HEAD, but not by nearly as much, and perhaps we can still find other things to optimize to get it even closer.
This way, we can clear the path to getting the general 600k refactor in, and then have a separate issue to discuss how much performance trade-off is acceptable to support multilingual menus by default. Perhaps even the followup issue could implement something smart like only forward to the entity if more than one language is enabled on the site.
Comment #25
pwolanin CreditAttribution: pwolanin commented@effulgentsia - that's an interesting suggestion, since we could get the best of both. I'll take a crack at it.
Comment #26
pwolanin CreditAttribution: pwolanin commentedHere's a re-roll to account for #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities and to try to implement the optimization suggested by @effulgentsia. It looks like the isMultilingual() is a reasonable check to use, but happy to have suggestions if there is something better or more reliable for this use case.
Comment #28
pwolanin CreditAttribution: pwolanin commentedMissed one entity schema change needed in that test:
Comment #29
pwolanin CreditAttribution: pwolanin commentedThis has a lot of code style and comment cleanup, but no functional code changes other than removing the default table name for the tree storage.
Comment #31
pwolanin CreditAttribution: pwolanin commentedoh, silly me - needs to be public.
Comment #32
pwolanin CreditAttribution: pwolanin commentedComment #34
pwolanin CreditAttribution: pwolanin commentedAnd small test fix due to constructor change.
Comment #35
pwolanin CreditAttribution: pwolanin commentedComment #36
pwolanin CreditAttribution: pwolanin commentedre-roll for conflict in system.schema.yml
Comment #37
pwolanin CreditAttribution: pwolanin commentedWhile the proposed architecture was reviewed and marked fixed/accepted in the prior issue, catch asked for it to be posted here again to facilitate final review.
Here's a patch for review only containing only the new files added by the patch, which are:
core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
core/lib/Drupal/Core/Menu/Form/MenuLinkFormInterface.php
core/lib/Drupal/Core/Menu/MenuLinkBase.php
core/lib/Drupal/Core/Menu/MenuLinkDefault.php
core/lib/Drupal/Core/Menu/MenuLinkInterface.php
core/lib/Drupal/Core/Menu/MenuLinkTree.php
core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
core/lib/Drupal/Core/Menu/MenuTreeStorage.php
core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php
core/lib/Drupal/Core/Menu/StaticMenuLinkOverridesInterface.php
core/lib/Drupal/Core/ParamConverter/MenuLinkPluginConverter.php
core/modules/menu_link_content/menu_link_content.info.yml
core/modules/menu_link_content/menu_link_content.install
core/modules/menu_link_content/menu_link_content.local_tasks.yml
core/modules/menu_link_content/menu_link_content.module
core/modules/menu_link_content/menu_link_content.routing.yml
core/modules/menu_link_content/src/Controller/MenuController.php
core/modules/menu_link_content/src/Entity/MenuLinkContent.php
core/modules/menu_link_content/src/Entity/MenuLinkContentInterface.php
core/modules/menu_link_content/src/Form/MenuLinkContentDeleteForm.php
core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
core/modules/menu_link_content/src/MenuLinkContentAccessController.php
core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php
core/modules/menu_link_content/src/Tests/MenuLinkContentUITest.php
core/modules/menu_ui/src/Form/MenuLinkEditForm.php
core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php
core/modules/system/config/install/menu_link.static.overrides.yml
core/modules/system/src/Tests/Menu/MenuLinkTreeTest.php
core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
core/modules/system/tests/modules/test_page_test/test_page_test.menu_links.yml
core/modules/user/src/Plugin/Menu/MyAccountMenuLink.php
core/modules/views/src/Plugin/Derivative/ViewsMenuLink.php
core/modules/views/src/Plugin/Menu/Form/ViewsMenuLinkForm.php
core/modules/views/src/Plugin/Menu/ViewsMenuLink.php
core/modules/views/views.menu_links.yml
core/profiles/standard/standard.menu_links.yml
core/tests/Drupal/Tests/Core/Menu/MenuLinkTreeTest.php
Comment #38
effulgentsia CreditAttribution: effulgentsia commentedI reran the benchmark from #19/#22/#24 and found the same result as #24 if the site is monolingual (default install of Standard):
HEAD: 136ms
Patch (#36): 159ms
@catch: given #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, are you concerned about this regression? I can try to dig in and find the source of it (I suspect the fairly large call chain in instantiating the plugins is partially responsible) and ideas to optimize, but how important is that if that issue will cache it anyway?
Comment #39
pwolanin CreditAttribution: pwolanin commentedEven with standard it should be monolingual?
So, if this is a general problem for plugins, we should address it in a general way for Drupal 8 as a performance enhancement?
In this specific case, I could try to instantiate all the plugins initially to see what kind of speed gain that gives, though I'm not really fond of the extra complexity and potential fragility that adds.
Comment #40
effulgentsia CreditAttribution: effulgentsia commentedI was wrong about plugin instantiation being a significant contributor. I'm still unclear on what's causing the difference.
Comment #41
pwolanin CreditAttribution: pwolanin commentedHere's a small tweak to make the base form more consistent with the entity form and use an HTML5 input element.
Comment #42
pwolanin CreditAttribution: pwolanin commentedUsing xhprof, it's easy to identify a notable difference:
Symfony\Component\Routing\Generator\UrlGenerator::doGenerate and Drupal\Core\Utility\LinkGenerator::generateFromUrl
are called a lot more with the patch since the link is generated at run time from the route and parameters. In contrast, current HEAD saves the system path as part of the entity data.
i.e. in preSave() HEAD does:
Using getPathFromRoute() which is marked as deprecated and to be removed before 8.0.
I would argue the HEAD behavior is at best fragile and arguably incorrect since it's basically storing forever something that should only be cached or not used at all. We could certainly speed this bit up in the patch potentially by generating the href and caching it earlier, though that would be a bunch of churn and maybe a change to the LinkGenerator that I'd rather do as a follow-up. I know there is also already a patch in the works to add caching to the UrlGenerator.
The change in function theme_menu_link(array $variables) is basically:
The l() function call we removed calls UrlGenerator::generateFromPath(), which is marked as deprecated and to be removed before 8.0, so it seems this is moving us closer to release in that regard. This relates also to our other discussion about uses of _system_path in core and trying to get rid of all uses except pre-boot and actual route matching.
Comment #43
catchGiven #42 that's an issue with using the generator rather than this patch.
The menu tree caching patch will hide it, I think we also need a separate issue to profile generator performance - I really didn't like it from a performance perspective when it went in originally, and it's at least as bad as expected.
Comment #44
pwolanin CreditAttribution: pwolanin commentedHere's a series of screenshots plus a small update to the patch.
Embedding the key edit forms:
Editing a static (module-provided) link
Adding a new menu link content link
After enabling multi-lingual, the link has a language selector and Translate tab
Adding a link via Views, it has a form similar to the static link, but the title can be edited.
Comment #45
Bojhan CreditAttribution: Bojhan commentedI see no major issues with this, I think that its clear to users that it cannot be directly translated or configured and we inform them what is happening.
I would suggest a few textual changes:
For static links:
This link is provided by the X module. The label and path can not be changed.
To provide more information where its exactly coming from, because thats the first thing a user would probably wonder. Especially when these links are coming through contrib modules.
For links where you know the source, such as Views.
This link is provided by Views module. The path can be changed by editing the Frontpage view.
To provide more information where this link is coming from, to decrease the amount of searching you have to do to find this link - when you have dozens of views.
Comment #46
pwolanin CreditAttribution: pwolanin commentedOk, here's a 1st implementation of the 2 form changes suggested by Bojhan, plus more code style cleanup and a little test fix.
Screen shots show the updated edit forms.
Comment #47
larowlanAttempting to review this behemoth, using https://github.com/larowlan/drupal/pull/3/files because dreditor not an option and would hate to loose my comments
Comment #48
pwolanin CreditAttribution: pwolanin commentedLooking there, it seems we have a schema.inc change in the patch that may be unrelated. Looks like it carried over from one of the other entity or field patches we needed before. Let me re-make it without.
Comment #49
pwolanin CreditAttribution: pwolanin commentedHere's that cleanup and some comment fixes.
Comment #50
AndyThornton CreditAttribution: AndyThornton commentedPostponed/duplicated https://drupal.org/node/2266629#comment-8855469 (
Fix menu link reparenting logic) on @pwolanin's recommendation
Comment #51
larowlanMy code review is here https://github.com/larowlan/drupal/pull/3
I will keep it up to date with any interdiffs posted here as well as 8.x
I have some concerns around the size/scope of the plugin manager.
I have some concerns around misuse of the config system to store overrides.
No UI or manual testing yet.
Comment #52
pwolanin CreditAttribution: pwolanin commentedThis starts addressing the feedback in that PR review.
Comment #54
pwolanin CreditAttribution: pwolanin commentedOops - ignore #52. This has the updated patch and changes since #49
Comment #55
pwolanin CreditAttribution: pwolanin commentedResponding to feedback from @larowlan that there needs to be a way to override the form element used to select the menu parent, this patch adds a method to the manager so that that element could be changed by subclassing and setting the subclass as the service.
In the prior issue for general review, we had some debate about the size/scope of the plugin manager but @effulgentsia felt he was comfortable enough going forward as-is so I'd rather not re-debate that unless we missed something. #2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins
Comment #57
pwolanin CreditAttribution: pwolanin commentedconflicts in comment.module and SystemController.php
The latter is not immediately trivial to resolve because of the changes in #2085571: admin/content should not depend on node.module.
That adds a $path parameter to SystemController::overview() that needs to be changed to a link ID. I'll have to finish re-rolling in the morning.
Comment #58
pwolanin CreditAttribution: pwolanin commentedSo, the SystemController fix seems not hard, but then there is a circular dependency problem due to the access manager and param converter. This is due to recent core change:
Issue #2250239 by dawehner | sun: Remove needless ContainerAware dependency
A work around suggested by daniel of setting injection, doesn't completely solve the problem so far. I think this may be the reason to split the actual plugin manager from the tree code that does access checking.
Comment #59
effulgentsia CreditAttribution: effulgentsia commentedReference for that is #2227441-84: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins. While decoupling more would be nice, I don't think it needs to hold up this patch unless there's something that makes things more monolithic than HEAD is already.
If needed to resolve bugs / circular dependencies, or even if you just feel like it, I certainly have no objections to you splitting MenuLinkTree into less monolithic pieces.
Comment #60
pwolanin CreditAttribution: pwolanin commentedYes, working on the split - basically we will have 2 services:
and
both will have the menu.tree_storage service injected, and the menu.link_tree will also have the plugin manager injected.
Places like the param converter and edit forms will only need the plugin manager service, and that will fix the dependency loop in terms of the param converter and access mananger.
Comment #61
pwolanin CreditAttribution: pwolanin commentedThis will still have some test fails, but it's mostly working and you can see the logic of the class split.
Comment #63
pwolanin CreditAttribution: pwolanin commentedI think the tests will pass now.
Comment #65
pwolanin CreditAttribution: pwolanin commentedoops - renamed that resetDefinitions() method and don't need to call it anyhow in the test.
Comment #66
pwolanin CreditAttribution: pwolanin commentedSome small doxygen fixes.
Comment #67
pwolanin CreditAttribution: pwolanin commentedA github PR with this new patch is at https://github.com/pwolanin/drupal/pull/1/files for anyone who wants to review it using that format (but please summarize your comments back here).
Comment #68
pwolanin CreditAttribution: pwolanin commentedHere are some other small fixes from self-review.
Comment #69
effulgentsia CreditAttribution: effulgentsia commentedThere's a new source of performance slowdown for content links. Updated numbers from #38:
HEAD: 159ms (HEAD seems to have slowed down since then)
Patch (#68): 238ms
This is about the same magnitude slowdown as in #22 prior to the isMultilingual() optimization of #26, so I thought maybe something new is causing MenuLinkContent entities to get loaded despite that optimization, but their constructor isn't running, so it's something else. Possibly an xhprof comparison could help find it.
Comment #70
pwolanin CreditAttribution: pwolanin commentedWow - that's frustrating. Let me make sure the static caching of definitions in the menu tree storage is working correctly - that would be the most likely cause since it would mean an extra 100 SQL queries on that page.
Comment #71
pwolanin CreditAttribution: pwolanin commentedSo, indeed, that was probably the problem causing worse performance, but the solution was more involved than just finding a logic error. I needed to push the tree cache down to the menu tree storage (which is really just a fancy definition cache anyhow) so that it could get back the relevant definitions from the cache entry instead of re-loading each one from the database.
I'm not sure the incremental diff has 100% of the changes since the last patch, but you can at least see the major changes.
Comment #72
pwolanin CreditAttribution: pwolanin commentedSo, I ran the ab test 2x before and after patch #71:
Patch #68
Time per request: 647.767 [ms] (mean)
Time per request: 677.689 [ms] (mean)
HEAD:
Time per request: 434.074 [ms] (mean)
Time per request: 429.293 [ms] (mean)
Patch #71:
Time per request: 438.852 [ms] (mean)
Time per request: 432.480 [ms] (mean)
So this brings it back very close to HEAD (obviously my machine is slower than Alex's)
Comment #73
effulgentsia CreditAttribution: effulgentsia commentedThanks for fixing the definition caching! On my machine, I get a larger regression of #71 relative to HEAD than what's reported in #72. I get:
HEAD: 159ms
Patch (#71): 183ms (so, better than #69)
However, I wanted to test the hypothesis of #42 that most of that is due to UrlGenerator, and indeed it is. I filed #2289319: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache to move that from theme_menu_link() to the tree-data cache, so as to bring the runtime rendering logic closer to what HEAD does. With that patch applied, I get:
Patch (#71 + #2289319): 160ms (essentially parity with HEAD, yay!)
@catch: given #43, do you think we should commit #2289319: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache (or a refinement of it) into this issue's sandbox, in order to remove the interim regression, but then potentially undo it once alternate caching is in place, or would you rather keep the code cleaner despite an interim regression?
Comment #74
pwolanin CreditAttribution: pwolanin commentedSee #42 - getPathFromRoute() is marked deprecated, so I don't think adding that back is the right way forward.
Comment #75
Wim Leerspwolanin and I discussed in detail (3.5 hour long chat detail) how to move this forward and combine with #2289033: Refactor MenuTree to be simpler and have full unit test coverage.
Even though pwolanin set out to refactor just Menu Link entities/storage, and I set out to just add render caching of menu trees (in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees), and those logically have close to zero overlap, the sad reality is that the menu system in HEAD is in a poor state. Due to the tight coupling in
MenuTree
, where lots of things live together that don't belong together, and worse yet,MenuTree
calling out to procedural functions inmenu.inc
, the unfortunate consequence is that we have both cleaned up in different ways, with different goals in mind.So, alas, even though we both had best intentions, we're now left to somehow reconcile both patches.
After careful deliberation, we've agreed to move forward by:
We both began the discussion sad and frustrated, and came out hopeful — particularly because we should be able to move faster by combining our efforts on the same patch :) Particularly, we both want this mammoth, long overdue work to be behind us :P
The plan
In short: the following will be moved into pwolanin's patch from mine:
MenuTreeItem
,MenuTreeParameters
, the menu tree manipulators concept,DefaultMenuTreeManipulators
,MenuTree::render()
(which brings all rendered menus to a single template:menu-tree.html.twig
), the use ofMenuTreeParameters
everywhere and all added unit test coverage.The rendering process becomes:
$tree = build($menu_name, $parameters)
orbuildSubtree($menu_name, $parent)
$tree = transform($tree, $manipulators)
$rendered = render($tree)
The plan: detail
pwolanin and I have also discussed how to do this exactly. Key to this all is that we agree on
MenuTreeInterface
(pwolanin renamed this toMenuLinkTree
), so that's what we discussed in detail, and where we've agreed on how to combine everything.In pwolanin's latest patch, over at #2256521-71: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module, it looks like this:
And we're going to refactor that to this, where "commented = planned to be deleted":
Those changes amount to:
MenuTreeItem
, on par with existing terminology in HEAD. The name and exact properties may change, but the concept is needed.MenuLinkTree
as focused asMenuTree
in my patch. Notes:MenuLinkTree::maxDepth()
method is the successor for theMENU_MAX_DEPTH
constant. We'd both like to get rid of it, but can't think of a better place for now. It's minor enough that it doesn't matter in the grand scheme of things, so barring a sudden insight, this is acceptable.buildPageData()
,buildAllData()
andbuildTree()
are removed in favor of a singlebuild()
method, which accepts parameters.DefaultMenuTreeManipulators
can almost be copy/pasted verbatim from my patch, including test coverage.MenuTreeParameters
value object (just like in my patch), but pwolanin preferred to have the tree manipulators to not be defined as part of theMenuTreeParameters
. This is a distinction that is also fine by me:MenuTreeParameters
is then solely for determining which menu links to load, and the tree manipulators are applied as part of a newtransform()
step that accepts the tree manipulators.buildSubtree()
was necessary/valuable (to get the tree below a given parent item). This will just be an alias:buildSubtree($menu_name, $parent) === build($menu_name, $parameters = array('parent' => $parent))
(pseudocode).getChildLinks()
belonged in the interface/was worth keeping around, but we figured that'd become clearer once all of the above had been done.resetStaticCache()
method was already planned to be removed, and will be.MenuActiveTrail(Interface)
. This is like in my patch. But unlike in my patch, pwolanin has already done the work to get rid of the proceduralmenu.inc/menu_link_get_preferred()
(YAY!) — though we probably want a better name for the method now :)MenuParentFormSelectorInterface
. We both would like to remove this, but since we want modules to (finally) be able to override this form item cleanly (e.g. to use something that scales better, like Hierarchical Select), we need this to be a service.All other methods should go away though, but that may only be addressed in a follow-up issue.
@pwolanin: if I misrepresented anything, ping me, and I'll correct it.
Comment #77
effulgentsia CreditAttribution: effulgentsia commentedAFAICT, there are no fundamental objections or concerns left with the refactor of the MenuLink non-content non-config entity type into a plugin type, thereby allowing static links, content links, and Views links, to each make independent choices on how/where to store their data. The implementation in #71 has already reached a state where there are no critical feature, UX (see #45), or performance (see #73) regressions. Meanwhile, it solves a critical regression, #2265847: [PP-0.5] Regression from D7: default titles of customized menu links aren't translated, and also adds the fantastic feature of custom links being translatable out of the box, which while per #2256497-28: [meta] Menu Links - New Plan for the Homestretch, isn't a critical requirement for releasing D8, is still a fantastic thing to get in if we can.
However, as part of implementing that refactor, the menu_link module got removed (as part of the MenuLink entity type getting removed), so its MenuTree class and a bunch of related code from menu.inc needed to move into a new \Drupal\Core\Menu\MenuLinkTree class, and along the way some of the implementation needed to get cleaned up to adjust for links as plugins rather than links as entities. Because of that, the overlap with Wim's parallel cleanup of that same code in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees is substantial.
Therefore, I think the approach in #75, and merging that into this issue, makes sense. However, that makes this a blocker for #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, so raising this to critical. And, since there's substantial data model changes in this patch, that also makes this a beta blocker. Given #2256497-28: [meta] Menu Links - New Plan for the Homestretch, I would have been against this combining and the resulting escalation if #71 wasn't already so far along. But since it is that far along and doesn't have any known critical problems left to solve (it could still use some docs and method name cleanup though), I think this is ok, especially since incorporating #75 gives us the benefit of Wim's prior work in the other issue in figuring out those docs and method names.
Looking forward to seeing the next patch with #75 implemented! Until then, this is "needs work".
Comment #78
larowlanPut me down for a review once you have merged approaches.
Comment #79
Gábor HojtsyYay, yay, yaaaaaaaay! Also: Yay! And: yay!
Comment #80
pwolanin CreditAttribution: pwolanin commentedA small note - all 4 methods on MenuParentFormSelectorInterface will be in the patch for this issue. We should just push to a follow-up the removal of getParentSelectOptions() since it requires re-doing various ajax form calls on the node type form.
Also, I think it may make more sense to pass a single manipulator instance in like this:
If the interface has a fixed set of methods we call (or even just 1 method) it would seem you could mix and match via subclassing or composing them.
-Peter
Comment #81
pwolanin CreditAttribution: pwolanin commentedActually, I'm not even sure why we need transform() to be on the MenuLinkTree class at all? The manipulator can just be called directly, or maybe pass that into the render call?
A lot of what the manipulators were doing in the prior patch is outdated since we are using Url objects and the plugin instances know how to localize/translate themselves.
Comment #82
pwolanin CreditAttribution: pwolanin commentedDiscussing with EclipseGC in IRC. There is no particular need to define value objects in general if associative arrays are already in core. Instead, he suggests simply omitting array type-hinting so that it's possible to convert to objects later if appropriate: See: #2170775: Remove array typehint from $plugin_definition
So, I'm quite sure we should not bother with MenuTreeItem, and I'm unclear in terms of the parameters array/object whether there is going to be any value.
Comment #83
pwolanin CreditAttribution: pwolanin commentedSo, I think I also missed considering the implication of moving the access check external to the menu tree class:
This would mean we could re-merge the plugin manager and menu tree since it was the requirement for the access manager that lead to the circular dependency. I think that would be ok, since we are pulling a lot of the code to other services.
I also think we can remove the getChildLinks() method since it's only used one place and you can actually get the same thing from buildSubtree() with a little extra work.
This patch is a re-roll for minor HEAD conflicts. it also removes some dead code and fixes a (new?) test.
Comment #84
pwolanin CreditAttribution: pwolanin commented1st step - create interface MenuParentFormSelectorInterface and class MenuParentFormSelector move all the parent selector code into a new service and re-factors consumers to use that service.
Also, cuts that new interface down to just 2 methods, one of which is marked deprecated.
Comment #85
pwolanin CreditAttribution: pwolanin commentedThis patch adds MenuActiveTrail and MenuActiveTrailInterface and removes the related methods from MenuLinkTreeInterface
Also changes the signature of MenuLinkManagerInterface::loadLinksByRoute(), since it was never used to load hidden links, but by allowing restriction to a menu we can possibly use it in SystemManager (and any similar place) instead of menuLinkGetPreferred()
Comment #86
pwolanin CreditAttribution: pwolanin commentedSo, here's what I'd see as the next incremental work:
to MenuLinkTreeInterface and MenuLinkTree. These would return the parameters array corresponding to what's internally generated now. I think it is a must to have helper function for this to avoid repeating the same code in several places to build those parameters.
From MenuLinkTree by using the new Parameters() methods above instead and then directly calling buildTree()
Comment #87
Wim Leers#83: looks good.
#84 & #85: look good, except missing docblocks.
I did #86, and cleaned up remnants from #84 and #85.
Comment #88
effulgentsia CreditAttribution: effulgentsia commentedAccording to pwolanin, #87 isn't the complete merge yet, so back to needs work.
Comment #89
pwolanin CreditAttribution: pwolanin commentedYes, not sure how best to mark it "needs review" for tests, but not ready for human review. This is NR for tests only!
Here's a patch that starts adding a DefaultMenuTreeManipulators service, though it's not used yet, and refactors some of the other code to make it easier to move the access checks, etc out to the external manipulators.
Comment #90
Gábor HojtsyHuge +1 to this in terms of unifying the how dynamic menu items are defined. As per #2291773: Creating dynamic hook_menu() successor items use different APIs menu items are now wildly different than any other hook_menu() successor (see views implementing an alter hook pre this patch to implement all its dynamic menu items). With this patch, menu items are brought in line with the other links (local tasks, local actions, contextual links) in terms of how dynamic items are defined.
Comment #91
Wim LeersNote that the #89 interdiff excludes some of the commits that I'd pushed to the sandbox after I posted #87. Just some small changes there, but still (reversely chronological):
In this reroll (reversely chronological):
@pwolanin: I encountered
This is fundamentally incompatible with menu tree render caching, because by doing this, you're creating two mechanisms for dynamically determining the visibility of a menu link for a given user: through access checking and by overriding this method. This ability must be removed completely in favor of access checking only. "hidden" should only be a flag that is configurable, and hence does not depend on the request context.
The same goes for the
isExpanded()
method.@pwolanin: The
data-drupal-link-systempath
attribute does not have a value in the primary and secondary menus, hence the default "Home" link doesn't get marked as active when you're on the front page.@pwolanin: Because menu links used to be only entities, and the
MenuLink
entity extended the baseEntity
, it was easy to have comprehensive test coverage for all cache tag invalidation situations. I'm not seeing anything that resembles the previousMenuLink::postSave()
. This needs to be rectified.Also, I see
Cache::invalidateTags(array('menu' => $affected_menus));
— please don't do this, you must load theMenu
config entity and call itsgetCacheTag()
method.For me, the next step is to get all rendered menus, including the primary and secondary menus, to be rendered using the same template. Only then will I be able to convert the custom tree manipulation and rendering into standard tree manipulators and the standardized rendering flow.
@pwolanin, @effulgentsia: As another next step, I'd like to see us evaluate the feasibility of removing
buildSubTree()
.One tricky aspect about
buildSubTree()
is that it includes the root (the menu link whose subtree is built). This is unlike the behavior ofbuild()
. It used to be worse still: would get a different data structure back frombuildSubTree()
than you did frombuild()
, but I fixed that. All places wherebuildSubTree()
is used, that root is actively ignored.Hence we might as well make this change:
buildSubTree($menu_name, $id, 1)
is equivalent tobuild($menu_name, $parameters = array('expanded' => $id))
. In fact, this is whatSystemManager::getAdminBlock()
has been doing all along.buildSubTree($menu_name, $id, N)
(with N>1) is equivalent tobuild($menu_name, $parameters = array('expanded' => $id, 'max_depth' => depth_of($id) + N))
.I think most of the perceived complexity lies in the parameters being difficult to understand.
If we decide against this, then at the very least I'd like to see us unify both methods into a single one.
build()
is for getting the entire tree, i.e. withroot = NULL
.buildSubTree()
is for getting a subtree, i.e. withroot = <some ID>
. Having both methods creates unnecessary confusion. Let us at least move towardsbuild($menu_name, $parameters = array(), $root = NULL)
. No confusion is possible anymore then.@pwolanin, @effulgentsia: pwolanin has expressed some reservations about porting the value object that I had in my patch to represent nodes in the tree (
MenuTreeItem
); reasons being: not enough wins, except for better documentation. Well, I say that on top of that, we'd get better DX because IDE code completion, and much more understandable code. If you see something like this:Who wouldn't run away screaming?
With a value object, that'd look like this:
$this->assertEquals($this->links[4]->getPluginId(), $tree['50000 bar test.example2']->children['50000 baz test.example3']->children['50000 qux test.example4']->link->getPluginId());
It's much clearer what's going on there; it no longer is an incredibly nested array of doom. And when developers *do* make a mistake (e.g. when writing their own menu tree manipulator), then they'll get explicit failures rather than something crashing somewhere deep down in the menu tree handling code because you used a wrong array key.
Comment #92
pwolanin CreditAttribution: pwolanin commented@Wim Leers:
re: the account link. This was directly ported from HEAD:
I think we need a way to flag links that are dynamically hidden, or have dynamic text, or a dynamic query string so they can be post-processed also. Otherwise, it's a big regression in terms of what you could do before with hook_translated_menu_link_alter(). This dynamic show/hide cannot be accomplished with an access check, since the route is accessible in both cases.
So, I think we'll need to add a new method to the menu link class like isCacheable() that indicates the rendering must be done dynamically.
I agree allowing hidden to be dynamic is particularly yucky though. In this particular case, we could instead implement the route parameters as dynamic to link to the current user profile, which should never be accessible for uid 0.
So, we could basically define the content of the Url object as dynamic (route, route parameters, title, description) if isCacheable() is FALSE
re: getCacheTag()
The menu names in the tree are actually arbitrary - the API doesn't force you to match them to any menu entity, and the menu entity is just donating this string key which we use in the link data.
So, I think the code as is is basically correct, and loading a menu entity is not even guaranteed to be possible.
re: buildSubTree() there is currently no easy way to get depth_of($id), so it's quite difficult to implement via parameters if relative depth > 1. I think having this simple method is preferable to trying to force everything through build() with parameters. I'm not sure why having the single build() method is important?
re: value objects. Part of my push back on value object is from discussions with a variety of people in IRC to understand where we are trying to go in Drupal 8 when using them. From what I've understood - using public properties is not the norm and should not used for new code being added to core. So, if you are using methods it looks more like:
And then you also have to define setters so you can change the children of the tree element, etc. I'm not fundamentally opposed to it, but it seems like a lot of work that's not necessary to get the patch done, plus some performance overhead, and the hugely nested calls seem very test-specific, not something you'd see in practice.
Comment #93
pwolanin CreditAttribution: pwolanin commentedPer discussion w/ effulgentsia adds a MenuLinkInterface::isCacheable() method so we can start to account for links that formerly would be passed to hook_translated_menu_link_alter(). Also added a @todo to MyAccountMenuLink fix it since hidden should NOT be dynamic, but we can make this into a access check if the route parameters are dynamic.
Does some small clean-up in the MenuLinkTree and MenuTreeStorage so we can further consolidate the code in the 2 build methods.
makes a fix to transform to use is_callable() and to try to use the more efficient call_user_func(). It should be possible to pass in a lambda or function name.
changes MenuLinkTreeInterface::build() back to MenuLinkTreeInterface::buildTree(). Reasoning: almost every use of build() in core returns a render array, but this does not.
Also, fixed the naming of DefautlMenuLinkTreeManipulatorsTest.php to DefaultMenuLinkTreeManipulatorsTest.php and updated it to use a new MockMenuLink class so we can re-use that in other unit tests.
Do we want to rip out MenuLinkInterface::build()?
Comment #94
pwolanin CreditAttribution: pwolanin commentedQuick further update. Adds a new test case starting to use the mock menu link class in MenuLinkTreeTest. Also fixes a bug in MenuTreeStorage::treeDataRecursive() that was for some reason still populating the p1...p9 values into the tree element array. No idea how that snuck back in there, but it's someplace in all the squashed commits.
Also simplifies the flatten() code and removes the dangling child trees.
Comment #95
Wim Leers#92:
user.module
.Then you're saying that the
user.page
route needs a route parameter like "show_profile". Then we can distinguish between/user
linking to the current user's profile and to the login page. And then the menu link can specifically refer to the profile, in which case we can deny access for anonymous users.But if we do all that, then why will
isCacheable()
still be needed?Very interesting thinking! I don't fully grok it yet though :) Hopefully this will help us solve this.
data-drupal-link-systempath
MenuLinkInterface::getMenuName()
seems to confirm that. Well, then this should use something likeMenu::load($link->getMenuName())->getCacheTag()
. If no menu (Menu
config entity) is associated, then no cache tag needs to be invalidated.buildSubTree()
build()
is important because there's then only a single way to build a menu tree. People don't have to choose, nor do they have to learn a whole lot about how our tree structure implementation details work. Even if internallybuild()
would call eitherMenuLinkTreeStorage::loadTreeData()
orMenuLinkTreeStorage::loadSubTreeData()
, that would still be a big improvement, because the main touch point isMenuLinkTree
— rarely will they need to talk toMenuLinkTreeStorage
directly.#93:
MenuLinkInterface::isCacheable()
MenuLinkInterface::isCacheable()
is unfortunately problematic; because we don't know which context the cacheability depends upon; i.e. by what it is varied. In order for this to work, we'll also need that information. In the example we've been discussing, it's cacheable per role, for example. If we have that information, then we can cache it again, per role, instead of having to render this dynamically on every page load.MenuLinkInterface
(as per the above), in which case this can be ignored :)is_callable()
call_user_func()
buildTree()
render()
should be calledbuild()
, shouldn't it? Thenbuild(Sub)Tree()
should be renamed to something else. I can't thing of any good names at the moment.construct()
is the best I can think of right now.load()
(instead of the first half of what the currentbuild(Sub)Tree()
does) would just load the needed links from tree storagetransform()
(instead of the second half of what the currentbuild(Sub)Tree()
does and what the currenttransform()
does) would transform the links into a tree and apply any manipulators to itbuild()
(instead of the currentrender()
) would turn the tree into a render arrayWhat do you think?
MenuLinkInterface::build()
MenuLinkTree
andMenuTreeStorage
: looks good.MenuLinkMock
: awesome, thank you! :)build(Sub)Tree()
(i.e. both methods) was incomplete (which in my patch used to beMenuTreeItemInterface[]
, it's now been updated to read:This just helps make my case that I made above: public properties are still a step forward compared to the inherent lack of structure in arrays.
And another symptom of the problematicness of choosing arrays over value objects. This should not be necessary.
#94:
MenuLinkTreeTest::testCreateLinksInMenu()
MenuLinkTreeTest
to a unit test rather than aKernelTestBase
test?DefaultMenuLinkTreeManipulators::flatten()
in this test? You might as well make that a static method, so you can call it from here without having to instantiateDefaultMenuLinkTreeManipulators
. Furthermore, the only reason you flatten the tree, is to count the total number of nodes. This is yet another reason to have value objects for the nodes in the tree; I had a::count()
method on mine, which removes the need to flatten it just for counting purposes.flatten()
Now working on what I said I'd do next in #91.
Comment #96
pwolanin CreditAttribution: pwolanin commentedre: My Account, I was proposing something simpler even: link to the user.page route, and maybe the route parameter dynamic with the current user uid. This will fail the access check for uid 0.
So, isCachable() is still needed since the rendered URL will be different for every user.
I general, I think we can define isCacheable() as TRUE to mean that it works with the render caching's contexts.
re: is_callable I sent dawehner a note asking for clarification, or I can trying to catch larowlan later.
RE: data-drupal-link-systempath I didn't have a chance to look into it and continued use of system path makes me a bit sad.
That seems like a regression in core?
re: cache tags. I still disagree. The menu_name string on the links is the right cache tag. Menu entities don't have anything to do with it.
The central methods are in MenuLinkManager, and I think we should eventually move the cache invalidates back there.
There is some test coverage in MenuCacheTagsTest, but I'm not sure what you were adding before.
Re: method naming, yes, the final one should not be called render() but rather buildRenderArray() or something more clear.
Let's not re-organized the rest. I really even find the transform() method as is to be a little strange, and I'd rather not expose more of the guts to the developer.
I guess I just basically disagree about the ease of having a very clear method buildSubtree that does what it's called vs. having to grok the docs if you need that functionality.
So, I would prefer we just leave it basically as it is except renaming the last method.
re: value objects, if you feel like doing that work inside the MenuLinkTree code to create the object, I don't really care, but I fell like it's still going to slow things down.
I guess you could just throw it into MenuLinkTree::createInstances() and toughly do:
In IRC chx suggested "element" was at least better than "item" since "node" would be most correct but confusing, and "vertex" unfamiliar.
re: MenuLinkTreeTest, no it' can't be a unit test, since the storage and entities use the database. However, it's still very fast to run as is.
Comment #97
Wim LeersFinished what I said in #91:
And with that, the porting of
DefaultMenuTreeManipulators
in my patch toDefaultMenuLinkTreeManipulators
in this patch has been completed — including test coverage. Several of the manipulators I had are made obsolete by theMenuLinkTreeStorage
abstraction that this patch introduces.Comment #98
dcrocks CreditAttribution: dcrocks commentedJust some history. hook_translated_menu_link_alter was deleted from D8 a long time ago in favor of hook_menu_link_load. It turned out the latter wasn't being called at the right time to prevent the account menu being made visible to anonymous users, so hook_translated_menu_link_alter was reinstated(#1912806: Regression: 'User Account' displayed on front page for anonymous users). It looks now that both will be gone. Assuming the functionality survives, the only possible impact might be on contrib modules(Devel?). But I think this change will cause them all to be redone in any case.
Comment #99
Wim LeersChanges since #97, again in reverse chronological order:
Next steps:
MenuTreeParameters
(but call itMenuLinkTreeParameters
instead)MenuTreeItem
(but call itMenuTreeElement
instead)I think it'll make more sense to defer the "render into placeholder" stuff to #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees. It's not essential to this patch and might spark bikeshedding, which we better avoid here.
I expect to complete the combining of my patch with this patch tomorrow. I'll answer to #96 after I finish the above. I need to get it done first, because of an upcoming vacation.
Comment #101
Wim Leers#2292025: Convert BreadcrumbBuilderInterface applies() and build() to use RouteMatch changed hunks in both
core/core.services.yml
andcore/modules/menu_link/menu_link.module
, hence the patch won't apply. We'll have to rebase the sandbox. For now, I'll just continue. I've done plenty of rebasing, but I don't know what the best practices are around rebasing in a sandbox.Comment #102
Wim LeersTalked to pwolanin how he prefers rebasing to be done in this sandbox. Now rebased.
Changes since #99, again in reverse chronological order:
I wanted to introduce the
MenuLinkTreeElement
value object. At first, I had a lot of trouble doing so, because all the object model changes are effectively undocumented. After a lot of trial and error and trying to put things together, it started to make sense. Here's a high-level explanation that pwolanin will hopefully confirm is mostly correct, indicate what's wrong, and turn into actual documentation so that reviewers don't have to read all the code to understand the architecture and assumptions.Instead of a menu link knowing where it lives, a menu link is now a separate thing that only knows its "own" (immutable) properties (like
title
,route_name
,menu_name
, and strangely alsoparent
), but not those properties that are determined by the (mutable) placement in the tree (likedepth
andhas_children
).Consequently, we deal with 3 different representations of a link (@pwolanin, feel free to change all terms):
{menu_tree}
MenuTreeStorage::prepareLink()
), but then loses the tree-related metadata, likedepth
andhas_children
MenuLinkManager::createInstance()
)A built menu link tree eventually needs to deal with link instances. But then all the tree-related metadata is lost. This is the purpose of
MenuLinkTreeElement
(and previously the unstructured array I was unhappy with):depth
andhas_children
)inActiveTrail
,access
andoptions
)I've also documented this on
MenuLinkTreeElement
's docblock:Here's a simple microbenchmark with 1000 "menu links" to prove it: http://3v4l.org/XcfvE, see http://3v4l.org/XcfvE#v5411 or http://3v4l.org/XcfvE#v5512 — CPU time is identical (you probably want bigger numbers, even though 1000 is already much more than almost any menu) and memory usage of arrays is *double* that of objects.
Some links to explain that:
So, in this patch, I introduced
MenuLinkTreeElement
, which can theoretically reduce memory usage while staying equally fast. However, becauseMenuTreeStorage
assumes it's working with "menu tree link definitions" and "menu link definitions" (see earlier), and we probably don't want to instantiate menu link objects from withinMenuTreeStorage
(if we'd do that, we'd cause a recursive service dependency, so…), I decided to allowMenuLinkTreeElement::link
to be of the type\Drupal\Core\Menu\MenuLinkInterface|array
. Hence it can be either a menu link definition or a menu link instance.I didn't do any profiling of the actual code, because 1) the above shows it shouldn't matter, 2) once we have render caching of menus, performance considerations in building of menu trees will be pretty much irrelevant, 3)
\Drupal\menu_ui\Tests\MenuTest
takes the same amount of time as before, and that arguably is the best benchmark we have.I also found a fun bug in the Memory cache backend: #2293163: MemoryBackend's cache entries may be modified unintentionally thanks to object references. I had to fix that to get tests to pass.
Comment #104
Wim LeersFixing the sole test failure in #102.
Comment #105
Wim LeersThe steps I went through for this reroll (in chronological order):
TreeStorage
at all, but I showed thatTreeStorage::loadSubTreeData()
andTreeStorage::loadAllChildLinks()
really are just special cases of the same generic loading logic. In both cases, it's just about choosing a different root than the "real" root (which in this patch has the id''
(i.e. empty string), and in HEAD used to have the id0
).buildSubTree()
just a special case ofbuildTree()
.buildSubTree()
calls with a custom set of parameters being passed intobuildTree()
.buildSubTree()
obsolete, so I removed it. Also because it hardcoded the "exclude hidden links" condition, and therefore it was less expressive thanbuildTree()
, leaving it with barely any remaining purpose.buildTree()
not tobuild()
but toload()
. "Loading a tree" makes sense, and that's actually already whatTreeStorage
calls it:TreeStorage::loadTreeData()
.render()
toload()
.load()
, 2)transform()
, 3)build()
.only_active_trail
parameter because it was only around for legacy reasons (see below for more info).MenuLinkTreeParameters
value object. Only this time, it's much clearer (than in my patch). There are very clear helper methods for the typical use cases, to construct the parameters fluent-style.MenuLinkTreeParameters
object with a solid DX, it was time to look atbuild(All|Page)DataTreeParameters()
. Turns out one of them had a chunk of dead code that was used by nothing at all in core (all along in this patch!), leaving only a "max depth" setting that can now be set elegantly and simply enough without needing this additional method contaminating the interface.buildPageDataTreeParameters()
on the other hand, does do some genuinely useful things, that are indeed relatively complex and hence we might want to shield the user from it. So I kept everything (except the setting of max depth, which, again, can be set elegantly and simply enough now) and renamed it to the much clearergetDefaultRenderedMenuTreeLinkParameters()
.And with that, the combining of #2289033: Refactor MenuTree to be simpler and have full unit test coverage with this patch is complete! I intentionally left out the changes to rendering menus, because they can be added in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, and can undergo more scrutiny there, and are non-essential to this patch.
The essential parts that have been added by my work are 1) the concept of menu tree manipulators, 2) much better DX in the form of a clean/simple/understandable
MenuLinkTreeInterface
, value objects instead of unstructured arrays and guiding documentation.Changes since #104, again in reverse chronological order:
Background info for the removal of the
only_active_trail
parameter:From D7's
menu.inc
:Comment #107
pwolanin CreditAttribution: pwolanin commentedre-rolled for conflict with HEAD
In terms of removing the active_trail_only functionality - I feel like we should retain this somehow in case people want to re-implement menu based breadcrumbs - though I guess we can already just load the active trail IDs?
Comment #108
Wim Leers#107: Thanks for the rebase!
Yes, they can just use
MenuActiveTrail::getActiveTrailIds()
(just likeMenuTree::getActiveTrailIds()
in HEAD).I think/fear #2286681: Make public properties on ConfigEntityBase protected might cause this patch to need another rebase.
#96:
Alex and I discussed this further, and I think he had ideas to kind of unify our opposing angles. I think he'll talk to you about that soon, if he hasn't already.
is_callable()
data-drupal-link-systempath
The actual regression is the one that this patch introduces compared to HEAD: the "front page" link is no longer marked as active.
I think we're just misunderstanding each other.
Menu
config entities andMenuLink
content entities. It doesn't make sense to have a cache tag for each menu link, because a menu link is not an expensive thing that is rendered on its own, but is always rendered (and manipulated) in the context of aMenu
. Hence it made sense to discardMenuLink
's own cache tags entirely, and just always make them refer to the associatedMenu
entity's cache tags (i.e.MenuLink::getCacheTag()
can be considered a proxy ofMenu::getCacheTag()
, for the associated menu).Looking at this patch, that has been lost:
MenuLinkContent
get their own cache tags, instead of referring to that of the associatedMenu
entity. That's an easy fix; you can migrate the code 1:1 from HEAD'sMenuLink
entity :)MenuCacheTagsTest
— which tested all this — thankfully still exists, but has been changed in ways that result in decreased coverage. Instead of creating aMenuLink
entity (MenuLinkContent
in this patch), then modifying it and deleting it), it now does those things with a static menu link (MenuLinkDefault
), and then just creates aMenuLinkContent
entity. So the basics are still there, but now that there are multiple menu link plugin types, we need to apply the original test coverage to menu links of each each menu link plugin types.That's the most important part.
Menu::getCacheTag()
. Even though it yields correct results.buildSubTree()
, because after the incremental improvements that I made (which you can see in the sandbox' commit history, plus a full explanation of the rationale in #105), there seemed to be little to no value left in keeping it. I hope you're on the same page after reading #105/looking at the incremental steps.transform()
method is a little bit strange. But that's only because we pulled it out frombuild(Tree)()
like I had it in my patch. And the main reason we pulled it out there, is because we have also hadbuildSubTree()
. Now that I've finished the combining of both patches in #105, and especially now that we have aMenuLinkTreeParameters
value object, I think you might see the appeal to put the menu link tree manipulators inMenuLinkTreeParameters
as well, and letload()
(previouslybuildTree()
) apply the menu link tree manipulators?::transform()
and the associated DX (an array with menu link tree manipulator callables) is by far the worst remaining part ofMenuLinkTree
. If you have any idea on how to make the DX better, but still retain the overall concept, I'm totally open to that.::load()
automatically call::transform()
to apply themenu.default_tree_manipulators:checkAccess
menu link tree manipulator.::build()
automatically call::transform()
to apply themenu.default_tree_manipulators:generateIndexAndSort
menu link tree manipulator.::transform()
is limited to only the advanced use cases.For example, this:
could change to:
And if this weren't an advanced use case (i.e. if no additional menu link tree manipulator had to be applied), it'd have been changed to:
Downsides:
::load()
to apply a different access checking manipulator.generateIndexAndSort
menu link tree manipulator would always be called prior to rendering; making it impossible to render trees in a different order. But in the extremely rare case that this is needed, the developer could just override theMenuLinkTree
service and override this aspect, and keep everything else.Having written this, the downsides don't look too bad. If you like this, I'll get this done.
While working on #105, I also found another bug (on top of the broken "active" marking of the front page, due to missing the value for the
data-drupal-link-systempath
attribute): menu links defined in YML files can be repositioned in the tree just fine. But whenever caches are cleared, the original position is restored!Comment #109
pwolanin CreditAttribution: pwolanin commented@Wim Leers - most of this sounds fine. I need to look more at how you implemented the subtree handling - as long as we can load a tree with relative depth to the selected root, it's not really a big concern.
As far as cache tags, I'll emphasize again, that the menu links API is not based on Menu entities except that we use to make the expected list and human-readable names. But the API doesn't restrict in any way to using menu_name values that correspond to any actual Menu entity. So the correct cache tags need to be the set found in the links in any given menu. This will always be a single, unique one. But again, may bear no relation to any Menu entity, so it's incorrect to involve the Menu entity in finding the cache tags.
Comment #110
pwolanin CreditAttribution: pwolanin commentedre-rolled patch for HEAD conflict. Going to make some mall enhancements to #2293163: MemoryBackend's cache entries may be modified unintentionally thanks to object references
Assuming that goes in promptly, we can take it out of this patch.
Comment #111
pwolanin CreditAttribution: pwolanin commentedre-roll for #2293163: MemoryBackend's cache entries may be modified unintentionally thanks to object references and other HEAD conflicts
then, renamed class MenuLinkTreeElement to MenuTreeElement, since MenuTreeStorage should be generic (e.g. we should try to re-use it for book module) and not specific to menu links.
Comment #113
pwolanin CreditAttribution: pwolanin commentedneeded to be updated for #1875996: Reconsider naming conventions for derivative classes
Comment #114
pwolanin CreditAttribution: pwolanin commentedSo, after further thought, reverted the class name change in #111. Instead, I moved all use of that class out of the MenuTreeStorage so tree element instances are only created in MenuLinkTree. This keeps the tree storage more generic and maybe a little simpler to re-implement.
In a similar vein, renamed class MenuLinkTreeParameters to MenuTreeParameters and added some @todos about making it have a full set of methods and an interface.
These changes are pretty much internal to the interaction between MenuLinkTree and MenuTreeStorage, so not really important to the big picture for anyone who has been reviewing.
Comment #115
pwolanin CreditAttribution: pwolanin commentedSome more fixes: use RouteMatchInterface in MenuActiveTrail. Also did away with checking for the _exception_statuscode attribute on the request because of it - I think it only makes sense if some information like that is on the RouteMatch
Also fixes SystemManager to actually limit to the admin menu (I confused myself after adding a link to /admin in the main menu the /admin page was blank - clearly a bug), and fixes the @todo about ordering in the query to load by route.
Added some more code comments too.
A new PR for visual review has been created: https://github.com/pwolanin/drupal/pull/2
Comment #117
larowlanRe-roll after #2290129: Menu/routing topic needs an overhaul
Comment #118
larowlanFirstly - some coding standards violations for new files:
<3 phpcs and coder.
Reveals:
Comment #119
larowlanbtw, happy to fix those coding standards violations if you set me up with sandbox access so I can push
Comment #121
larowlanShould fix the fails from #116
Now that MenuTreeStorage is responsible for sorting links, and the ksort has been removed, one of the test cases in the provider is redundant.
Also, needs to use RouteMatch, not RequestStack
All becomes clear in interdiff.
Comment #122
pwolanin CreditAttribution: pwolanin commentedSome of the code style warnings are bogus.
I think we are now using inline @var docs for type documentation like:
Though it seems some can be removed due to the conversion to use
\Drupal\Core\Menu\MenuLinkTreeElement
The 83 char lines are @param lines like:
It also seems unhappy about this, though I think there's no valid way to make it shorter?
Made a new PR due to the re-roll (github apparently doesn't understand how to handle that): https://github.com/pwolanin/drupal/pull/3
Comment #123
pwolanin CreditAttribution: pwolanin commentedsome more little cleanups:
remove build() method from MenuLinkInterface since it's basically unused and redundant. Wim and I discussed this last week, but hadn't gotten to it.
adds a getExpanded() method to MenuLinkTreeInterface and implements it as a simple proxy to the MenuTreeStorage since otherwise it's impossible for a developer to get the list of expanded links like is done inside the method to get the default parameters.
renames the expanded property to expandedParents on the parameters class and tries to clarify the docs a bit.
Comment #124
RainbowArrayI took a look through this with pwolanin today to see if the way the new menu system works would be compatible with what I was working on in #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables. From what I can see, yes it is, and in a much easier way than before. The parameters for loading a menu tree seem to work much more sensibly than the previous system. This weekend I'll try to set up a proof of concept patch to get #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables working when this issue's most current patch is applied. I think it should work, but it will be good to try.
Nice work here.
Comment #125
effulgentsia CreditAttribution: effulgentsia commentedA lot of code has changed here since #73, so here's updated benchmarks. First of all, attached is an updated tester module (see #19).
HEAD: 163ms (4ms slower than it was at #73 (2 weeks ago), sigh)
Patch (#122): 203ms
Patch + the hack in #2289319-2: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache: 179ms
In #73, patch+hack was same as HEAD, now it's 10% slower. Probably due to the merging of Wim's work (better code sometimes has a perf cost). Would be good to know what specifically is responsible for that to make an informed decision on whether to accept it or try to optimize.
Comment #126
larowlanManual testing:
Looks great so far
Comment #127
pwolanin CreditAttribution: pwolanin commented@effulgentsia - yes we are doing things like instantiating more of the plugins on the assumption that the later render-cache work will give us quite an added performance boost. There are also some calls to the controller resolver we could potentially avoid by just building the callable, but I'm doubtful that would be a significant difference.
As before, I don't think storing the system path is the correct answer.
Comment #128
pwolanin CreditAttribution: pwolanin commentedhad to re-roll for #1987424: seven.theme - Convert theme_ functions to Twig Would love some help watching the RTBC queue to avoid conflicts and bad changes like that.
Comment #129
pwolanin CreditAttribution: pwolanin commentedOk, spent a while flailing on this but turns out that the implementation of MyAccountMenuLink didn't actually allow you to toggle the link off. This was exposed by the changed test in #2285381: Bad xpath in UserAccountLinksTests::testDisabledAccountLink() once I'd fixed the changed CSS class name.
Comment #131
pwolanin CreditAttribution: pwolanin commentedStupid use statement conflict.
Comment #132
joelpittet@pwolanin re #128 If I knew what was bad about that change, I'd have not done it to start. Could you explain?
Comment #133
pwolanin CreditAttribution: pwolanin commented@joelpittet - it's not as bad as I thought initially. However, I think constructing the whole A tag in the template is a little strange.
I'm also just worried I missed something in the re-roll in terms of Wim's plans for render caching.
Comment #134
joelpittet@pwolanin ok fewf.
This patch is green and looking good. Attached is just 4 space to 2 space for the twig template and removal of the unnecessary class key check & init as empty array.
Comment #135
joelpittetjebus getting late...
Oh just a note not probably not this patch but it would be nice if that url object was just __toString() instead of ->toString() method
Comment #136
pwolanin CreditAttribution: pwolanin commented@joelpittet - thanks for that formatting fix. I'll add the incremental change to the sandbox.
There was some debate about toString() vs __toString() - we'll have to search for the issue (wasn't my patch), I agree it's not that intuitive.
Comment #137
effulgentsia CreditAttribution: effulgentsia commentedTo aid reviewers, I added a section to the issue summary that details where various code from the former menu_link module is moved to by this patch.
Comment #138
pwolanin CreditAttribution: pwolanin commentedUpdated the summary a bit.
Per discussion w/ effulgentsia , I'm working on removing the discovered field from the definition - it's really internal to the tree storage.
renaming hook_menu_links_alter() to hook_menu_links_discovered_alter() to make it more clear what's being passed in.
Comment #139
effulgentsia CreditAttribution: effulgentsia commentedComment #140
pwolanin CreditAttribution: pwolanin commentedremove discovered from definition, kill active_menus_default config, rename alter hook
Comment #141
effulgentsia CreditAttribution: effulgentsia commentedAdded menu.inc function removals to the issue summary's patch map.
Comment #142
effulgentsia CreditAttribution: effulgentsia commentedThis patch is a subset of #140. It doesn't contain the \Drupal\Core\Menu additions or their unit tests, the menu_link module removal or the menu_link_content module addition, and I also stripped out the menu.inc function removals documented in the issue summary. Basically, what's left is what the impact of the patch is to the rest of Drupal. This is what I plan to do a detailed dreditor review of first, so uploading the patch in the hopes it's small enough for dreditor to not choke on.
Other reviewers: please feel free to review this portion as well, the full #140 patch, or a subset of your choice.
Comment #143
pwolanin CreditAttribution: pwolanin commentedafter more discussion w/ @effulgentsia, removing function menu_link_rebuild_defaults() in favor of direct calls to the plugin manager rebuild() method. There is only one except in tests.
Also replaces calls to \Drupal::service('menu.link_tree') with \Drupal::menuTree() to get better type hinting.
For clarity, rename $tree_cache_backend to $menu_cache_backend in the MenuTreeStorage
updated PR since the last time I had to rebase: https://github.com/pwolanin/drupal/pull/4
Comment #144
larowlanI've reviewed this again manually and the code.
In this code-review I focused on the external-facing API.
Screencast of manual review here: http://youtu.be/LxnKzQharK8
Plus some screenshots of views integration which I missed in the screencast
We have some of the highest phpunit coverage in core/Menu with this patch
Code review comments (copied from https://github.com/pwolanin/drupal/pull/4):
So with the exception of 6 and 7, which need a committer's input, I think most of this can be done in follows/quick edits.
I'm going to be bold here and say this is RTBC - but we need a draft change notice and a list of change notices that will require updating first in addition to a call on LinksTest and MenuRouterRebuildTest.
in other words, once we have a change-notice draft and word on LinksTest and MenuRouterRebuildTest:
Comment #145
larowlanEmbedding the screencast for those who don't want to leave the comfort of d.o
Comment #146
effulgentsia CreditAttribution: effulgentsia commentedIf someone is willing to RTBC this patch more or less as-is, and a committer is willing to commit it, despite the various problems that would get surfaced with detailed reviews, I won't stand in the way of that.
However, I have my doubts on a committer stepping up to do that. So, I took a stab at splitting the patch up into 5 steps that can be reviewed and committed in sequence without introducing any functional regressions (mostly, because the first 3 steps just add code without changing anything to use it, and the last step just deletes code, so only step 4 is the magic "convert all the things" step). See this issue's "child issues" block for the links.
Comment #147
Wim Leers#115: good catch in
SystemManager
not specifying the menu for which to retrieve the active trail. I indeed missed that one.#123: +1 for removing
build()
#124: Yes! :) This now incorporates much of the refactoring of #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, and there I took it specifically into account to make #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables trivial — i.e. to provide a sane API to do get any kind of menu tree you could possibly want :)
#125: Thanks for the profiling!
As of #146, this issue is now effectively split up into multiple child issues, to make it more reviewable/committable. It's mentioned in #146, but here are the links inline for your convenience:
Comment #148
heddnRe-titling to make it clear this is only a meta at this point.
Comment #149
plachSorry, guys, did I dream it or at some point in the sandbox theMenuLink
entity used to have a data table? Without it we cannot translate (content) menu link titles/descriptions.Sorry, that was the
MenuLinkContent
entity type :)Comment #150
pwolanin CreditAttribution: pwolanin commentedThis handbook page has been partly updated, but the image needs to be revised, and I would appreciate help fleshing out the node type menu link example.
https://www.drupal.org/node/2118147
Comment #151
xjmI've moved the beta blocker tags to the child issues.
Comment #152
Dries CreditAttribution: Dries commentedJust committed the last child issue so marking this beta blocker fixed! :)
Comment #153
Gábor HojtsyRe #150, I updated the image on https://www.drupal.org/node/2118147 as requested. The image source is in this Google Drawing: https://docs.google.com/drawings/d/1oiHWy9ERY3ySI8uulQtSxbGEsdlmmrN8ojv8... -- shared with the world, so if for some reason I disappear, you can copy and adjust from there if nothing better.
Comment #154
xjmI added a link to the Google Drawing in the doc itself so anyone who wants to update it can find that easily to copy from.