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 #1898052 by jenlampton, taslett, Cottser, Pierre Paul Lefebvre, c4rl: Convert color module to Twig.
Task
Use Twig instead of PHPTemplate
Remaining
Theme function name/template path | Conversion status |
---|---|
theme_color_scheme_form | converted |
To test
To test this code, load the color form at admin/appearance/settings/bartik
Related
#1757550: [Meta] Convert core theme functions to Twig templates
Comment | File | Size | Author |
---|---|---|---|
#40 | 1898052-40.patch | 3.06 KB | star-szr |
#40 | interdiff.txt | 740 bytes | star-szr |
#39 | 1898052-39-twig-color-theme.patch | 3.06 KB | joelpittet |
#39 | interdiff.txt | 1.03 KB | joelpittet |
#36 | 1898052-36.patch | 3.05 KB | star-szr |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
c4rl CreditAttribution: c4rl commentedTagging
Comment #3
c4rl CreditAttribution: c4rl commentedTagging
Comment #4
c4rl CreditAttribution: c4rl commentedSorry, I hit submit a few extra times there :)
Comment #5
taslett CreditAttribution: taslett commentedComment #6
taslett CreditAttribution: taslett commentedComment #7
Fabianx CreditAttribution: Fabianx commentedThe drupal_render is not needed.
The drupal_render_children() might not be needed, might need to add a:
#theme => 'children'
currently discussing within #drupal-twig.
---
The last one is fine, because it is always printed and big part of the template.
Comment #8
taslett CreditAttribution: taslett commentedThe $form has an empty #children element, in Bartik at least.
Should we be using:
Or can we remove children from the template?
Comment #9
star-szrThanks for your work on this, @taslett!
Some newly created documentation here on Twig preprocess best practices: http://drupal.org/node/1920746
As @Fabianx mentioned, you should be able to just remove those
twothreedrupal_render()
calls, we're working on the best way to deal withdrupal_render_children()
in #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead so that can be left for now.This @todo should probably stay unless we have a reason for removing it.
One other thing I noticed, color-scheme-form.html.twig needs a newline at the end of the file, spotted with Dreditor.
Comment #10
taslett CreditAttribution: taslett commentedThanks Cottser,
I have re-added the comment, fixed the drupal_render_children call, and added the newline to the twig template file.
Comment #11
star-szrGreat, thanks @taslett! I see a couple more minor tweaks that can be done.
Some trailing whitespace was introduced here in the latest patch. If you can configure your editor/IDE to automatically remove these it'll make your life much easier :)
I didn't notice this one before, this comment should end in a period per http://drupal.org/node/1354#drupal.
Comment #12
taslett CreditAttribution: taslett commentedComment #13
jenlamptonThanks for your work on this taslett! A few more things:
1) We need to remove all mention of data types in the docs at the top of template files. Anyone working in these files doesn't need to know if something is an object, array, or string anymore, all they need to do is know it's name and what it is, so they can print it.
2) Giving people a variable named 'rows' also doesn't help them understand what's inside. Let's name the variable in a way where they understand what is being printed. The docs at the top of the template call these colors, so let's stick with that :)
Here's a go at an updated template file with more verbose docs:
I also ran into some problems related to #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead that was causing the whole form to be rendered inside itself (though not infinitely). I've made some changes in the preprocess function to stop this, but it will need to be reworked later. (Marked clearly with @todos)
Comment #14
star-szrMostly docs updates here. Reformatted preprocess docs per the pending standards and removed a few comments from the preprocess function, same idea as the "Set the template variables" comments here http://drupal.org/node/1898428#comment-7106282.
Comment #16
star-szr#14: 1898052-14.patch queued for re-testing.
Comment #18
star-szr#14: 1898052-14.patch queued for re-testing.
Comment #18.0
star-szradded a To Test section
Comment #19
star-szrAdded a commit message to the issue summary, I wasn't able to find any issues in the sandbox and git blame says credit goes to @jenlampton on this one. If I'm missing anyone, please edit.
Comment #20
jenlamptonThis one looks great to me! Thanks for the docs cleanup @Cottser :)
Comment #21
Fabianx CreditAttribution: Fabianx commented+1 for RTBC, please get this in.
Comment #22
xjm#14: 1898052-14.patch queued for re-testing.
Comment #23
nikkubhai CreditAttribution: nikkubhai commented#14: 1898052-14.patch queued for re-testing.
Comment #23.0
nikkubhai CreditAttribution: nikkubhai commentedAdd commit message
Comment #23.1
star-szrAdd conversion summary table
Comment #24
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.
Comment #25
alexpottComment #26
epersonae2 CreditAttribution: epersonae2 commented#14: 1898052-14.patch queued for re-testing.
Comment #26.0
epersonae2 CreditAttribution: epersonae2 commentedRemove sandbox link
Comment #27
Pierre Paul Lefebvre CreditAttribution: Pierre Paul Lefebvre commented--- Edit : bad scenario, will redo it.
Comment #28
Pierre Paul Lefebvre CreditAttribution: Pierre Paul Lefebvre commentedScenario
1 - Put admin/appearance/settings/bartik as homepage
2 - Gives anonymous permission to administer theme settings
Settings
Twig cache on
APC class loader on
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fe6587b366&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fe6587b366&...
Comment #28.0
Pierre Paul Lefebvre CreditAttribution: Pierre Paul Lefebvre commentedAdded reference to profiling tag to Remaining section
Comment #29
tlattimore CreditAttribution: tlattimore commentedThis @todo needs to be addressed? #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead is marked fixed.
Same
Comment #30
joelpittet@tlattimore removed the drupal_render_children bit.
@Pierre Paul Lefebvre thank you for the profiling!
Comment #30.0
joelpittetUpdated attribution.
Comment #31
star-szrSimplified preprocess a bit more and reprofiled.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51aad15cb89bb&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51aad15cb89bb&...
Comment #32
star-szrDid markup comparison again via visual diff and DaisyDiff and this still checks out.
Comment #33
joelpittetI tried to do that on scheme and palette but it didn't seem to fly, I see you got one of them done, nice work:)
Comment #34
star-szrDon't know why I didn't think of this before.
Comment #35
star-szrOops, two interdiffs.
Comment #36
star-szrIt's late ok? :)
Comment #37
star-szrRe-ran the markup comparison, #36 still looks good. Here are more profiling results. Less function calls!
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51aaed0fcbd4e&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51aaed0fcbd4e&...
Comment #38
joelpittetNice work Scott! I had a look at it and it works great.
Nitpicks in docs and I'd say this is RTBC.
Formatting the second line should be indented by two more spaces no? Also need to add form.palette
Comment #39
joelpittetHa, well I guess I RTBC'd it already. Here's the patch:S You can re-RTBC if the comment looks good.
Comment #40
star-szrYeah the indent was my mistake and I forgot to update the colors/palette docs, thanks @joelpittet! Attached patch makes one more docs tweak, most notably adding the trailing colon before a sublist per http://drupal.org/node/1354#lists.
Comment #41
joelpittetAnd RTBC again.
Comment #42
alexpottCommitted 1e0200d and pushed to 8.x. Thanks!
Comment #43.0
(not verified) CreditAttribution: commentedRemove profiling from remaining steps