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 #1898444 by jenlampton, likin, Cottser, lbainbridge, duellj, joelpittet, Stefan Freudenberg, scor | c4rl: rdf.module - Convert theme_ functions to Twig.
Task
Use Twig instead of PHPTemplate
Remaining
Theme function name/template path | Conversion status |
---|---|
theme_rdf_metadata | converted |
theme_rdf_template_variable_wrapper | converted |
Related
#1757550: [Meta] Convert core theme functions to Twig templates
Comment | File | Size | Author |
---|---|---|---|
#51 | 1898444-51-twig-rdf-theme.patch | 2.9 KB | joelpittet |
#51 | interdiff.txt | 675 bytes | joelpittet |
#50 | twig-convert_rdf-1898444-50.patch | 2.93 KB | jenlampton |
#45 | 1898444-45.patch | 8.07 KB | scor |
#45 | interdiff.txt | 3.07 KB | scor |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedFeel free to ping at any time if there are questions or if this needs reviews.
Comment #3
duellj CreditAttribution: duellj commentedAttached patch converts rdf_metadata and rdf_template_variable_wrapper theme calls to twig. There was no rdf_template_variable_wrapper twig file in the sandbox, so I created one.
Comment #4
star-szrThe docblocks for the preprocess functions will need to be updated at some point but it's too soon to say exactly how, so they can probably be left for now. See #1913208: [policy] Standardize template preprocess function documentation.
These should end in parens i.e. template_preprocess() per http://drupal.org/node/1354#see.
Otherwise looks pretty good to me, thanks @duellj!
Comment #5
duellj CreditAttribution: duellj commentedThanks for the review @Cottser! I've updated the @see references. Watching #1913208: [policy] Standardize template preprocess function documentation to see what happens.
Comment #7
star-szr#5: 1898444-5-twig-rdf.patch queued for re-testing.
Comment #8
jenlamptonA few other changes:
- renamed
rdf_attributes
toattributes
- added a missing space before a closing
}}
- updated docs to reference
.html.twig
files instead of.tpl.php
(maybe premature?)- removed all dollar signs ($) from docs and instead surrounded variable names with single quotes
- removed references to variable types in docblock
- removed any mention of theme functions in docblock
- trimmed down the docs in templates, and restored docs on preprocess functions.
- updated preprocess docs to match new standards :)
Looking good. Test bot?
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedThis looks good to me. FYI, this whole part of the system will likely (or at least hopefully) be revisited to improve our RDFa output.
There are two whitespace issues in the last chunk, but other than that I'd say it's RTBC.
Comment #10
jenlamptonRemoved whitespace for final review.
We're going to be removing the entire process layer eventually in favor of having Twig drpal_render() all the pieces instead. I don't think that will be a huge problem for RDF since we're mostly talking about Attributes() here anyway, but we'll definitely need to triple check the output when that's done. If you're refactoring as well for better markup we'll just need to quadruple check!
Comment #11
star-szrAlmost… :)
One more bit of whitespace here.
Comment #13
star-szrTestbot is having a rough day. I'll do a quick reroll for the whitespace tonight.
Comment #14
star-szrComment #16
jenlampton#14: 1898444-14.patch queued for re-testing.
Comment #17
scor CreditAttribution: scor commentedGreat work here, and good to see progress on Twig.
why do we need a special case for class here? Isn't Twig going to treat it like any regular attribute and output it if it's present? in other word, why can't we use:
Comment #18
jenlampton@Scor,
great feedback. The short answer is: we can!
In general, we want to separate out the class from the rest of the attributes in places where we think front-end devs will want to add a class to something. If you don't think this is a place anyone will theme override, or they won't be doing it to add a class (which - now that you bring it up, I think is the exact case) then we should just print all the attributes at once.
The reason for separating them out at all is that 1) front end devs will instantly recognize
class=""
in the markup, and 2) won't need to consult Drupal documentation to do something as seemingly straightforward as adding a class.That said, attached patch does not treat classes special here. (and slightly clarifies docblock)
Comment #19
scor CreditAttribution: scor commentedThe class attribute wasn't explicitly pulled before Twig when we were using drupal_attributes(), so I don't see a need to pull it out now, so you patch looks good. template_preprocess_rdf_metadata() actually sets a default class 'rdf-meta' automatically, so that class is there for themes if they need to target this element for some reason. There would still be a way for themers to alter the class attribute I imagine through some kind of preprocessing function, or via overriding rdf-metadata.html.twig. I'm assuming both these operations are/will still be possible with Twig?
whitespaces are back! (3 lines)
Comment #20
star-szrRerolled to remove whitespace and added a commit message to the issue summary based on git blame and sandbox issues. If I missed anyone, please edit the issue summary.
Comment #21
scor CreditAttribution: scor commentedthanks, #20 looks great.
Comment #22
xjm#20: 1898444-20.patch queued for re-testing.
Comment #23
nikkubhai CreditAttribution: nikkubhai commented#20: 1898444-20.patch queued for re-testing.
Comment #23.0
nikkubhai CreditAttribution: nikkubhai commentedAdd commit message
Comment #24
star-szrFound some minor documentation and code style things that can be fixed up, then back to RTBC. Making these changes would be a good novice task, tagging.
These should start with 'Default theme implementation…' per http://drupal.org/node/1823416#docblock and the second one should be under 80 characters per http://drupal.org/node/1354#file. If need be, more description can be added in a separate paragraph after the summary line.
The {{ attributes }} should not have a space before it, see http://drupal.org/node/1823416#attributes.
Comment #25
star-szrLet's remove the
@ingroup rdf
lines from the templates as well, I don't see why RDF is a special case here, all other templates have just @ingroup themeable.Comment #26
lbainbridge CreditAttribution: lbainbridge commentedWorking on this currently, will post more later
Comment #27
lbainbridge CreditAttribution: lbainbridge commentedOk I incorporated the feedback from comments 24/25, just a little cleanup.
Comment #28
lbainbridge CreditAttribution: lbainbridge commentedMissed some documentation, will revise later
Comment #29
joelpittetNice work cleaning that up! You got some ending whitespace in there. Depending on your ide/editor there may be an option to trim trailing whitespace on save. Keep it up!
Comment #30
lbainbridge CreditAttribution: lbainbridge commentedOk, I cleaned up the whitespace and trimmed the documentation even further to keep with the coding standards (http://drupal.org/node/1354#file). Also fixed some indentation as per http://drupal.org/node/1823416#expressions.
Comment #32
star-szrLooks like a random test failure. I worked on this patch with @lbainbridge, but looking at the docs again…
I think this should say 'the content is wrapped' instead of 'wrap the content'. We could also consider adding something like "If not, the content is left as is."
Comment #33
star-szrSomething like this?
Attached an interdiff from #20 as well (previous RTBC).
Comment #34
jenlamptonJust gave this one a quick re-test, still works, new docs look great!
Comment #34.0
jenlamptonAdd conversion summary table
Comment #34.1
star-szrUpdate commit message
Comment #35
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 #36
alexpottComment #37
minneapolisdan CreditAttribution: minneapolisdan commented#33: 1898444-33.patch queued for re-testing.
Comment #38
scor CreditAttribution: scor commentedwill take on profiling this one.
Comment #39
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedhttp://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fc956834f6&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fc956834f6&...
Comment #40
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedThe previous profile did not use theme_rdf_metadata. Here's results for a page that executes both theme functions.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ffa0e6cdec&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ffa0e6cdec&...
Comment #40.0
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedRemove sandbox link
Comment #41
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedaccidental post
Comment #42
OpenChimp CreditAttribution: OpenChimp commentedprofiling done
Comment #43
scor CreditAttribution: scor commentedCreated a node with a comment and made it the front pagein for both rdf_template_variable_wrapper and rdf_metadata Twig templates to be rendered.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ff693b4ca2&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ff693b4ca2&...
Comment #44
joelpittetI am almost positive we don't need to do this due to https://drupal.org/node/1982024
If so this should be slightly faster and we can remove the preprocess, don't forget to remove the @see in the twig template too.
Comment #45
scor CreditAttribution: scor commented@joelpittet you're right, this approach seems to work. perf++.
Comment #45.0
scor CreditAttribution: scor commentedUpdated issue summary.
Comment #46
scor CreditAttribution: scor commentedpostponing, see #1941286-13: Remove the process layer (rdf module).
Comment #47
joelpittetunpostponing as the process layer is out of there. Needs a re-roll.
Comment #48
joelpittet#45: 1898444-45.patch queued for re-testing.
Comment #50
jenlamptonrerolled.
Comment #51
joelpittetThanks for the re-roll @jenlampton!
Removed this as per #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file
This is ready to go and has been profiled in #39 #40 and #43
Anything else or RTBC?
Comment #52
star-szrI would actually like to see this re-profiled first since things have changed. Other than that looks great!
Comment #53
joelpittet#51: 1898444-51-twig-rdf-theme.patch queued for re-testing.
Comment #53.0
joelpittetCapitalization in commit message
Comment #54
joelpittet51: 1898444-51-twig-rdf-theme.patch queued for re-testing.
Comment #55
joelpittetSame Scenario as #43 except 5 comments.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52907577b0b08&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52907577b0b08&...
Comment #56
star-szrI think this one is ready to go!
I manually tested the patch once more and this still checks out. Profiling results look reasonable to me and I think we made it faster because #43 tested 1 comment and #55 tested 5 comments.
Comment #57
alexpott51: 1898444-51-twig-rdf-theme.patch queued for re-testing.
Comment #58
xjm51: 1898444-51-twig-rdf-theme.patch queued for re-testing.
Comment #59
alexpottCommitted 360c0c0 and pushed to 8.x. Thanks!