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.
Issue #1843774 by joelpittet, chrisjlee, steveoliver, shanethehat | tostinni: Convert views-ui-display-tab-setting.tpl.php to Twig.
Meta issue: #1898472: [meta] Convert views_ui module to Twig
#1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Profiling results
=== 8.x..8.x compared (5198ed6acdf41..5198eec546553):
ct : 61,422|61,422|0|0.0%
wt : 317,151|316,951|-200|-0.1%
cpu : 277,465|277,365|-100|-0.0%
mu : 16,452,000|16,452,000|0|0.0%
pmu : 16,710,184|16,710,184|0|0.0%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198ed6acdf41&...
=== 8.x..views-ui-display-tab-setting-1843774-53 compared (5198ed6acdf41..5198eef80c6dc):
ct : 61,422|62,574|1,152|1.9%
wt : 317,151|318,977|1,826|0.6%
cpu : 277,465|281,875|4,410|1.6%
mu : 16,452,000|16,477,224|25,224|0.2%
pmu : 16,710,184|16,742,272|32,088|0.2%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198ed6acdf41&...
Comment | File | Size | Author |
---|---|---|---|
#53 | twig-views-ui-display-tab-setting-1843774-53.patch | 3.93 KB | shanethehat |
#50 | interdiff.txt | 1.19 KB | joelpittet |
#50 | 1843774-50-twig-views-ui-display-tab-setting.patch | 3.87 KB | joelpittet |
#47 | 1843774-47-twig-views-ui-display-tab-setting.patch | 3.86 KB | joelpittet |
#47 | interdiff.txt | 1.51 KB | joelpittet |
Comments
Comment #1
joelpittetfirst draft
Comment #2
mbrett5062 CreditAttribution: mbrett5062 commentedTagging.
Comment #3
jpamental CreditAttribution: jpamental commentedApplies cleanly, displays properly. Gets description, 'zebra' and settings links as needed.
Comment #4
dawehnerBack to needs review due to missing documentation of variables, sorry.
Comment #5
steveoliver CreditAttribution: steveoliver commentedThis could use some logic out of the template and into + cleaned up in preprocess. Assigning to myself. Patch coming up.
Comment #6
steveoliver CreditAttribution: steveoliver commentedComment #7
steveoliver CreditAttribution: steveoliver commentedMoving to Core queue.
Comment #8
joelpittetAdding twig tag
Comment #9
star-szr#6: drupal-views-ui-display-tab--1843774-6.patch queued for re-testing.
Comment #11
webchick#6: drupal-views-ui-display-tab--1843774-6.patch queued for re-testing.
Comment #12
dawehnerThanks for working on that one!
I will hopefully review all twig issues.
Just wondering why we need the two "-" here.
Couldn't we just use http://twig.sensiolabs.org/doc/filters/join.html for that? This seems to be the best way to simplify this difficult logic.
Comment #13
star-szrTagging.
Comment #14
thedavidmeister CreditAttribution: thedavidmeister commentedCode standards review of #6:
- We're not talking about PHP data types in Twig template documentation.
- Don't mix attributes.class with hardcoded classes like "views-display-setting" in a Twig template if it can be avoided, move these into the preprocess.
Not quite right for the preprocess function docs, see #1913208: [policy] Standardize template preprocess function documentation. The style is wrong and this is missing @param.
- I like the idea of using join if we can as suggested in #12. Somebody should try that out...
Comment #15
star-szrThanks for reviewing @thedavidmeister. Tagging as novice for the tweaks in #14, but I have a feeling we're not going to be able to solve attributes.class as a whole during conversion, IMO it can be left as is:
Comment #16
joelpittetDocs update and join added.
Comment #17
shrop CreditAttribution: shrop commentedThe markup before and after patch in #16 match. The Twig version works as expected in my manual testing.
Before:
After:
Comment #18
thedavidmeister CreditAttribution: thedavidmeister commentedLooks good to me. We'll want to clean up the @todos and the zebra striping as part of followup issues later.
Comment #19
dawehnerShould be default ... see twig standard.
Comment #20
thedavidmeister CreditAttribution: thedavidmeister commentedComment #21
joelpittetMoved the class attributes to the preprocess, hopefully making it slightly easier to cleanup and drop the zebra stuff later.
Also fixed some documentation pieces, but not sure if I got them all correct. Please have a critical eye:)
Comment #22
dawehnerAre you sure about this change?
Comment #23
joelpittet@dawehner re #22 Yes, I am positive.
It's a mistake everywhere it exists:
Comment #24
dawehnerWell I would say it's used by ViewEditFormController::buildOptionForm().
Comment #25
joelpittet@dawehner could you point it out explicitly? I can't see anything referring to `attributes_array` in that method, building that key or using it.
Comment #26
dawehnerSee
Comment #27
joelpittet@dawehner I think I see what you're are saying... I didn't remove #overridden, but I removed the unused 'title'. Maybe that title should be there and it's just a bug in Views?
Should I change it to the following?
Comment #28
dawehnerOH I see, yeah use the proper key. Thank you!
Comment #29
joelpittetComment #30
joelpittet#29 should do it.
Comment #32
dawehner#29: 1843774-29-twig-views-ui-display-tab-setting.patch queued for re-testing.
Comment #34
joelpittetfix for #23 at #1484704: Remove instances of attributes_array needs re-roll once that get in.
Comment #35
Fabianx CreditAttribution: Fabianx commented#29: 1843774-29-twig-views-ui-display-tab-setting.patch queued for re-testing.
Comment #37
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 #37.0
star-szrUpdate to point to right meta issue
Comment #38
chrisjlee CreditAttribution: chrisjlee commentedthis one had a "added by us" merge conflict. I'm guessing the tpl file isn't needed anymore. So i created a rerolled patch without the tpl file.
Comment #40
chrisjlee CreditAttribution: chrisjlee commentedpatch placed the template files in the wrong folder. new patch reflects this change.
Comment #41
star-szrNew patch needs to remove the .tpl.php template please :)
Comment #42
chrisjlee CreditAttribution: chrisjlee commentedsure thing.
Comment #43
barraponto CreditAttribution: barraponto commentedCode seems ok to me. Two questions:
* why do we need non-breaking spaces there?
* why do we add
span
tags to the separator? the 'label' class is deceiving, since it is wrapping the separator, not the label... particularly, I rather pass the separator as a clean preprocessed variable with a default value of' | '
.Comment #44
star-szr@barraponto - thanks for reviewing! In my opinion those points are out of scope for a straight conversion - it's not the conversion's job to make decisions like removing
from markup. The conversions are meant to keep markup as close as possible to the pre-conversion state. Please open another issue if you'd like to change the markup. Thanks!Comment #45
thedavidmeister CreditAttribution: thedavidmeister commentedSorry to nitpick but:
Should link be "The settings link" or should description be "The main setting's description" (note the inconsistent apostrophes affecting interpretation). What does the word "main" mean in this context anyway?
Should be "settings description" or "setting's description", depending on the above.
These descriptions imply to me that (defaulted = !overidden) and nothing else. Is this really the case? it seems weird that there would be redundant variables being passed in.
"An option"
Comment #46
star-szrTagging for profiling.
Comment #47
joelpittet@thedavidmeister the "main" is referring to primary so I changed to hopefully make that more clear and it's not the main setting, it's the main link in the setting.
Can't do much about the overridden!=defaulted bit, maybe you can open up an issue on that note?
Comment #48
thedavidmeister CreditAttribution: thedavidmeister commented* - defaulted: A boolean indicating the setting is in it's default state.
"its" not "it's". Whoever first wrote these docs got the meaning of apostrophes backwards :/ sorry that I missed this last time.
Is this a boolean? if it is, could we make this part of the comment consistent with the rest, eg. "A boolean indicating XXX,
Comment #49
thedavidmeister CreditAttribution: thedavidmeister commented#1993512: template_preprocess_views_ui_display_tab_setting() doesn't need two variables that mean the same thing. as per #47.
Comment #50
joelpittetWas probably me, I know the difference between its and it's but my fingers refuse to obey my mind:P
Comment #51
thedavidmeister CreditAttribution: thedavidmeister commentedcode looks clean and was manually tested in #17
Comment #52
star-szrThis one needs a reroll, likely from #1938430: Don't add a default theme hook class in template_preprocess().
Comment #53
shanethehat CreditAttribution: shanethehat commentedReroll
Comment #54
thedavidmeister CreditAttribution: thedavidmeister commentedComment #55
star-szrThanks @shanethehat, back to RTBC!
Comment #56
star-szrDibs on this for profiling.
Comment #57
star-szrProfiling results for editing frontpage view:
/admin/structure/views/view/frontpage/edit
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198ed6acdf41&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198ed6acdf41&...
Comment #57.0
star-szrUpdated issue summary.
Comment #58
alexpottBeing that the this is view ui conversion I think the performance regression here is okay.
+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #58.0
alexpottAdd profiling results
Comment #59
alexpottCommitted f202b56 and pushed to 8.x. Thanks!
Comment #60.0
(not verified) CreditAttribution: commentedUpdated issue summary -- add git commit message.