Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Problem/Motivation
The longrunning goal is to pretty much move any logic from core into menu_link as it is pointless to use it without actual available menu links.
Proposed resolution
Move all tree related functions into a MenuTree service on the menu_link module. We could iterate on additional methods to provide interfaces etc. if we can agree what we actually need at the end.
Remaining tasks
None, it has been already committed.
User interface changes
None.
API changes
See the change record.
Comment | File | Size | Author |
---|---|---|---|
#54 | interdiff.txt | 6.24 KB | jessebeach |
#54 | menu-tree-2207893-54.patch | 88.39 KB | jessebeach |
#51 | menu-tree-2207893-50.patch | 88.38 KB | jessebeach |
#51 | interdiff.txt | 24 KB | jessebeach |
#47 | menu_tree-2207893-47.patch | 88.38 KB | dawehner |
Comments
Comment #1
dawehnerBlocked on #2107533: Remove {menu_router}
Comment #2
dawehnerFirst version waiting until #2178697: Move code for finding/saving default menu links from menu.inc to menu_link module is in.
Comment #5
dawehnerRemoved a couple of old tests and replaced it with proper unit testing.
Comment #7
tim.plunkettUgh, running phpunit via simpletest sucksssssss
Drupal\menu_link\Tests\MenuTreeTest must implement \Drupal\Tests\UnitTestCase::getInfo().
It runs fine from the CLI of course.
Comment #8
dawehnerGo along, there is no helpful information here.
Comment #9
xjmNeat. We should probably try to get this in by beta; I'm not sure if this is a required part of #2177035: [Meta] Menu system home-stretch or not? Also, nice summary... ;)
Comment #10
xjmErg. Stupid d.o bug...
Comment #11
dawehnerWell, better than nothing.
Comment #12
pwolanin CreditAttribution: pwolanin commentedThis is more-or-less what I'm doing for book module. However, I'm a little unclear if this will actually play nicely with menu link entities?
Possibly the menu link entity itself should only know about plid and maybe has_children?
The hierarchy stuff could be in the same table or a different table, but it could be updated acting on an entity save, rather than being part of the entity itself.
Comment #14
dawehner@pwolanin
I kinda think that all these tasks can be done on top of the patch later. This just provides a solid foundation to iterate on top of it.
Comment #15
amateescu CreditAttribution: amateescu commentedThis patch will impact #1842858: [PP-1] Convert menu links to the new Entity Field API quite a bit and since it's still postponed on other issues, I'm going to add this one to the list.
Comment #16
dawehnerNo idea what happened, just reuploading the last patch.
Comment #18
dawehnerThis was probably easier than expected.
Comment #19
jibranwe should ads @deprecated in the doc block.
QueryFactoryInterface I think.
more then 80 chars.
@param block missing.
Type hint missing.
@var doc block missing.
doc block missing.
I like this function.
Comment #20
dawehnerHere are some of the docs, more will come later
Comment #21
dawehnerLet's just replace the few instances which are left. This aren't that often used APIs so let's clean up menu.inc already.
Yeah I ran into that trap as well. We have two level of factories here: The Entity\query\QueryFactory takes care about
getting the query factory for a specific storage type (config, sql) and asking that specific query factory (which has the queryfactoryInterface) for an instance. The QueryFactory itself should not be swappable.
The reason for this was that the dependencies for the query is just known per storage type (for example the database connection).
Fixed all the instances.
Good catch, fixed all of those.
Well, it is how it is. We can extend the test coverage later
Comment #23
dawehnerThis should be it.
Comment #24
dawehner23: menu_tree-2207893-23.patch queued for re-testing.
Comment #25
andypostawesome clean-up, maybe better to decouple storage logic from the service and then name it as
MenuTreeBuilder
is this a service?
Comment #26
dawehnerYes it is.
Comment #27
sunBogus indentation.
Hm. Two concerns:
1. The "output" (render array) generation does not really belong to the responsibilities of a menu tree class — I think we should separate the concerns of menu tree building vs. rendering.
2. output() is a slightly strange method name, because nothing is actually output. ;) How about renaming these methods to renderMenu($menu_name) and renderTree($tree)?
Ugh, we're still hard-coding Node Access...?
Not a fault of this nice and innocent patch and thus OT here. But I hope that this refactoring will allow us to inject an Entity Access Controller later on. :)
And once more...
Do we really need to add those comments everywhere? (I personally find them pretty disturbing) :-/
Anyway, not to be discussed here, we just need to fix the faulty indentations throughout the patch.
Comment #28
dawehnerRight, probably caused by a copy and paste error. In general though I really prefer using that in our existing procedural code. For example this allows really easy refactoring, as the IDE has enough context
available.
Sounds like a great idea. What about doing that in a follow up?
Comment #29
pwolanin CreditAttribution: pwolanin commentedThis patch incorporates the bug fix at #2206235: Optimized access check for node menu links is broken.. Also tries to make all the access munging more internal to the manager.
Still has at least 1 test fail, but I need to run out for a bit.
Comment #31
pwolanin CreditAttribution: pwolanin commentedOk, hopefully this bring it down to 1 fail.
I think the test may be broken - or the menu code is broken in terms of handling unpublished nodes with an access query.
In Drupal 7, links to unpublished nodes never appeared. A little weird (maybe a bug) - so Drupal 8 is doing the same now that the optimizaiton is fixed.
Comment #33
dawehnerRerolled. I hope this also fixes all the test failures.
Comment #34
sunThanks! This is most certainly a big step in the right direction.
Comment #35
pwolanin CreditAttribution: pwolanin commentedSo, discussed with catch in IRC, I think we should just rip out the node access optimization here (but leave it for book which was always the more importatnt use of it).
I also don't think the logic change in the interdiff is actually right.
Comment #36
pwolanin CreditAttribution: pwolanin commentedThis simplifies the logic quite a bit.
The public method names are still all too similar and hard to understand, but that's a bit hard to unwind at this point.
Comment #37
pwolanin CreditAttribution: pwolanin commentedIt's kind of sad that we are wrapping _menu_link_translate.
Really feels like that should be build into the menu link entity?
Comment #38
dawehnerOh great, even better if we don't need it anymore.
Opened a follow up for the _menu_link_translate bit: #2219073: Figure out a good place for _menu_link_translate
Comment #40
dawehner36: menu_tree-2207893-36.patch queued for re-testing.
Comment #41
andypostback to rtbc
Comment #42
catchOnly small issues, overall this looks good and it'll make further cleanup simpler.
menu menu tree?
Should this be menuTreeData? It's not clear how it's different from menuTree from the comment.
No need for isset($cache->data) if $cache is not FALSE.
Just code moving around but this is no longer true at all. Wonder whether some of this logic is redundant now and whether we're testing it anywhere - not for this issue though.
Nice to see this gone and the unit test added :)
Comment #43
dawehnerYeah one of them contains the tree without the hidden items and the other one contains them.
Sorry for that dejavu moment.
Yeah unit tests just makes it much easier to test the edge cases.
Comment #44
jessebeach CreditAttribution: jessebeach commentedThis is a beta blocker, not a target, since it blocks other blockers.
Comment #45
catchAlso critical then.
Comment #46
BoobaaChasing HEAD: the patch didn't apply, so I rerolled it. Three small nitpicks.
Drupal\menu_link\MenuTree
had aprotected $language<strong>r</strong>Manager
(with a typo in it); I fixed this to$languageManager
(as it is the thing being used later on).Drupal\menu_link\MenuTree::buildPageData()
may use$page_not_403
uninitialized. I don't know if this is acceptable or not.Drupal\menu_link\MenuTree::renderTree()
may use$data
as a leftover from a previous foreach cycle. I don't know if this is by design or not.Comment #47
dawehnerGlad you found the issue!
This will never happens. $system_path will just exist if $page_not_403 is also set.
Yeah we just want to ues the menu name, so this leaking is always expected and will always happen. Nevertheless we can and should introduce a proper variable instead here.
Comment #48
BoobaaPatch still applies (with some offsets, but anyway); tested by hand and all looks OK to me.
Comment #49
catch47: menu_tree-2207893-47.patch queued for re-testing.
Comment #51
jessebeach CreditAttribution: jessebeach commentedIn #1194136: Re-organise core cache bins, the bin for menu cacheing changed from
cache.menu
tocache.data
. Since we're injecting the cache backend to the MenuTree class, I've updated themenu_link.services.yml
to inject the@cache.data
service instead of the@cache.menu
service.Otherwise, the cache bin changes caused a big reject in menu.inc; that was just deleting functions.
Both changes are represented in the interdiff.
I'll get started on the change notice.
Comment #52
dawehnerThanks for the rerolls! I was actually quite sure that we would have already added one here, but I can't fit on https://drupal.org/list-changes/review
so this is probably not the case.
Comment #53
dawehnerJust fixed the
<?php>
tags, but beside form it, perfect!Comment #54
jessebeach CreditAttribution: jessebeach commentedJust nit-picky comment code standard stuff. We had a few lines over 80 characters.
Comment #55
jessebeach CreditAttribution: jessebeach commentedI didn't change any code. Back to RTBC when it comes back green.
Comment #56
BoobaaBot said green, so it's RTBC (again). Interdiff is fine this time. ;)
Comment #57
dawehner+1
Comment #59
catchThanks!
Comment #60
jessebeach CreditAttribution: jessebeach commentedPublished the change notice: https://drupal.org/node/2226481
Comment #61
BoobaaComment #62
jessebeach CreditAttribution: jessebeach commentedRemoved 'Needs change record'.