Issue #1938848 by Jaesin, Cottser, jenlampton, hefox, Shawn DeArmond, frob, boze, joelpittet, mrf, dcrocks: seven.theme - Convert PHPTemplate templates to Twig.
(as of #59)
Task
Convert PHPTemplate templates to Twig templates.
Remaining
Template path | Conversion status |
---|---|
core/themes/seven/templates/maintenance-page.tpl.php | converted |
core/themes/seven/templates/page.tpl.php | converted in #1961870: Convert page.tpl.php to Twig instead so breadcrumb can be converted to a render array |
Profiling results
=== seven-maintenance-8.x..seven-maintenance-8.x compared (5198b38f0f1d5..5198b3b171080):
ct : 16,931|16,931|0|0.0%
wt : 107,587|107,545|-42|-0.0%
cpu : 92,995|93,049|54|0.1%
mu : 8,896,408|8,896,408|0|0.0%
pmu : 8,969,600|8,969,600|0|0.0%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198b38f0f1d5&...
=== seven-maintenance-8.x..seven-maintenance-1938848-53 compared (5198b38f0f1d5..5198b3cd5c9c1):
ct : 16,931|17,051|120|0.7%
wt : 107,587|108,046|459|0.4%
cpu : 92,995|93,272|277|0.3%
mu : 8,896,408|8,938,544|42,136|0.5%
pmu : 8,969,600|9,004,104|34,504|0.4%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198b38f0f1d5&...
To test this code:
To test the tablesort indicator override, apply this patch along with the latest patch from #1939090: Convert theme_tablesort_indicator() to Twig and visit admin/content. The arrow indicating the column sort should be black instead of the default gray.
(others @todo)
Related
#1938864: [meta] Update all core themes to use Twig
#1987424: seven.theme - Convert theme_ functions to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
Comment | File | Size | Author |
---|---|---|---|
#53 | 1938848-53.patch | 4.08 KB | star-szr |
#53 | interdiff.txt | 528 bytes | star-szr |
#51 | twig-convert_seven_templates-1938848-51.patch | 4.08 KB | jenlampton |
#50 | twig-convert_seven_templates-1938848-50.patch | 1.79 KB | jenlampton |
#49 | twig-convert_seven_templates-1938848-49.patch | 4.08 KB | jenlampton |
Comments
Comment #1
mrf CreditAttribution: mrf commentedA few of us made a bit of progress on this today.
http://drupal.org/sandbox/mrf/1950628
Comment #2
mrf CreditAttribution: mrf commentedIn order to override the overrides in the template.php we'll need #1898038: custom_block.module - Convert theme_ functions to Twig #1898432: node.module - Convert PHPTemplate templates to Twig in before this will apply cleanly.
Comment #3
mrf CreditAttribution: mrf commentedAlso overriding #1939090: Convert theme_tablesort_indicator() to Twig
Comment #4
Jaesin CreditAttribution: Jaesin commentedOur first pass at converting the seven theme to the twig engine.
Comment #6
yoroy CreditAttribution: yoroy commentedI'd like to find out if and how this effort can help implement the proposed new styles for Seven (http://drupal.org/sandbox/ry5n/1932040)
Comment #7
ry5n CreditAttribution: ry5n commented@yoroy Folks more familiar with the Twig work should correct me on the following. AFAIK, the template conversions involve first porting the existing templates as-is to Twig, then cleaning them up. Until the first step is done, we can work in parallel. Once we hit the second step, we need to coordinate much more closely. I’m hoping to have all our prototyping work done by the time the Twig conversions are ready. Then we can do the Seven UI integration along with markup and CSS cleanup.
Comment #8
star-szr@ry5n, that sounds exactly right to me. Thanks!
Comment #9
Jaesin CreditAttribution: Jaesin commented#4: drupal-convert_seven_theme_to_twig-1938848-4.patch queued for re-testing.
Comment #10
Jaesin CreditAttribution: Jaesin commented@mfr, @hefox
The first test failed because twig wasn't enabled during the install phase. http://drupal.org/node/1875992#comment-7234864 was committed fixing the problem.
Comment #11
Jaesin CreditAttribution: Jaesin commentedThat was the wrong link. This is the patch that was committed Fri Mar 29 11:02:49 2013: http://drupal.org/node/1942490#comment-7235030. It cleared the way for the patch in #4.
Comment #11.0
Jaesin CreditAttribution: Jaesin commentedinfo
Comment #12
star-szrThanks @Jaesin.
As far as I know, this is no longer a blocker for #1898038: custom_block.module - Convert theme_ functions to Twig, so I'm removing that issue from the summary.
Comment #12.0
star-szrAdd core themes conversion meta
Comment #13
hefox CreditAttribution: hefox commentedShould just be an update for changes to seven in core and update for #5 patch in #1939090: Convert theme_tablesort_indicator() to Twig
Comment #15
hefox CreditAttribution: hefox commentedComment #16
star-szrInterdiffs would be great here, thanks for working on this @hefox :)Edit: Sorry, nevermind, this is just a reroll I think.
Comment #17
star-szrSome manual testing here would be great.
Comment #18
dcrocks CreditAttribution: dcrocks commentedInstalled patch on new copy of 8.x before install. Immediately noticed menu appearance changes. All the menus got wider and 'structure' and 'reports' seem to have lost some styling. Attached some images. You can't see it in image but 'after' image had horizontal scroll bar.
Comment #19
star-szrThanks @dcrocks, that's great!
Please see #1938864-3: [meta] Update all core themes to use Twig, I think I'll try to put together a simplytest.me link for testing this issue along with its dependencies.
Comment #19.0
star-szrRemoving blocking issue from summary
Comment #20
star-szrseven_admin_block_content() needs converting.
Comment #21
star-szrThis will also need a reroll now that #1033116: Rename template.php to THEMENAME.theme to eliminate ambiguity around "the template file" vs. "template files" is in.
Comment #22
Jaesin CreditAttribution: Jaesin commentedI added seven_preprocess_admin_block_content.
Updated to patch against seven.theme.
Patches against Commit: 809138a7c6951528e626987bc4e524ba1e85759a
Date:Thu Apr 11 21:58:27 2013 +0100
Comment #24
star-szr@Jaesin, thank you. It looks like the patch didn't come across quite right. The patch does not contain any of the changes in #15 and it removes two files:
core/themes/seven/templates/maintenance-page.tpl.php
core/themes/seven/templates/page.tpl.php
Please find more information here on contributing patches:
http://drupal.org/node/707484
Comment #25
Jaesin CreditAttribution: Jaesin commented#22: drupal-convert_seven_theme_to_twig-1938848-22.patch queued for re-testing.
Comment #26
Jaesin CreditAttribution: Jaesin commentedI guess I should look over the patch before uploading it.
Patch applies to Commit: ee1c53e517fdf466a770d27fddb67bbcbaeb209a
Date: Thu Apr 11 22:42:45 2013 +0100
Comment #27
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedcustom-block-add-list.html.twig needs work.
See: http://drupal.org/node/1898038#comment-7280932
Also, this isn't right:
Since they're not node types.
@TODO: compare custom-block-add-list.html.twig in both
http://drupal.org/files/drupal-convert_seven_theme_to_twig-1938848-26.patch
and
http://drupal.org/files/1898038-22.patch
and make the best template file.
Comment #28
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedHere's a patch that does what I said in #27.
I'll go remove custom-block-add-list.html.twig and any changes to seven_preprocess_custom_block_add_list() from #1898038: custom_block.module - Convert theme_ functions to Twig
Comment #29
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedHuh. Noticed some whitespace issues in the docblock for seven_preprocess_custom_block_add_list().
Comment #29.0
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedAdd conversion summary table
Comment #30
star-szrJust tweaked the tablesort indicator override to work alongside #1939090: Convert theme_tablesort_indicator() to Twig and added steps for testing that to the summary. Adding testing steps for the other overrides would be a great help, thanks to everyone who's worked on this issue so far :)
Comment #31
Jaesin CreditAttribution: Jaesin commentedFixed some invalid HTML. Namely closing tags from </dl> to </ul> on admin-block-content.html.twig and node-add-list.html.twig.
Comment #31.0
Jaesin CreditAttribution: Jaesin commentedStart on testing steps
Comment #32
star-szrGreat catch @Jaesin!
After giving this another look I updated the issue summary to indicate that everything is converted. Woohoo!
Here are some points that can be touched up, all minor and related to coding standards and documentation:
This file needs a newline at the end of the file per http://drupal.org/coding-standards#indenting.
This docblock needs some tweaking to match up with http://drupal.org/node/1823416#docblock (We're missing the PHP-style /**) and we should remove the blank line between the docblock and the doctype.
And page.tpl.php should be page.html.twig here.
html.html.twig.
template_process() itself is gone in Drupal 8 so let's remove this line. #1290694: Provide consistency for attributes and classes arrays provided by template_preprocess()
Comment #33
Tim Bozeman CreditAttribution: Tim Bozeman commentedComment #34
Tim Bozeman CreditAttribution: Tim Bozeman commentedFixed issues from comment #32, also removed line 2 whitespace from page.html.twig.
tyvm Long Beach Drupal contrib meetup!
Comment #35
frobThere needed to be a line at the end of node-add-list.html.twig
Comment #35.0
frobUpdate conversion table
Comment #36
mike stewart CreditAttribution: mike stewart commentedpatch applies cleany and everything working when switch to seven theme.
However, seems to be a styling problem related to seven/templates/page.html.twig & #main-content ... but that seems outside of scope of this issue.
Comment #37
star-szrIf we can get full manual testing steps in the summary that would be great for testing this patch! Here's my review of the latest patch:
For these, 'Implements hook_preprocess_HOOK' makes more sense, but let's keep the comments explaining these overrides.
Capitalize 'class' and end this comment in a period per http://drupal.org/node/1354#drupal.
Since 'type' is just the loop variable and can be called anything, we should say something like:
Same thing here.
And here.
I think since we're not printing any markup around sidebar_first, let's just remove the if block.
maintenance-page.html.twig is missing the
<footer>
This is one character too long per http://drupal.org/node/1354#file. The summary line has to fit within 80 characters.
Not sure about the extra whitespace here in the Twig version, caught my eye. I think the spaces around printing the primary_local_tasks variable can be removed.
Comment #38
frobComment #39
frobLets hope this gets it.
Comment #40
frobComment #41
star-szrThanks @frob! I'm just moving page.html.twig from this issue to #1961870: Convert page.tpl.php to Twig because the automated tests use the seven theme for breadcrumb testing.
After that, I will post a quick review of the changes in #39.
Comment #41.0
star-szrUpdate remaining
Comment #42
star-szrThanks again for moving this forward @frob! Here's another review:
The summary line here needs to end in a period and have a blank line below it per http://drupal.org/node/1354#drupal and http://drupal.org/node/1354#functions.
In this case the 'Prepare variables' comment is unnecessary, let's please remove it.
I should have referenced this before but when starting a new list indentation we need to end the previous sentence in a colon (http://drupal.org/node/1354#lists), so for these it would make sense to say something like:
We shouldn't be adding an id="footer" here, it's not in maintenance-page.tpl.php.
I didn't think of this before but this isn't actually the default theme implementation. This is Seven's theme implementation (see maintenance-page.html.twig):
We also can't wrap summary lines, they have to fit within 80 characters per http://drupal.org/node/1354#file.
So for this one "Seven's theme implementation to list node types available for adding content." would just barely fit within 80 characters :)
Comment #43
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 #1987424: seven.theme - Convert theme_ functions to Twig for theme_ function conversion.
Comment #43.0
c4rl CreditAttribution: c4rl commentedUpdate conversion table
Comment #44
joelpittetDoc tweaks from #42
Haven't split out the theme_functions yet from #43
Though this one is a bit strange because page.tpl.php is over at #1961870: Convert page.tpl.php to Twig, thoughts?
@Cottser have another crack at reviewing this, I added a few more bits missing, hopefully in the right direction. I bet after another little cleanup it would be good to go for the split.
Comment #46
star-szrThank you @joelpittet! page.tpl.php was moved over so breadcrumb tests would pass over at #1961870: Convert page.tpl.php to Twig.
This should pass tests, I wasn't thinking it through leaving page.tpl.php here and also leaving
engine: twig
in seven.info.yml. We're only allowing Twig templates in PHPTemplate themes temporarily for conversion, not the other way around.I'm thinking this issue should live on as converting seven's maintenance-page.tpl.php (I'd probably say move it to #1987426: Convert maintenance-page.tpl.php to Twig but then it'd make sense to move Bartik's there too) and we should split off the theme_ function conversions to #1987424: seven.theme - Convert theme_ functions to Twig as described in #43.
Comment #47
star-szrThis is the last patch that needs to be split up to only contain .tpl.php conversions and related preprocess functions.
Comment #48
jenlamptondibbs
Comment #49
jenlamptonOkay, here's a re-roll of just the maintenance page template.
Comment #50
jenlamptondoh, trying again
Comment #51
jenlamptonokay, 3rd times a charm.
Comment #52
star-szrFound some tweaks and will do manual testing.
Comment #53
star-szrTwo changes:
The markup matches up, checked via markup diff and DaisyDiff. If someone can give the docs and code a once-over I think this one is RTBC.
Comment #54
joelpittetHad a scan through the code and docs and looks great, thanks @Cottser!
Comment #55
star-szrI'm planning on profiling this template by copying it to the Stark theme.
Comment #56
star-szrProfiling.
Comment #57
star-szrProfiling results:
Added this to the end of seven_preprocess_maintenance_page():
I copied node.html.twig from modules/node/templates to seven/templates.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198b38f0f1d5&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198b38f0f1d5&...
Comment #57.0
star-szrRevise summary
Comment #58
star-szrLoading the extra node is for the purpose of isolating profiling to this conversion only, not loading all of the Twig classes etc.
Comment #59
alexpott+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #59.0
alexpottAdd profiling results
Comment #60
alexpottCommitted 3d24449 and pushed to 8.x. Thanks!
Comment #61
Jaesin CreditAttribution: Jaesin commentedAwsome! Thanks to everyone who worked on this.
Comment #62.0
(not verified) CreditAttribution: commentedAdd commit message