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.
Issue #1898464 by jenlampton, Cottser, joelpittet, steveoliver, Dustin Currie, shanethehat, cafuego, mr.baileys: toolbar.module - Convert theme_ functions to Twig.
Task
Use Twig instead of PHPTemplate
Remaining
Patch needs review
Theme function name/template path | Conversion status |
---|---|
theme_toolbar | converted to Twig template: toolbar.html.twig |
theme_toolbar_item | theme function removed, pre-render (toolbar_pre_render_item() ) kept |
theme_toolbar_tab_wrapper | removed and made part of toolbar template |
theme_toolbar_tray_heading_wrapper | Removed and refactored - it was just an <h3> prefix |
theme_toolbar_tray_wrapper | removed and made part of toolbar template |
Consolidation ideas (all completed here for performance reasons):
- Let's take the twig code from toolbar_tray_heading_wrapper and put it inside toolbar_tray_wrapper with some logic instead.
- Would be nice if we could figure out how to get rid of toolbar-item.html.twig since it doesn't do anything. (do we need it?)
- Maybe we can do the same with toolbar_tab_wrapper, too :)
Related
#1757550: [Meta] Convert core theme functions to Twig templates
Review Bonus
See #2094585: [policy, no patch] Core review bonus
#190466: URL filter fails on square brackets +2
Points: 2
Comment | File | Size | Author |
---|---|---|---|
#108 | dev_toolbar-update_render_structure.patch | 510 bytes | star-szr |
#108 | interdiff.txt | 3.35 KB | star-szr |
#108 | 1898464-108.patch | 10.54 KB | star-szr |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
steveoliver CreditAttribution: steveoliver commentedComment #3
steveoliver CreditAttribution: steveoliver commentedComment #4
steveoliver CreditAttribution: steveoliver commentedAttached patch addresses the following:
Need newlines at end of files.
Needs @see & @ingroup.
Comment #5
jenlamptonrenamed
wrapper_attributes
toattributes
for consistency with other templates.renamed
heading_label
tolabel
for clarityI really hate looking at the tangled code in this module, it makes me want to kill all theme wrappers, grr... but aside from that, this conversion looks great! :)
Follow-up issue ideas:
- Let's take the twig code from toolbar_tray_heading_wrapper and put it inside toolbar_tray_wrapper with some logic instead.
- Would be nice if we could figure out how to get rid of toolbar-item.html.twig since it doesn't do anything. (do we need it?)
- Maybe we can do the same with toolbar_tab_wrapper, too :)
Comment #7
Dustin Currie CreditAttribution: Dustin Currie commented#5: core-update_toolbar_to_twig-1898464-5.patch queued for re-testing.
Comment #8
steveoliver CreditAttribution: steveoliver commentedJen #5, I think your first item makes sense, but 2 and 3:
I think we want to keep these for hook_toolbar.
Comment #9
steveoliver CreditAttribution: steveoliver commentedAlso unassigning. I think Dustin will be taking this over.
Comment #10
Dustin Currie CreditAttribution: Dustin Currie commentedRemoved toolbar_tray_heading_wrapper per idea 1 in #5
Agree with Steve on keeping tabs and trays and even items as separate templates.
Comment #11
Dustin Currie CreditAttribution: Dustin Currie commentedComment #13
Dustin Currie CreditAttribution: Dustin Currie commentedComment #14
joelpittetRunning testbot again.
Can you add a newline to the end of that one file for git's sake?
Also could pull out attributes.class like this:
@see http://drupal.org/node/1823416
Comment #16
joelpittet@Dustin Currie scratch that attributes change, it's a moving target and a little discussion on irc seemed to land that the class will be pulled out for bartik I believe due to it's audience. All the rest may be clean like that one...
This looks close! Just that new line and the testbot error in EntityTranslationUITest which I haven't looked into yet.
Comment #17
joelpittet@Dustin Currie It passed locally for me, I just cleaned up a few things in the comments and light formatting and we'll run this sucker again. There may be some more nitpicky things to look at in the preprocess docblocks as per http://drupal.org/node/1913208
Comment #18
hass CreditAttribution: hass commentedPlease also look into #1944572: Remove "ul.menu" dependency to prevent theme clashes.
Aside - are we really removing phptemplate now or do we keep both? I heard rumours about this and like to know for my themes...
Comment #19
joelpittet@hass I don't think #1944572: Remove "ul.menu" dependency to prevent theme clashes will have any effect on this conversion issue but thanks for sharing nontheless because that sounds like a good thing to see happen! Also, yes we are really removing phptemplate now:)
Comment #20
hass CreditAttribution: hass commented@joelpittet: Thanks for this details. I think it's very bad that phptemplate get's removed, so I'm very unhappy about this. This is not positive and nothing to smile about.
Comment #21
joelpittet@hass PHPTemplate may end up in contrib... but you should hear some of the reasons why twig was chosen:
http://www.youtube.com/watch?v=QGIqu_Te0PA
Comment #22
nikkubhai CreditAttribution: nikkubhai commentedI tested the patch using simpletest and everything worked fine. I didn't notice any differences during manual testing.
Before:
After:
Comment #22.0
nikkubhai CreditAttribution: nikkubhai commentedadded idears
Comment #23
star-szrThis is looking good, thanks everyone. Found some tweaks but this is pretty close to RTBC in my opinion.
@Dustin Currie - I'm unassigning since it's been a while but if you'd like to work on updating this patch please reassign. Thanks!
This seems like it should be "within the toolbar's bar area".
Two "the", pick one to remove :)
The @see should be removed and "Default template: toolbar.html.twig." added between the summary line (Prepares…) and the @param.
Remove @ingroup themeable.
Prepares variables for toolbar item templates.
Prepares the tab portion of the toolbar item, the tray portion is rendered
later. (combine into one sentence).
Default template: …
The @see toolbar_pre_render_item() can be kept.
Remove the other @see and the @ingroup themeable.
Comment #24
star-szrThe changes in #23 would make a good novice task.
Comment #25
shanethehat CreditAttribution: shanethehat commentedI think this covers everything in #23
Comment #26
star-szrGreat work, thanks @shanethehat :)
Comment #27
shanethehat CreditAttribution: shanethehat commentedComment #28
star-szrThis is ready to go. Tested manually, markup matches up, code looks good.
Just rolling in one more tiny documentation tweak.
Comment #29
star-szrI messed up, the actual patch from #28 doesn't include the changes from the interdiff. I was experimenting with http://hojtsy.hu/blog/2012-apr-13/quick-and-simple-interdiffs and missed a step. Here's the correct patch with docs tweak (patch + interdiff).
(edited to fess up)
Comment #29.0
star-szrAdd conversion summary table
Comment #30
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.
Comment #31
alexpottComment #32
vinmassaro CreditAttribution: vinmassaro commented#29: 1898464-29.patch queued for re-testing.
Comment #34
mr.baileysAssigning to myself for the DC PDX Friday sprint
Comment #35
mr.baileys#29: 1898464-29.patch queued for re-testing.
Comment #36
mr.baileysGave the anonymous user permissions to the toolbar and a ton of admin stuff to make sure there are enough items in the toolbar, then profiled it and uploaded the results (I ran a couple of batches, results are consistent):
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fb8e35d279&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fb8e35d279&...
Comment #37
mr.baileysNovice tasks have been tackled, so removing that tag.
Comment #38
mr.baileysUnassigning
Comment #39
ezeedub CreditAttribution: ezeedub commentedassigning for profiling
Comment #40
ezeedub CreditAttribution: ezeedub commenteder, nm
Comment #41
cafuego CreditAttribution: cafuego commentedDoing manual test of this at the DrupalCon sprint.
Comment #42
mr.baileysRemoving the "Needs profiling" tag, profiling results in #36
Comment #43
cafuego CreditAttribution: cafuego commentedOutput looks good! Checked visually and diffed the original and twiggified output (after piping through tidy to remove whitespace issues). Diff is attached.
Comment #45
tlattimore CreditAttribution: tlattimore commentedTagging this back as RTBC. The diff from #43 should have had "*--do-not-test.*" in the filename as to not have it run by the test bot, it tried to apply it, and of course it failed and put back to needs work. The patch from #29 looks good.
Comment #45.0
tlattimore CreditAttribution: tlattimore commentedRemove sandbox link
Comment #46
alexpottSo the profiling numbers show a 2% wt time regression. We need to work out why this is. 2% on top of the already large regression from D7 is quite large.
Comment #47
joelpittet@alexpott I see some easy performance wins we could make in the attributes bits. The best performance improvement would be to move all the 4 templates into one template. Or at the very least drop toolbar-item.html.twig.
But first I am going to confirm #36 performance.
Comment #48
joelpittetScenario:
First Run
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ad01387670b&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ad01387670b&...
Second Run
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ad024086f90&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ad024086f90&...
Comment #49
joelpittetSame Scenario, but with the attached patch.
Seems to be 1% regression down to 0.7% regression with just the attributes creation touch-ups.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ad0610ca11a&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ad0610ca11a&...
Comment #50
star-szrAgreed with #47, there is a lot of template nesting going on here that I think we should try to reduce to at least 3 layers if not 2.
Comment #51
joelpittetChanging to CNW based on #50 and #47
Comment #52
gnugetComment #53
gnugetI didn't had the chance to work on this, maybe someone else can take it
Comment #54
jenlamptonI'll take a shot at this one.
Comment #55
jenlamptonThe patch needed a reroll too, so here's a reroll before the refactor.
Comment #56
jenlamptonthis one passes tests, and has some more minor cleanup.
Comment #57
jenlamptonHere's a patch that removes toolbar-tab-wrapper.html.twig. Changing to NR to get testbot involved, but I'll remove at least one more template in the next patch.
Comment #58
jenlamptonthis patch also removes toolbar-item.html.twig and also has a little more cleanup.
Comment #59
jenlamptonand this one removes toolbar-tray-wrapper.html.twig too :) We're ready for a new review now.
Comment #61
jenlamptonpassing tests locally.
Comment #62
star-szrThis is exciting! Great work @jenlampton :)
Going to do some manual testing and this will need re-profiling. In the meantime I found some things that can be cleaned up:
We should remove the 'tab.' - indenting in a list means drill down/"add a dot" and 'tab' is just the loop variable.
We should doc the attributes, label and links variables contained within trays, and we can call trays a list too.
Extra space inside div before attributes are printed.
So awesome :D
Typo here, Prepare.
I think we should document why we're unsetting this.
Comment #63
star-szrOkay, this needed a reroll so here is a reroll after #1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors, with no other changes.
Comment #64
star-szrCompleted 1, 3, and 5 from #62. Nothing needs to be done for 4 :) 2 and 6 are remaining documentation tasks.
This should get the markup closer, I did a DaisyDiff comparison. The only difference I'm seeing now in DaisyDiff is that there are two "Edit" shortcut buttons. I rolled back and couldn't reproduce this with the patch in #61 though.
DaisyDiff showing two edit buttons:
Visually seeing two edit buttons:
Comment #65
star-szrBetter :)
Also wanted to note that there is documentation referring to the toolbar_tab_wrapper theme function that will need to be updated.
Comment #65.0
star-szrUpdated issue summary.
Comment #65.1
star-szrUpdate commit message and conversion table
Comment #65.2
star-szrtoolbar.module not Toolbar.module
Comment #66
star-szrNeeds work for points 2 and 6 from #62.
Edit: And the documentation updates mentioned in my last comment.
Comment #67
joelpittetTagging
Comment #68
joelpittetThere is only a number change on the tray item id's
"toolbar-item--6-tray" and whitespace diffs. I think this is fine...
Comment #69
joelpittetFixes 2 & 6 from #62
Comment #70
joelpittetI'll profile this if someone can confirm that the id changes are fine in #68?
Comment #71
jibran@file missing.
Comment #72
joelpittetThanks #71 is fixed.
Comment #73
star-szrLooks like we lost some stuff from #69 to #72.
Comment #74
joelpittetwhoops, fixed and doing profiling.
Comment #75
joelpittetScenario:
Seven theme
Full permissions for anonymous
Front page set to /node
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f01900df5c&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f01900df5c&...
Comment #77
phiit CreditAttribution: phiit commented#74: 1898464-74-toolbar-twig.patch queued for re-testing.
Comment #77.0
phiit CreditAttribution: phiit commentedUpdate toolbar_item status
Comment #78
markhalliwellPatch reviewed, nothing stands out. Several rounds of profiling with negligible impact. All feedback has been addressed. Let's get this in :)
@joelpittet, if we could get an issue summary update and issues created for the follow-ups, that'd be great!
Great job everyone!
Comment #78.0
markhalliwellreview bonus
Comment #78.1
star-szrReorder suggested commit message based on Dreditor data
Comment #78.2
star-szrUpdate remaining tasks
Comment #79
star-szrThis should be "HTML attributes" (lowercase A on attributes). Other than that I agree with @Mark Carver, this looks good and is still RTBC. Issue summary updated as well.
Tagging for BADCamp sprinting :)
Comment #80
jessebeach CreditAttribution: jessebeach commentedBy eliminating these theme functions, we've removed the ability of module developers and themers to create Toolbar components that do not have a tray. The tray structure is meant to be a convenience, not a fixed structural imposition. Not doing this would represent a regression is flexibility of this module.
We need to reintroduce these theme functions and atomize the toolbar.html.twig file.
Can we do partial template includes? I'm sure TWIG proper can do this, just not sure if our implementation of TWIG has that inclusion.
I can help to break down single template into sub-templates.
Comment #81
star-szrOk, thanks @jessebeach. By partial template includes do you mean including only part of a template into another template? Maybe we can convene in the next couple days to discuss this issue.
Comment #82
jessebeach CreditAttribution: jessebeach commentedYa! We can get this resolved during the sprints tomorrow. I don't think the changes will be difficult or blocking.
Comment #83
jessebeach CreditAttribution: jessebeach commentedcottser and I just discussed this problem. We can probably put a catch-all variable that prints the rest of the render array content (non-tab, non-tray renderables) that were passed to
hook_toolbar
.Comment #84
jenlamptonwe talked about doing the same thing for nodes, forms, and other renderables that need to render some things explicitly, but then still need to render "the rest" and the variable name we came up with for that was
{{ remainder }}
. We should probably be consistent with that naming convention and do the same here. (@see #2004252: node.html.twig template for {{ content.remainder }} example)Comment #84.0
jenlamptonUpdate remaining tasks
Comment #85
jessebeach CreditAttribution: jessebeach commentedAt BADCamp I promised cottser a sandbox module that implements
hook_toolbar
. Here it is:https://drupal.org/sandbox/jessebeach/dev_toolbar
The item it adds is a mock shopping cart. It includes a tab that toggles a div that would contain the shopping cart info. But this div is not in a tray. It's just rendered to the Toolbar wrapper.
EXCEPT, in building this module, I noticed bad structuring. wah wah.
The shopping cart container info div should be a child of
.toolbar
, not.toolbar-bar
. This is something we can correct in the template.I hope this helps!
Comment #86
star-szrAwesome, thanks @jessebeach! The markup in that screenshot is coming from the existing theme functions right?
Comment #87
jessebeach CreditAttribution: jessebeach commentedYes, the module is built from off-the-shelf parts. The markup is just a string in a #markup key.
Comment #88
star-szrTagging for reroll and then we can update the Twig template as discussed.
Comment #89
adnen CreditAttribution: adnen commentedI'm working on this issue
Comment #90
adnen CreditAttribution: adnen commentedComment #91
adnen CreditAttribution: adnen commentedComment #93
adnen CreditAttribution: adnen commentedComment #95
star-szrThanks @adnen. The patches are missing the Twig template for toolbar.html.twig (use
git apply --index
to stage additions when applying a patch) and missing the removal of the theme_toolbar_* functions and template_preprocess_toolbar_tab_wrapper(). The patch file size having shrunk by about half was my first clue.Comment #96
adnen CreditAttribution: adnen commentedi added --index when appling the patch , i also staged the toolbar.html.twig file
Comment #97
star-szrGreat, it has the toolbar.html.twig template now! But the patch still needs to remove the theme_toolbar_* functions and the preprocess function. Please check and compare to the earlier patches.
Comment #99
adnen CreditAttribution: adnen commentedComment #101
adnen CreditAttribution: adnen commentedComment #102
joelpittet@adnen great that's looking better. Green! Just a couple things:
I think this got added by accident because it wasn't in the previous patch.
These bits look like they got added by accident too.
Comment #103
star-szrNeeds work based on the above.
Comment #104
star-szrWorking on this.
Comment #105
star-szrFirst a reroll from #74 to address #102.
Comment #106
joelpittetThis has been profiled, works with @jbeach's module, manual tested and the re-roll is identical.
Comment #107
star-szrNot quite yet :)
Comment #108
star-szrHere are the changes to add
{{ remainder }}
, this makes @jessebeach's module (https://drupal.org/sandbox/jessebeach/dev_toolbar) work. I'm also attaching a small patch to @jessebeach's module that modifies the render array to match up with what's in core now and prevents double tab rendering when tested along with this patch.This also takes care of the minor change pointed out in #79, and I re-profiled and we are actually calling less functions now than before because there are less calls to theme().
Scenario is the same as #75, a fresh install with the standard profile and gave anonymous users full permissions.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c8629919553&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c8629919553&...
Comment #109
star-szrAnd I forgot -do-not-test for the dev_toolbar patch :P
Comment #111
star-szrComment #112
jessebeach CreditAttribution: jessebeach commentedThe #108 patch looks great!!!
Cottser, I added your patch to the dev_toolbar module, btw, although I don't think we'll need it any more.
I tested the patch with the dev_toolbar module and had a look through the rendered HTML. Everything is where it should be.
I'm the one who dropped this from RTBC in #80. I have no more objections. Cottser provided profiling, which has caused alexpott to drop this from RTBC. So I think we can safely re-up this patch to RTBC now.
I'M SO EXCITED ABOUT THIS! Awesome work!
Comment #113
webchickHoly cow. This is a GREAT clean-up!!
Committed and pushed to 8.x. Thanks!
Jesse, does it make sense to backport this to the D7 toolbar module too?
Comment #114
star-szrWoohoo!