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

#1898472: [meta] Convert views_ui module to Twig
#1757550: [Meta] Convert core theme functions to Twig templates

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Issue tags: +Needs manual testing

Tagging.

joelpittet’s picture

Status: Active » Needs review
FileSize
2.43 KB

meta split.

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -Twig, -VDC

The last submitted patch, 1918648-2-twig-views-ui-view-info.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +Twig, +VDC
thedavidmeister’s picture

Status: Needs review » Needs work

Review 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.

star-szr’s picture

Issue tags: +Novice

Creating 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!

thedavidmeister’s picture

Manual testing the markup is a good novice task too!

designesse’s picture

Status: Needs work » Needs review
FileSize
591 bytes
2.43 KB

fixing capital v addressed in #5.

disasm’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/views_ui/views_ui.theme.incundefined
@@ -55,7 +55,13 @@ function template_preprocess_views_ui_display_tab_bucket(&$variables) {
+ *   - View: The view object.

Sorry, 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.

designesse’s picture

fixing capital v addressed in #5 and #9.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Novice

Thank 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.

thedavidmeister’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Aaah, sorry to nitpick a nitpick but + * - view: View object. is "The View object." in almost every other Views conversion patch being reviewed currently.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.45 KB
2.43 KB

I 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...

Fabianx’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +Needs manual testing, +Twig, +VDC

The last submitted patch, 1963986-13-twig-views-ui-view-info.patch, failed testing.

star-szr’s picture

Issue tags: +Needs reroll

Tagging for reroll, the Views UI module got moved to core/modules in #1820414: CHANGE NOTICE: Move views_ui.module directly into /core/modules/.

chrisjlee’s picture

Status: Needs work » Needs review
FileSize
2.38 KB

Reroll attempt... luckily no merge conflicts.

chrisjlee’s picture

Issue tags: -Needs reroll

Status: Needs review » Needs work
Issue tags: -Novice, -Needs manual testing, -Twig, -VDC

The last submitted patch, 1963986-reroll-patch13-17.patch, failed testing.

chrisjlee’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs manual testing, +Twig, +VDC

#17: 1963986-reroll-patch13-17.patch queued for re-testing.

star-szr’s picture

Status: Needs review » Needs work

Thanks @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).

chrisjlee’s picture

Status: Needs work » Needs review
FileSize
2.36 KB

Ah whoops.

StryKaizer’s picture

Assigned: Unassigned » StryKaizer
Issue tags: -Needs manual testing

patch in #22 passed my manual testing and daisydiffing.

thedavidmeister’s picture

Issue tags: -Novice +needs profiling
carsonevans’s picture

I will profile

carsonevans’s picture

bbranches 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

carsonevans’s picture

Issue tags: -needs profiling

Removing "needs profiling" tag

carsonevans’s picture

Issue tags: +needs profiling

My profiling was flawed, adding the needs profiling tag back on so someone else can tackle it.

joelpittet’s picture

Assigned: StryKaizer » Unassigned

Unassigning @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

YesCT’s picture

in #5 @thedavidmeister asked

We need manual review steps written in the issue summary and we need somebody to manually review the markup.

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

[diff]
  renames = copy

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...

joelpittet’s picture

Issue tags: +Needs manual testing

Hmm, @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.

waynethayer’s picture

FileSize
2.37 KB

Rerolled patch as it no longer applies.

Will manual test to confirm results.

waynethayer’s picture

Issue summary: View changes

Remove sandbox link

waynethayer’s picture

You can see this on admin/structure/views. I've added a step to Issue Summary.

waynethayer’s picture

Issue tags: -Needs manual testing

Manual testing checks out. :)

Status: Needs review » Needs work
Issue tags: -needs profiling, -Twig, -VDC

The last submitted patch, 1963986-32.patch, failed testing.

waynethayer’s picture

Status: Needs work » Needs review
Issue tags: +needs profiling, +Twig, +VDC

#32: 1963986-32.patch queued for re-testing.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/views_ui/templates/views-ui-view-info.html.twigundefined
@@ -0,0 +1,17 @@
+ * @see template_preprocess()

Afaik we remove this references now.

chrisjlee’s picture

Status: Needs work » Needs review
FileSize
488 bytes
2.34 KB

Removed @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.

Status: Needs review » Needs work
Issue tags: -needs profiling, -Twig, -VDC

The last submitted patch, 1963986-38.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: +needs profiling, +Twig, +VDC

#38: 1963986-38.patch queued for re-testing.

joelpittet’s picture

Assigned: Unassigned » joelpittet

profiling...

joelpittet’s picture

Scenario:
Theme: Stark
URL: http://d8prof.dev/admin/structure/views/
Permissions: Views UI: Administer views

=== 8.x..8.x compared (51e2215579f70..51e22193d5fd6):

ct  : 45,067|45,067|0|0.0%
wt  : 249,318|249,273|-45|-0.0%
cpu : 218,033|217,973|-60|-0.0%
mu  : 14,328,520|14,328,520|0|0.0%
pmu : 14,385,744|14,385,744|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51e2215579f70&...

=== 8.x..1963986-twig-views-ui-view-info compared (51e2215579f70..51e221c38f48f):

ct  : 45,067|45,421|354|0.8%
wt  : 249,318|249,165|-153|-0.1%
cpu : 218,033|219,060|1,027|0.5%
mu  : 14,328,520|14,353,096|24,576|0.2%
pmu : 14,385,744|14,411,504|25,760|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51e2215579f70&...

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -needs profiling

un-tagging and RTBC.

  • Profiling falls within margin of error.
  • Manually Tested.
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3bc754a and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Added testing step.