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.
Task
Use Twig instead of PHPTemplate
Remaining
- Replace all tpl.php files with .html.twig equivalents
- Replace all theme functions with .html.twig equivalent templates
- Add new preprocess functions for the .html.twig equivalent templates
- Update all hook_theme definitions
Testing Steps:
Found in /admin/structure/views
Related Issues
#1898472: [meta] Convert views_ui module to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
Comment | File | Size | Author |
---|---|---|---|
#38 | 1963986-38.patch | 2.34 KB | chrisjlee |
#38 | interdiff-1963986-38.txt | 488 bytes | chrisjlee |
#32 | 1963986-32.patch | 2.37 KB | waynethayer |
#22 | 1963986-reroll-patch13-22.patch | 2.36 KB | chrisjlee |
#17 | 1963986-reroll-patch13-17.patch | 2.38 KB | chrisjlee |
Comments
Comment #1
star-szrTagging.
Comment #2
joelpittetmeta split.
Comment #4
star-szr#2: 1918648-2-twig-views-ui-view-info.patch queued for re-testing.
Comment #5
thedavidmeister CreditAttribution: thedavidmeister commentedReview for #2:
+ * - view: The view object.
for basic administrative info about a view.
+ * Prepares variables for Views UI view info templates.
Capital "V" for a View when it's an object.
Other than that, this looks good to me.
We need manual review steps written in the issue summary and we need somebody to manually review the markup.
Comment #6
star-szrCreating a revised patch and interdiff based on #5 would be a good novice task, tagging.
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 #7
thedavidmeister CreditAttribution: thedavidmeister commentedManual testing the markup is a good novice task too!
Comment #8
designesse CreditAttribution: designesse commentedfixing capital v addressed in #5.
Comment #9
disasm CreditAttribution: disasm commentedSorry, but I think I was mistaken on this at the ladder meeting tonight. I think the "The" is unneeded and the view we capitalized should be lowercase, with the description View being uppercase. Can you change this to view: View object.
Comment #10
designesse CreditAttribution: designesse commentedfixing capital v addressed in #5 and #9.
Comment #11
joelpittetThank you @designesse! Just removing novice tag and changing this to needs review for the testbot to kick in. This one should be fairly simple one to do Manual Testing on.
Comment #12
thedavidmeister CreditAttribution: thedavidmeister commentedAaah, sorry to nitpick a nitpick but
+ * - view: View object.
is "The View object." in almost every other Views conversion patch being reviewed currently.Comment #13
joelpittetI kind of feel responsible, I saw that comment and didn't speak up:(
Maybe this patch will make up for it? I uppercased a couple other "View" caps. Please check if those make sense...
Comment #14
Fabianx CreditAttribution: Fabianx commented#13: 1963986-13-twig-views-ui-view-info.patch queued for re-testing.
Comment #16
star-szrTagging for reroll, the Views UI module got moved to core/modules in #1820414: CHANGE NOTICE: Move views_ui.module directly into /core/modules/.
Comment #17
chrisjlee CreditAttribution: chrisjlee commentedReroll attempt... luckily no merge conflicts.
Comment #18
chrisjlee CreditAttribution: chrisjlee commentedComment #20
chrisjlee CreditAttribution: chrisjlee commented#17: 1963986-reroll-patch13-17.patch queued for re-testing.
Comment #21
star-szrThanks @chrisjlee! The template will need to be moved to core/modules/views_ui/templates (right now it's in core/modules/views/views_ui/templates).
Comment #22
chrisjlee CreditAttribution: chrisjlee commentedAh whoops.
Comment #23
StryKaizerpatch in #22 passed my manual testing and daisydiffing.
Comment #24
thedavidmeister CreditAttribution: thedavidmeister commentedComment #25
carsonevans CreditAttribution: carsonevans commentedI will profile
Comment #26
carsonevans CreditAttribution: carsonevans commentedbbranches 519ffe6e4d3f0 1963986-reroll-patch13-22.patch
=== 8.x..8.x compared (519ffe6e4d3f0..519ffece8bb60):
ct : 40,655|40,655|0|0.0%
wt : 453,036|453,638|602|0.1%
cpu : 438,023|438,391|368|0.1%
mu : 40,715,592|40,715,592|0|0.0%
pmu : 40,844,736|40,844,736|0|0.0%
Profiler output
=== 8.x..1963986-reroll-patch13-22.patch compared (519ffe6e4d3f0..519ffee50146c):
ct : 40,655|40,655|0|0.0%
wt : 453,036|454,363|1,327|0.3%
cpu : 438,023|438,242|219|0.0%
mu : 40,715,592|40,716,144|552|0.0%
pmu : 40,844,736|40,844,048|-688|-0.0%
Profiler output
---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
Comment #27
carsonevans CreditAttribution: carsonevans commentedRemoving "needs profiling" tag
Comment #28
carsonevans CreditAttribution: carsonevans commentedMy profiling was flawed, adding the needs profiling tag back on so someone else can tackle it.
Comment #29
joelpittetUnassigning @StryKaizer, though you are more than welcome to profile this patch.
Here's some instructions if you want to give it a crack:
https://drupal.org/contributor-tasks/profiling
Comment #30
YesCT CreditAttribution: YesCT commentedin #5 @thedavidmeister asked
seems like @StryKaizer might have done manual testing in #23...
but the steps to test were not added to the issue summary.
Putting the steps to test in the issue summary makes it a lot easier for people to repeat the testing in the future. https://drupal.org/contributor-tasks/add-steps-to-reproduce explains more. :)
[edit:]
maybe an interdiff would have been nice for the concern brought up in #21 https://drupal.org/documentation/git/interdiff
Actually I was thinking on this more and, maybe an interdiff wasn't there because it seemed to not make sense for moving a file. There are git config settings that make git show renames of files in a nice way.
I put
in my .gitconfig file to help with git diffs on a issue I was working on a while back that was moving files around. I'm not sure if that would have helped...
Comment #31
joelpittetHmm, @YesCT just noticed that we didn't actually post our steps for manual testing. @StryKaizer do you mind posting your manual testing steps in the issue summary?
And whomever picks up the profiling bit, writing your steps out in the Issue Summary would really help that as well.
Comment #32
waynethayer CreditAttribution: waynethayer commentedRerolled patch as it no longer applies.
Will manual test to confirm results.
Comment #32.0
waynethayer CreditAttribution: waynethayer commentedRemove sandbox link
Comment #33
waynethayer CreditAttribution: waynethayer commentedYou can see this on admin/structure/views. I've added a step to Issue Summary.
Comment #34
waynethayer CreditAttribution: waynethayer commentedManual testing checks out. :)
Comment #36
waynethayer CreditAttribution: waynethayer commented#32: 1963986-32.patch queued for re-testing.
Comment #37
dawehnerAfaik we remove this references now.
Comment #38
chrisjlee CreditAttribution: chrisjlee commentedRemoved @template_preprocess from template as per #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file and as requested by dahwehner in #37.
Comment #40
joelpittet#38: 1963986-38.patch queued for re-testing.
Comment #41
joelpittetprofiling...
Comment #42
joelpittetScenario:
Theme: Stark
URL: http://d8prof.dev/admin/structure/views/
Permissions: Views UI: Administer views
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51e2215579f70&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51e2215579f70&...
Comment #43
joelpittetun-tagging and RTBC.
Comment #44
alexpottCommitted 3bc754a and pushed to 8.x. Thanks!
Comment #45.0
(not verified) CreditAttribution: commentedAdded testing step.