Issue Summary can be found in this gist: https://gist.github.com/steinmb/5632073
Problem/Motivation
The Twig team has decided to focus on .tpl.php to .html.twig conversions for the immediate future, with the goal of getting these conversions committed before or at DrupalCon Portland. As a result, some of our existing conversion issues will need to be split up and the .tpl.php conversions and related preprocess changes moved to the new issues.
This will be the first Twig conversion patch to get into core, solely focused on *.tpl.php. Conversion of concatenation-based theme_ functions is a desired secondary step, but is lower priority than all work below.
Current status
We're ready to run profiling on the issues listed below under Remaining tasks. See the Contributor profiling instructions for more information.
Finished tasks
These issues have converted all tpl.php files within their scope to html.twig, are well documented, been manually tested for HTML inconsistencies, are performant with proof and have been verified by a core maintainer.
- theme_test module templates: #1898458: theme_test.module - Convert PHPTemplate templates to Twig Assigned to: widukind
- html.tpl.php: #1961872: Convert html.tpl.php to Twig
- page.tpl.php: #1961870: Convert page.tpl.php to Twig
- region.tpl.php: #1961868: Convert region.tpl.php to Twig
- maintenance-page.tpl.php: #1987426: Convert maintenance-page.tpl.php to Twig
- overlay module templates: #1898436: overlay.module - Convert PHPTemplate templates to Twig
- taxonomy module templates: #1898460: taxonomy.module - Convert PHPTemplate templates to Twig
- seven theme templates: #1938848: seven.theme - Convert PHPTemplate templates to Twig
- book module templates: #1898050: book.module - Convert PHPTemplate templates to Twig
- user module templates: #1898468: user.module - Convert PHPTemplate templates to Twig
- aggregator module templates: #1896060: aggregator.module - Convert PHPTemplate templates to Twig
- layout module templates: #1898424: layout.module - Convert PHPTemplate templates to Twig
- forum module templates: #1898418: forum.module - Convert PHPTemplate templates to Twig
- system module templates: #1898454: system.module - Convert PHPTemplate templates to Twig
- comment module templates: #1898054: comment.module - Convert PHPTemplate templates to Twig
- block module templates: #1898034: block.module - Convert PHPTemplate templates to Twig
- node module templates: #1898432: node.module - Convert PHPTemplate templates to Twig
- views ui module: #1898472: [meta] Convert views_ui module to Twig
- views module: #1843738: [meta] Convert views module to Twig
- #1843758: Convert views/templates/views-view-row-rss.tpl.php to twig
- #1843760: Convert views/templates/views-view-rss.tpl.php to twig
- #1843740: Convert views/templates/views-exposed-form.tpl.php to twig
- #1843766: Convert views/templates/views-view-table.tpl.php to twig
- #1843770: Convert views/templates/views-view-unformatted.tpl.php to twig
- #1843754: Convert views/templates/views-view-list.tpl.php to twig
- #1843750: Convert views/templates/views-view-grid.tpl.php to twig
- #1843762: Convert views/templates/views-view-summary.tpl.php to twig
- #1843742: Convert views/templates/views-more.tpl.php to twig
- #1843752: Convert views/templates/views-view-grouping.tpl.php to twig
- #1843744: Convert views/templates/views-view.tpl.php to twig
- #1843764: Convert views/templates/views-view-summary-unformatted.tpl.php to twig Assigned to: tlattimore
- #1843746: Convert views/templates/views-view-field.tpl.php to Twig
- #1806478: Make twig the default engine once all modules templates are converted from .tpl.php to .html.twig
- field module templates: #1898062: field.module - Convert PHPTemplate templates to Twig
- views module: #1843738: [meta] Convert views module to Twig
- #1843748: Convert views/templates/views-view-fields.tpl.php to twig Assigned to: tlattimore
- bartik theme: #1938840: bartik.theme - Convert PHPTemplate templates to Twig
Finished tasks (unverified) a.k.a. the Alex Pott section
These issues have converted all tpl.php files within their scope to html.twig, are well documented, been manually tested for HTML inconsistencies, are performant with proof but have not yet been verified by a core maintainer.
For Fabianx
(currently empty)
Patches needing review/RTBC
(currently empty)
Profiling tasks
(currently empty)
Patch updates
(currently empty)
User interface changes
None.
API changes
Likely minimal to no API changes.
Related
#1757550: [Meta] Convert core theme functions to Twig templates
Resources
Performance followups
#2002094: Improve performance of comment.html.twig
#2002104: Improve performance of field.html.twig
#2002108: Improve performance of views-view-fields.html.twig
#2002222: Improve performance of block-admin-display-form.html.twig
#1815250: Type twig variables in PHP and remove superfluous render_var() and getAttribute() calls from the generated code
Comment | File | Size | Author |
---|---|---|---|
#34 | 1987510-34.patch | 357.69 KB | star-szr |
#34 | interdiff.txt | 37.54 KB | star-szr |
#23 | interdiff.txt | 1.11 KB | joelpittet |
#23 | 1987510-22-tpl-files.patch | 353.1 KB | joelpittet |
#19 | 1987510-18.patch | 352.46 KB | star-szr |
Comments
Comment #1
jenlamptonLooks like this one should be critical.
Comment #2
c4rl CreditAttribution: c4rl commentedI understand the intention of this issue but given the momentum of the existing conversion issues, I've argued that those are better for the template-based conversions and these new ones should be for the theme_-function based conversions.
I'm going to be revising these titles. I'm not we need a separate meta here the only justification was "too big." @jenlampton, can you clarify how "too big" is worse than "two meta issues?"
Comment #3
c4rl CreditAttribution: c4rl commentedRetitling per #2
Comment #3.0
c4rl CreditAttribution: c4rl commentedRemove unneeded sentence from first paragraph
Comment #4
c4rl CreditAttribution: c4rl commentedRevising priority for now.
Comment #5
jenlamptonThe intention of creating this issue was to prevent 1757550 from getting closed when "the patch" went in. "The patch" being the one with all the .tpl.php conversions bundled together. This issue was meant to be the one that got closed.
Comment #6
jenlamptonretitling to that end. Also, still critical.
Comment #6.0
jenlamptonRevise issues list
Comment #6.1
jenlamptonremove theme conversions
Comment #6.2
jenlamptonlinks to conversions
Comment #7
c4rl CreditAttribution: c4rl commentedClarifying title. @Fabianx has volunteered to combine patches above once they are at least "needs review" (and hopefully RTBC).
Comment #7.0
c4rl CreditAttribution: c4rl commentedbenchmarking, bold
Comment #8
Fabianx CreditAttribution: Fabianx commentedAnd here is the twig mega patch, this still needs the patch included to remove double engine support and add twig as default engine.
It is proposed to review individual issues, but this patch is the overall whole package.
( This was build automatically from: https://github.com/LionsAd/twig-in-core-megapatch/branches )
Lets see what testbot thinks :).
Comment #8.0
Fabianx CreditAttribution: Fabianx commentedUpdated issue summary.
Comment #9
thedavidmeister CreditAttribution: thedavidmeister commentedWe're doing the profiling against the "mega patch" now, right?
We can probably start doing that now if we want.
AFAICS almost everything that is "needs work" is documentation only and the majority of "needs review" is primarily waiting on manual testing rather than feedback on how the code will be structured.
Comment #9.0
thedavidmeister CreditAttribution: thedavidmeister commentedtrigger update
Comment #9.1
shanethehat CreditAttribution: shanethehat commentedtrigger update
Comment #10
thedavidmeister CreditAttribution: thedavidmeister commented1 month left! We've got 32 more patches to get [READY]. We need to average knocking off more than one every day now.
If you want to help, the main tasks left are Manual testing and Profiling. If you don't know how to do either then join #drupal-twig in IRC and ask. There's almost always someone in IRC who can help explain how we want Manual testing done and @Fabianx and @Cottser have been doing most of the profiling so far so they're the ones to talk to for that.
Comment #10.0
thedavidmeister CreditAttribution: thedavidmeister commentedupdate again
Comment #10.1
star-szrAdd default theme engine issue
Comment #11
star-szrProfiling is indeed our largest remaining task for .tpl.php conversion. Not sure where else to post this, so here goes.
Here is what I know so far in regards to Twig profiling, from what @Fabianx has taught me and my experience so far:
Most of us have been using @Fabianx’s xhprof-kit, please find installation instructions there. I recommend installing Drush 6.x and registry rebuild.
I’ve also been using these functions and aliases along with xhprof-kit, and these are used in the instructions below:
https://gist.github.com/Cottser/5588734
To be able to upload runs for others to see, ping @Fabianx on IRC for an API key.
Recommended profiling workflow
user-1898468-91
and commit the Twig conversion patch there.Isolate profiling to only the current conversion
To isolate the profiling to only the conversion at hand in cases where there are no nodes on the page, I recommend doing one of two things (the entity reference method takes a bit longer to set up but you may have more stable numbers using that method):
First, create a sample node.
Method one - Views:
Method two - Entity Reference:
Run profiling
To continue with the example of user.tpl.php profiling, we have a
user-1898468-91
branch with the Twig conversion committed to it and the scenario is set up so that anonymous users can view it on the homepage (with Stark theme and a node being rendered somewhere on the page). Once all that is in place I will:Switch to 8.x:
git checkout 8.x
Run
bbranch
, you should get output like this:loop time: |0.345511s|51978cc86c728|drupal-perf|8.x|<a href="http://d8prof.dev/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run=51978cc86c728&extra=8.x" target="_blank">Profiler output</a>
Copy the XHProf run ID from bbranch, e.g.
51978cc86c728
Run
bbranches 51978cc86c728 user-1898468-91
to compare your current branch (8.x) against the Twig conversion branch.Once that finishes running you should have something like this:
For the first 8.x..8.x comparison we don’t want the wt to fluctuate much at all - that is our baseline comparison to show that the first run (from
bbranch
) is still accurate, because that is what is being used to compare against these two new runs. You may have to go through this process (run bbranch, get run ID, run bbranches with new run ID) a few times to get a low wt fluctuation, we generally want something under +-0.5% but less is better. Don't waste all day trying to get a 0% fluctuation though :)Post results
Once you have a comparison you’re happy with, (and you have an API key from @Fabianx) you can run:
Where
51978cc86c728
is the baseline run used for comparing, originally frombbranch
.Then you'll get something like this:
I take out the 'Run [foo] uploaded successfully' bits, wrap the comparison chunks in
<code>
tags, and post.Comment #12
ezeedub CreditAttribution: ezeedub commentedFor xhprof on PHP 5.4, see http://michaelsanford.com/compiling-xhprof-for-php-5-4/
Comment #12.0
ezeedub CreditAttribution: ezeedub commentedAdded link to twig best practices doc.
Comment #12.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary to have a "finished" section
Comment #12.2
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary to have a "hopefully finished" section
Comment #12.3
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #12.4
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #12.5
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #12.6
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #13
thedavidmeister CreditAttribution: thedavidmeister commentedGreat work by everyone in the past few days, only 24 patches left that need profiling and/or manual testing!
The manual testing is mostly complete but we still need a lot of help profiling. AFAICS every single sub-task of the Views module is now "complete" but is just waiting on performance testing. If we can power through those (and nothing unexpected pops up) we'll be looking really good for the June 17 deadline :)
Comment #14
star-szrThanks for the awesome issue summary rearrangement @thedavidmeister. Let's move those issues on up! We got tons of people ready to do profiling today.
I just added branch completion to the xhprof-kit aliases/functions, enjoy: https://gist.github.com/Cottser/5588734
Edit: Also, if you are setting up xhprof-kit and are not getting any output, be sure to edit find-min-web.sh to point to your Drupal 8 install! (Change 127.0.0.1 if necessary).
Comment #14.0
star-szrUpdated issue summary.
Comment #15
thedavidmeister CreditAttribution: thedavidmeister commentedI just checked. Nothing is left with the manual testing tag and nothing is set to "needs work". Theoretically all that's left is the profiling.
Comment #16
joelpittetMoving and rewriting docs on #11 over at Contributor Profiling Instructions
Please update or let me know if you have some issues.
Comment #16.0
joelpittetUpdated issue summary.
Comment #16.1
jenlamptonrtbap
Comment #16.2
star-szrMovin' on up!
Comment #16.3
star-szrMove aggregator back for re-profiling
Comment #16.4
star-szrMore moving up :D
Comment #16.5
jenlamptonmove up
Comment #16.6
jenlamptonready
Comment #16.7
c4rl CreditAttribution: c4rl commentedLink to profiling instructions
Comment #16.8
star-szrMove views-view-rss up
Comment #16.9
star-szrMove up
Comment #16.10
star-szrMoving up!
Comment #16.11
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #16.12
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #16.13
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #17
star-szrI watched @Fabianx roll this new megapatch. This is a multipatch formatted patch for easier reviewing.
Also includes #1806478: Make twig the default engine once all modules templates are converted from .tpl.php to .html.twig and excludes #1898458: theme_test.module - Convert PHPTemplate templates to Twig.
Comment #19
star-szrMultipatch seems like it might not work. Here is a normal patch.
Patch is coming from https://github.com/Cottser/twig-in-core-megapatch
Comment #19.0
star-szrmoved aggregator to alexpott's area
Comment #20.0
(not verified) CreditAttribution: commentedmoving items for fabian.
Comment #20.1
joelpittetmoved views-view to be alexpotted
Comment #20.2
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #20.3
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #21
star-szrLooks like I missed something small in the double engine patch, I will update there and try to roll a new megapatch at some point today. I also will be adding some automated tests so that we ensure PHPTemplate themes still work as expected.
There are also a couple patches that conflicted unnecessarily so I will either remove those changes or mark them as novice tasks to do so :)
Just a reminder once we have all the individual patches profiled and [READY], we will do some more profiling on this megapatch for some basic scenarios, exact scenarios TBD but a node page with 50 comments would be a start.
Comment #21.0
star-szrUpdated issue summary.
Comment #21.1
cweagansMoving ready issues to ready category
Comment #21.2
tlattimore CreditAttribution: tlattimore commentedChanging fabianx's name to use lowercase
Comment #21.3
geoffreyr CreditAttribution: geoffreyr commentedMoved #1843750 to finished/unverified pile.
Comment #21.4
ianthomas_ukMove a couple more issues to "finished"
Comment #21.5
geoffreyr CreditAttribution: geoffreyr commentedMoved #1843752 to finished/unverified.
Comment #21.6
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #21.7
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #21.8
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #22
thedavidmeister CreditAttribution: thedavidmeister commentedjust 10 general profiling tasks left :)
Comment #22.0
thedavidmeister CreditAttribution: thedavidmeister commentedMoved #1843764 to finished/unverified.
Comment #22.1
mr.baileysUpdated issue summary.
Comment #23
joelpittetSame as #19 with the ThemeTest.php using twig instead of php template.
Comment #23.0
joelpittetUpdated issue summary.
Comment #23.1
geoffreyr CreditAttribution: geoffreyr commentedMoving #1843748 to finished/unverified
Comment #23.2
Fabianx CreditAttribution: Fabianx commentedmoved block back to tasks
Comment #24
star-szrThanks @joelpittet, green again!
New Twig contributor task available:
We need help giving everyone who has worked on these issues credit - in the form of commit mentions. At this point I think we are not totally sure whether the patches will get committed individually in their own issues or all at once as a 'megapatch'.
Either way, I think we can update the issue summaries on each sub-issue to include/update a commit message with the names of everyone who worked on the issues. Some sub-issues (example: #1898460: taxonomy.module - Convert PHPTemplate templates to Twig) have a commit message already at the top but those are probably very outdated.
Can one or more people take on this task? The sooner the better :)
Hint: If you install Dreditor you will get a 'Create commit message' on the comment form that will help pull in people who have worked on patches. Although IMO we should be crediting all the hardworking profilers as well!
Comment #24.0
star-szrMoving 1843762 to alexpott's queue.
Comment #25
joelpittetI second crediting the profilers. ^^
Comment #25.0
joelpittetMove up views-view-field
Comment #25.1
star-szrMove up layout
Comment #25.2
star-szrUpdated issue summary.
Comment #25.3
star-szrMove views-view-fields down to Fabian section
Comment #26
star-szr#23: 1987510-22-tpl-files.patch queued for re-testing.
Comment #26.0
star-szrMove up up up
Comment #26.1
star-szrUpdated issue summary.
Comment #26.2
geoffreyr CreditAttribution: geoffreyr commentedAdded #1898474 to finished/unverified
Comment #26.3
star-szrMove Fabian issues back for re-profiling
Comment #26.4
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #26.5
cweagansMoving another issue up
Comment #26.6
star-szrRemoving pager.inc from this issue - it does not belong here, please do not add theme function conversions to this meta.
Comment #26.7
star-szrMove forum up
Comment #26.8
star-szrMove up system
Comment #26.9
star-szrMove up bartik
Comment #26.10
star-szrUpdated issue summary.
Comment #26.11
star-szrUpdated issue summary.
Comment #26.12
star-szrUpdated issue summary.
Comment #26.13
star-szrUpdated issue summary.
Comment #26.14
star-szrUpdated issue summary.
Comment #26.15
star-szrUpdated issue summary.
Comment #26.16
star-szrUpdated issue summary.
Comment #26.17
Fabianx CreditAttribution: Fabianx commentedMove block up to alexpott section
Comment #26.18
star-szrMove up then sleep
Comment #26.19
Fabianx CreditAttribution: Fabianx commentedmove DEC to alexpott
Comment #26.20
geoffreyr CreditAttribution: geoffreyr commentedAdded performance followups section with first issue.
Comment #26.21
geoffreyr CreditAttribution: geoffreyr commentedAdded #2002104 to performance followups
Comment #26.22
geoffreyr CreditAttribution: geoffreyr commentedAdded #2002108 to performance followups
Comment #27
joelpittetThese 4 scenarios
Using stark theme:
Comment #27.0
joelpittetUpdated issue summary.
Comment #28
geoffreyr CreditAttribution: geoffreyr commentedI can take scenario 1: Standard frontpage view with 10 nodes.
Comment #28.0
joelpittetAdded #2002222 to performance followups
Comment #29
geoffreyr CreditAttribution: geoffreyr commentedScenario 1 with Stark.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ead70538d1&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ead70538d1&...
Comment #30
joelpittetScenario 1
10 nodes in Stark
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ead03a5283&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ead03a5283&...
10 nodes in Bartik
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519eb193b2faa&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519eb193b2faa&...
Comment #30.0
joelpittetAdded performance follow-up.
Comment #30.1
star-szrUpdated issue summary.
Comment #30.2
star-szrMove up alexpotted issues :)
Comment #30.3
star-szrUpdated issue summary.
Comment #31
geoffreyr CreditAttribution: geoffreyr commentedScenario 3 with Stark.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519eb486908e7&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519eb486908e7&...
Comment #32
geoffreyr CreditAttribution: geoffreyr commentedScenario 3 with Bartik. This is really weird, the function call number and memory usage is exactly the same as S3 with Stark. I'm quite sure I was using Bartik for this run.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519eb7a1537f7&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519eb7a1537f7&...
Comment #33
joelpittetScenario 2
50 threaded comments on a node in Bartik
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ec8682c2dc&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ec8682c2dc&...
50 flat comments on a node in Stark
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ede3a9dd65&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ede3a9dd65&...
Comment #34
star-szrNumbers are looking good for profiling, no real surprises. Yes comment is a bit slow but we have #2002094: Improve performance of comment.html.twig.
Here is a new megapatch with the recent changes from the sub-issues here.
Comment #34.0
star-szrUpdated issue summary.
Comment #34.1
Fabianx CreditAttribution: Fabianx commentedMoved patches up to alex section
Comment #35
bradwade CreditAttribution: bradwade commentedScenario 4
Maintenance Page (using Stark theme)
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fbf2154a5e&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fbf2154a5e&...
Comment #36
steveoliver CreditAttribution: steveoliver commentedAll template conversion patches have been committed. This issue is closed. :)
Comment #37.0
(not verified) CreditAttribution: commentedx