Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Issue #1898436 by chrisjlee, joelpittet, gnuget, Cottser: overlay.module - Convert PHPTemplate templates to Twig.
(as of #43)
Task
Convert PHPTemplate templates to Twig templates.
Remaining
n/a
Template path | Conversion status |
---|---|
core/modules/overlay/templates/overlay.tpl.php | converted |
Profiling results
=== 8.x..8.x compared (51981beb768ad..51981e001fd46):
ct : 36,140|36,140|0|0.0%
wt : 345,481|345,338|-143|-0.0%
cpu : 316,694|316,391|-303|-0.1%
mu : 28,878,264|28,878,264|0|0.0%
pmu : 28,993,328|28,993,328|0|0.0%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51981beb768ad&...
=== 8.x..overlay-1898436-33 compared (51981beb768ad..51981e3ae1e6c):
ct : 36,140|36,220|80|0.2%
wt : 345,481|345,604|123|0.0%
cpu : 316,694|317,226|532|0.2%
mu : 28,878,264|28,913,256|34,992|0.1%
pmu : 28,993,328|29,022,712|29,384|0.1%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51981beb768ad&...
Related
#1987408: overlay.module - Convert theme_ functions to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
Comment | File | Size | Author |
---|---|---|---|
#39 | 1898436-39.patch | 4.03 KB | star-szr |
#39 | interdiff.txt | 621 bytes | star-szr |
#38 | interdiff.txt | 2.37 KB | joelpittet |
#38 | 1898436-38-overlay-twig.patch | 3.94 KB | joelpittet |
#37 | 1898436-37-overlay-twig.patch | 3.07 KB | joelpittet |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
zakiya CreditAttribution: zakiya commentedComment #3
chrisjlee CreditAttribution: chrisjlee commentednot sure if i should add back in
template_preprocess_overlay_disable_message
function. if so #4 includes adding back in that function to core.Comment #4
chrisjlee CreditAttribution: chrisjlee commentedThis patch includes the
template_preprocess_overlay_disable_message
function.Comment #5
zakiya CreditAttribution: zakiya commentedPatch works as expected when applied to core. Marked as reviewed but should probably use classes instead of ids.
Comment #6
webchickAwesome!!
I would love a quick sign-off on this from one of the Twigluminati ;), so arbitrarily assigning to c4rl since he was up above.
Comment #7
joelpittet@zakiya Totally agree, we should get rid of the ID, although not in this issue, this is meant to be a as straight forward conversion as possible. Could you open another issue as maybe a follow-up to this one to look at removing the ID and fixing the JS/CSS that that change may affect?
Removed the
drupal_render()
And removed
theme_overlay_disable_message()
because from what I remember(unless things have changed) we are getting rid of these.And just some spacing nitpicks are in here too.
Functionally I would give this an RTBC.
Comment #8
star-szrFound some documentation tweaks that can be done to move this along further, and tagging for manual testing.
This should be updated to match #1913208: [policy] Standardize template preprocess function documentation, something like:
…and so on. No @ingroup needed.
This comment should be removed, since we're not rendering yet.
If we change "how to disable" to "disabling" this will fit within the 80 character limit for summaries (http://drupal.org/node/1354#file).
We can probably remove this line, the attributes are not output in the template. If we keep it, we should say something like "HTML attributes for the containing element.", assuming the attributes are for the
<div>
.First line is 81 characters, should wrap to 80 per http://drupal.org/node/1354#drupal. The second line should match the indent of the first, lining up with the 't' in 'tabs'.
Comment #9
star-szrGot the okay from @joelpittet to unassign.
Comment #10
star-szrCreating a new patch and interdiff based on #7 with the documentation changes in #8 would be a good novice task.
Comment #11
chrisjlee CreditAttribution: chrisjlee commentedAttempt to reroll.
@cottser Thanks for the helpful and thorough review
Comment #12
chrisjlee CreditAttribution: chrisjlee commentedoops forgot to remove @themeable
Comment #13
chrisjlee CreditAttribution: chrisjlee commentedChanges as per conversation with @Cottser
- Remove line above where @themeable in docblock was
Comment #14
chrisjlee CreditAttribution: chrisjlee commentedRealized i forgot to wrap one of the docblock lines as per comment #8.5.
Comment #15
star-szrThanks for keeping this moving @chrisjlee! I just tested this patch manually and found that overlay-disable-message.html.twig was not being picked up, we need to add the 'template' declaration in overlay_theme() for that template to be used.
And some more documentation tweaks:
In overlay-disable-message.html.twig, let's use the same documentation for these variables as in template_preprocess_overlay_disable_message(), the preprocess function docs are much better.
In overlay.html.twig, the description of the 'title' variable should start with a capital letter (capitalize the first 'the') per http://drupal.org/node/1354#drupal.
Comment #16
chrisjlee CreditAttribution: chrisjlee commented@Cottser thanks for the feedback again it was very helpful and thorough.
Comment #17
star-szrLooks good, just needs this docs tweak from #15 then I think this one is ready! I tested this one manually, works much better now :)
Comment #18
star-szrThe above change is a good novice task, tagging. Once that's done this patch is RTBC in my opinion.
If you'd like to work on this, please assign the issue to yourself. If you get stuck, drop by #drupal-twig on IRC or post your questions here. Thanks!
Comment #19
chrisjlee CreditAttribution: chrisjlee commentedOverlay works better for me as well after the patch.
Changes as per #15
Comment #20
chrisjlee CreditAttribution: chrisjlee commentedComment #21
star-szrI've been eyeing the first hunk of this patch and I finally tracked it down to being a reversal of part of http://drupalcode.org/project/drupal.git/commit/b574b9e. @chrisjlee is on it!
Comment #22
chrisjlee CreditAttribution: chrisjlee commentedremove offending change reference according to #21
Comment #23
star-szrAh, much better. This is good to go when it comes back green, thanks for all your work on this @chrisjlee!
Comment #25
star-szr#22: 1898436-convert-overlay-to-twig-22.patch queued for re-testing.
Comment #26
star-szrTestbot decided it didn't want to apply the patch, back to RTBC :)
Comment #26.0
star-szrAdd conversion summary table
Comment #27
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to templates rather than theme_ functions. See #1987408: overlay.module - Convert theme_ functions to Twig for theme_ function conversion.
Comment #27.0
c4rl CreditAttribution: c4rl commentedRemove sandbox link
Comment #28
gnugetComment #29
gnugetAdding a patch only with the .tpl.php conversions.
Comment #30
star-szrThanks @gnuget! We'll need a patch with only the theme_ function conversions uploaded at #1987408: overlay.module - Convert theme_ functions to Twig to complete the patch split. One way of doing this is by applying #22 locally then reverse applying #29 (
git apply -R patchname.patch
).Comment #31
chrisjlee CreditAttribution: chrisjlee commentedSince gnuget assigned it to anonymous, I took care of #30. Let me know if there are any issues.
Comment #32
star-szrWe should be able to lose the render_var here, just {{ tabs }} please.
Comment #33
gnugetRemoving the render_var
Comment #34
star-szrTagging for profiling.
Comment #35
star-szrI'll be profiling this and giving it another review.
Comment #36
star-szrProfiling results look great.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51981beb768ad&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51981beb768ad&...
This needs a few small tweaks and then it's RTBC from my perspective:
We need to add the 'role' and 'aria-controls' attributes as in the .tpl.php version.
We should document at least 'disable_overlay' - something like "the message about how to disable the overlay."?, but we can probably document the attributes ones too by copying and pasting from existing conversions.
Comment #37
joelpittetChanges from #36
Comment #38
joelpittetfew more things, moved ids to the preprocess and added attributes comments.
Comment #39
star-szrWow, even better. Thanks @joelpittet!
Just rolling in a typo fix for pre-existing docs. RTBC :)
Comment #39.0
star-szrClarify issue summary
Comment #41
star-szr#39: 1898436-39.patch queued for re-testing.
Comment #42
joelpittetLooks good, back to rtbc.
Comment #43
alexpott+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #43.0
alexpottAdd profiling results to summary, update remaining
Comment #44
alexpottCommitted b171982 and pushed to 8.x. Thanks!
Comment #45.0
(not verified) CreditAttribution: commentedUpdated issue summary.