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
Twig sandbox: http://drupal.org/sandbox/pixelmord/1750250
Remaining
- Patch needs review
- Manual testing
Related
Comment | File | Size | Author |
---|---|---|---|
#41 | 1939096-41-markup.png | 65.15 KB | star-szr |
#38 | twig-feed-icon-1939096-38.patch | 3.49 KB | drupalmonkey |
#38 | interdiff.txt | 1.31 KB | drupalmonkey |
#29 | interdiff.txt | 2.99 KB | chrisjlee |
#29 | twig-feed-icon-1939096-29.patch | 3.6 KB | chrisjlee |
Comments
Comment #1
chrisjlee CreditAttribution: chrisjlee commentedWill probably fail; but figures i should give it a shot.
Not sure what to do here with the template preprocess function:
Comment #2
chrisjlee CreditAttribution: chrisjlee commentedComment #4
chrisjlee CreditAttribution: chrisjlee commentedTry again. No idea if this will work.
Comment #6
chrisjlee CreditAttribution: chrisjlee commentedComment #8
joelpittet@chrisjlee great start! May need the t() function in twig to get the translations to pass.
Comment #9
chrisjlee CreditAttribution: chrisjlee commentedthanks joelpittet. I'll give that a try.
Comment #10
chrisjlee CreditAttribution: chrisjlee commentedProbably need to put 'subscribe to' in the translate as well but not sure if it's needed. We'll find out.
Comment #11
chrisjlee CreditAttribution: chrisjlee commentedComment #13
joelpittetTotally do need "Subscribe to" in there too:) But you got it down to two fails instead of 4! Keep up the good work:)
Also, need some escaping to pass the other one.
{{ variable_name|e }}
They will be autoescaped eventually... but that is a cleanup task... either escape it with check_plain() in the preprocess or in twig with |e, not sure which is best right now because they will be removed later. Maybe check_plain() would be better for now so the twig file can stay clean..er. You're call:)
Comment #14
chrisjlee CreditAttribution: chrisjlee commentedAnother try based on joelpettit's feed . Thanks joel!
Comment #15
chrisjlee CreditAttribution: chrisjlee commentedargh someday i'll get better at remembering to change the status.
Comment #17
joelpittetoh my freedback didn't help much there, did it:( I think the t() needs the full string with the args just like the original... but I could be wrong. I could take a stab if you want, ping me on irc or here.
Comment #18
chrisjlee CreditAttribution: chrisjlee commented@joelpittet don't worry about :) i'm open to any ideas to get this patch green.
I'm going to go try something different.
Comment #20
joelpittetI gave those tests a good look and in my travels I found that this was already attempted a twig conversion on the twig sandbox. It wasn't quite perfect but I used that as a base, so the interdiff will look a bit drastic. Not sure it will pass all tests but hopefully it will get things going in the right direction. I also tweaked a test because it was calling theme_feed_icon() directly which since it doesn't exist anymore is one of the fails.
the twig filenames should be feed-icon instead of feed_icon for consistency but that wouldn't have caused a fail.
*Crosses fingers*
Comment #21
joelpittet@chrisjlee, could you review the patch for anything you think may be wrong or silly or spelling mistakes or the like? I never get things right the first time.
Comment #22
chrisjlee CreditAttribution: chrisjlee commentedLooks good to me. I wouldn't know any better though. Hope someone with more experience reviews this. :)
Only issue i see would be something small and nitpicky like this:
Feed should probably be on the same line? Although, it'd be one character over the drupal character line limit (80) as per drupal documentation comment standards.
Comment #23
joelpittet@chrisjlee you must be a designer too, "say no to widows" :)
Comment #24
star-szrIndeed the widow must stay. Quick review:
Is adding 'attributes' here necessary? Can't we just add it in the preprocess function?
This goes over 80 characters, and shouldn't use the word "array", especially because in this case I think it's an Attribute object. I'm wondering if we can have a boilerplate of sorts for 'attributes' since it's such a common variable. It might be worth looking at some of the other Twig conversion issues (or in the Twig sandbox) to see if there's a good description in one of the other patches :)
Comment #25
joelpittetDoc cleanup from #22 and #24
@Cottser, I am not sure if it's necessary, I removed it in this patch.
Comment #26
star-szrOne teensy tiny tweak, then RTBC from me :)
HTML classes (lower c).
Comment #27
chrisjlee CreditAttribution: chrisjlee commentedi'll help out with this small patch
Comment #28
star-szrMissed this before, I think we should indent attributes.title and attributes.class here.
Also, can we please replace the theme function instead of adding the new preprocess function above drupal_common_theme()? I think that would be better (same as #1939088-17: Convert theme_meter() to Twig).
Comment #29
chrisjlee CreditAttribution: chrisjlee commented@Cottser Thank you for the feedback!
Docs tweaks and attempted to put back the template preprocess function where it was!
Comment #30
star-szrThis looks great, tested manually and markup matches exactly. Great work guys!
Comment #31
alexpottComment #32
joelpittetComment #33
joelpittetdamn, hate when that happens, re-tagging.
Comment #34
jenlamptonComment #35
jenlampton#29: twig-feed-icon-1939096-29.patch queued for re-testing.
Comment #36
star-szrNeeds some touch-ups, things have changed :)
We don't need this line anymore, see #1982024: Lazy-load Attribute objects later in the rendering process only if needed.
The indented lines should just say 'title' and 'class'. Indent means 'add a dot'/drill down.
This line should be removed - see #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file.
Comment #37
drupalmonkey CreditAttribution: drupalmonkey commentedComment #38
drupalmonkey CreditAttribution: drupalmonkey commentedTook care of the items in #36.
Comment #39
star-szrAnother quick round of manual testing would be great before we go ahead with the profiling, re-tagging.
Comment #40
star-szrI will take care of manually testing and profiling this one.
Comment #41
star-szrThis looks good to me.
Markup comparison
(debug output in place to show that the markup is in fact coming from the new Twig template…)
Profiling
Scenario: default homepage.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5219624c55207&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5219624c55207&...
Comment #42
webchickOops, sorry, thought I'd committed this awhile back.
0.2% is about double what these normally are, but RSS icons aren't used overly often, so this seems relatively ok. If it's not, alexpott, feel free to revert.
Committed and pushed to 8.x. Thanks!
Comment #43.0
(not verified) CreditAttribution: commentedUpdate remaining