Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The book navigation markup looks like the menu link tree. Get the two inline with each other.
Proposed resolution
- Remove the theme function and make book tree align with MenuLinkTree nnnnn/.
- Example of a conversion to a #type: #1938916: Convert locale theme tables to table #type
Remaining tasks
Create follow up issue to reuse the macro. (done: #2490308: Refactor the macro on menu.html.twig for reusability)
User interface changes
None
API changes
- theme_book_link()
will be gone (part of theme()
to Twig)
Beta phase evaluation
Issue category | Task because all theme() functions should be replaced with Twig template files. |
---|---|
Issue priority | Not critical because without this core will still work, but it's an improvement to the TX. |
Comment | File | Size | Author |
---|---|---|---|
#51 | interdiff.txt | 3.41 KB | lauriii |
#51 | remove_theme_book_link-2443361-51.patch | 9.89 KB | lauriii |
#43 | interdiff-40-43.txt | 1.36 KB | star-szr |
#43 | remove_theme_book_link-2443361-43.patch | 12.25 KB | star-szr |
#42 | interdiff.txt | 1.36 KB | star-szr |
Comments
Comment #1
Manuel Garcia CreditAttribution: Manuel Garcia commentedHere is a straight removal patch. Let's see what the bot says...
Comment #2
Manuel Garcia CreditAttribution: Manuel Garcia commentedComment #3
Manuel Garcia CreditAttribution: Manuel Garcia commentedComment #5
littleindian CreditAttribution: littleindian commentedComment #6
JeroenT@ashwinikumar, are you still working on this?
Comment #7
JeroenTComment #8
sqndr CreditAttribution: sqndr as a volunteer commentedComment #9
sqndr CreditAttribution: sqndr as a volunteer commentedComment #10
joelpittetTrying to get this closer to the MenuLinkTree.php (#pairprogramming with @sqndr)
Comment #11
joelpittetComment #12
sqndr CreditAttribution: sqndr as a volunteer commentedNice pair programming with joelpittet!
Comment #14
star-szrThis looks really good, nice work you two!
Oops :)
Comment #15
joelpittet@Cottser thanks for spotting that so quickly!
Comment #16
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedLoving this guys! I really like where this is going.
Why are we adding a twig macro here? Do you see this being used elsewhere besides book itself? Perhaps the idea to later on move this to a general macro? We could have this render all trees of links on book, menu etc.
Comment #17
joelpittet@Manuel Garcia Because it's in context and easy to change (mostly because of the class in classy) We'd like to abstract it out a bit but I think that's game for a follow-up for sure! @sqndr was thinking that yesterday too.
Would you mind opening a follow up for that @Manuel Garcia?
Comment #18
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedThanks @joelpittet for explaining the situation on IRC
Here is the followup (although I don't think its really a follow up, we should do this no matter what imho): #2490308: Refactor the macro on menu.html.twig for reusability
Comment #19
sqndr CreditAttribution: sqndr as a volunteer commentedThanks for creating the follow up @Manuel! Better to keep issue small in my opinion.
@joelpittet: High five for the pair programming once more! ;)
Comment #20
sqndr CreditAttribution: sqndr as a volunteer commentedAdded an API change. We still need to
- add a beta phase evaluation
Comment #21
sqndr CreditAttribution: sqndr as a volunteer commentedHm, didn't tim.plunkett had a comment about this at the sprints? Shouldn't we refactor the rest of the code in that function to the short syntax
[]
(instead of the longarray()
) as well?Comment #22
sqndr CreditAttribution: sqndr as a volunteer commentedChanged to the short syntax to keep the code consistent.
Comment #23
sqndr CreditAttribution: sqndr as a volunteer commentedTrying to update the beta phase evaluation.
Comment #24
Wim Leers#2483181: Make book navigation block cacheable is blocked on this, tagging as blocker, and bumping to major because of that.
I don't think the
$level = 0
should be part of the interface?Look at how
public MenuLinkTree::build()
andprotected MenuLinkTree::buildItems()
work together to have cleaner code, and avoid polluting the public API with internal details.Interestingly, this template is now functionally identical to
menu.html.twig
…Couldn't we just choose to import that template and call it a day?
That still allows themes to have completely different markup for books versus menus, but prevents the need for duplication.
AFAICT this is the only difference. Can't we reorganize things to avoid duplicating everything?
Comment #25
star-szrFor 2 and 3: #2490308: Refactor the macro on menu.html.twig for reusability
Comment #26
Wim LeersYou keep linking to awesome issues, @Cottser :D Thanks!
Comment #27
joelpittet@Wim Leers we copied MenuLinkTree::build() almost verbatim when we did this... it had $level in there. That was likely there because before that data was used to build up first and last classes I believe.
We can take that out though now as it looks like that method has changed since a couple weeks ago and we don't need the $level because it's in the macro.
Comment #28
Wim Leers#27: no,
$level
is definitely being added in this patch.Comment #29
joelpittet@sqndr Yes Tim was saying he wanted to keep it consistent but we avoid making changes outside of the diffs that we are already making so that it's easy to review the changes that are relevant to the patch and avoid conflicts with other patches that may be touching that part of the code (less unnessasary re-rolls).
I'm trying to convert the syntax bit by bit, the inconsistency is not a big concern. More discussion here #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8.
@Wim Leers I'm telling the truth. Hash 8e866ef
Comment #30
joelpittetThat change was introduce by one of the issues you worked on: #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees
Comment #31
joelpittetI think I got the gist of the changes minus the cachable meta data stuff as that is out of scope on this issue.
Comment #32
joelpittetWhoops left a level in there.
Comment #34
hass CreditAttribution: hass commentedA mix of short array vs old syntax should't be done.
Comment #35
joelpittet@hass it is done... everywhere in core. Read and contribute to the policy #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8
Comment #37
joelpittetBut you're right I shouldn't mix in the same patch (within the hunks i'm changing). So thanks @hass (though not sure you meant that?) that triggered me to find it.
Also had a double period typo.
Comment #39
Wim Leers#29: oh, yes, sure, it used to be in
MenuLinkTree
. But this patch doesn't touch that. This patch touches the equivalent Book module code. And it indeed got removed in that patch that I worked on, precisely for the reasons I mentioned: it wasn't on the interface, so it never should've been there. (And I was also the one to introduce it IIRC, and that was wrong.) I'm only asking to 1) not make the same mistake here, 2) adopt the same pattern that HEAD'sMenuLinkTree
has :)#31: splendid!
This is c/p'ed from
MenuLinkTree
, but this code doesn't actually throw this exception. Can be deleted :)IMO once this patch is green, it's ready to go. Great clean-up.
Perhaps an
@see \Drupal\Core\Menu\MenuLinkTree::build()
is useful, to make the reader aware of the similarity?Comment #40
joelpittet@Wim Leers yeah I was just mentioning we tried copying, it wasn't introducing anything new explicitly. The "end goal" would be to maybe extend from MenuLinkTree and avoid the duplication all together... but baby steps, we'll get there.
Local tests weren't happening this morning... so I just guess and cleaned up the items from #39
Comment #41
Wim LeersLooks great!
Comment #42
star-szrJust found a couple docs fixes.
Minor: Classy templates shouldn't say "Default theme implementation" or say "@ingroup themeable" because they are overrides.
Missing period.
That's all I could catch, so fixing and leaving at RTBC.
Comment #43
star-szrSorry, can't resist this fix (url → URL). Manually edited interdiff is from #40.
Comment #44
hass CreditAttribution: hass commentedYes, i mean inconsistency in one function. I know core is mixed, but it is consistent per file or function.
Comment #45
joelpittet@hass thank you for clarifying, it's hard to know sometimes without a reference context lines.
Thanks for the doc cleanup @Cottser!
Comment #46
sqndr CreditAttribution: sqndr as a volunteer commentedNice work! :)
Comment #47
hass CreditAttribution: hass commentedThis is still a mix of old and new array style and this in just one array. We should not do this I think.
Comment #48
sqndr CreditAttribution: sqndr as a volunteer commentedI asked the same question in #21. See the answer in #29. And again in #35.
Comment #49
joelpittetFeel free to change it if you feel strongly about it. I'll not block this from being committed on that.
Though I'd rather it change.
Comment #50
alexpottYep mixing array styles within an array is not nice - this should only be using
array()
I'm not sure about the new method name of
BookManager::build()
- how aboutbuildTree
since it is generating something using a book tree template. In fact why rename it? It does not look necessary. And not renaming it would make this patch really only change the theme layer.Comment #51
lauriiiComment #52
joelpittet@lauriii and @alexpott the name was chosen so that we can potentially merge the classes between MenuLinkTree class which has nearly identical logic. Set BookManager to extend MenuLinkTree or something later.
Comment #53
alexpott@joelpittet but if we do that at some point in the future, we can do it using a bc layer.
Comment #54
joelpittetSure, why not. I just wanted to keep them close inline with each other as possible. BC layers just extra crud laying about to me.
Just want to unblock and move forward.
Comment #55
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commented+1
Comment #56
sqndr CreditAttribution: sqndr as a volunteer commentedNice work! Hopefully we can get this in now!
Comment #57
alexpottCommitted 24bd910 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #59
joelpittetThanks, unlocked and unblocked caching level!
Comment #60
hass CreditAttribution: hass commentedFollowup.