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
Related
Comment | File | Size | Author |
---|---|---|---|
#62 | test-status_message-data.txt | 1.81 KB | joelpittet |
#58 | drupal-convert-theme_status_messages-1939082-58.patch | 4.25 KB | markhalliwell |
#58 | interdiff.txt | 532 bytes | markhalliwell |
#54 | 1939082-54-status-twig.patch | 3.69 KB | duellj |
#54 | interdiff.txt | 648 bytes | duellj |
Comments
Comment #1
larowlanGuess I should have a crack at at least one of these.
Comment #2
larowlanSomething like this?
Comment #3
larowlanMissed @ingroup themeable and declaring the template name
Comment #5
star-szr#3: twig-status-messages-1939082.3.patch queued for re-testing.
Comment #7
star-szrThanks for the patches, @larowlan! We'll need to remove the theme() functions as well. Haven't reviewed these patches yet but I think something must be up in the Twig template, now that it's being used in #3.
Comment #8
rszrama CreditAttribution: rszrama commentedWe need to rekey the messages array in the preprocess function, because it's not guaranteed that messages[0] will be the only message in the array. For example, some module may have manipulated the array such that message[1] is the only element. If we reset the key in the preprocess, we can keep the messages[0] in the template while ensuring that a single message array will have a value there.
That particular fix needs to be backported to D7, too - suppose that should be a separate issue?
Comment #9
star-szr@rszrama - that makes sense to me. I think a separate issue for D7 would be the way to go.
For the Twig template I'm curious if the first filter would do the trick here.
Comment #10
star-szrDrat, core uses Twig 1.12.1 and that filter was added in 1.12.2 :)
Comment #11
jcisio CreditAttribution: jcisio commentedIssue created #1946244: theme_status_messages expects the first message is $messages[0]. I think it is not part of the conversion.
Comment #12
larowlanSo turns out Twig isn't registered as a service in the installer, so we can't use drupal_set_message without it, so we need to register it.
Comment #13
star-szr#3: twig-status-messages-1939082.3.patch queued for re-testing.
Edited to add: #3 should pass now that #1942490: Make Twig engine available during install is in.
Comment #14
star-szrHere's a review of #3 - some documentation to be tweaked, and we'll need to remove the theme_status_messages() function too.
@larowlan - would you be able to take care of these changes? If not, please unassign. Thanks!
Prepares variables for status message templates.
The available variables don't need to be indented here, the indent level can match "Available variables:".
We've been adding template_preprocess() where missing here, so:
@see template_preprocess()
@see template_preprocess_status_messages()
Comment #15
larowlanWill try and get to it next weekish, but if anyone beats me - go for it!
Comment #16
larowlanLooks like Twig is registered in the installer already now so that hunk went after merging HEAD.
Comment #17
jcisio CreditAttribution: jcisio commentedThe interdiff seems not correct. BTW the "use... Definition" is no longer needed.
Comment #18
star-szrThanks @larowlan! This patch:
I noticed the
<h2 class="element-invisible">
is missing from the output. The Twig template was not referring to the correct variable but this still needs more debugging.Comment #19
star-szrComment #20
star-szrAlso we could add something like this to document the optional 'display' variable in the template docblock (needs wrapping at 80 chars):
* - display: (optional) Has a value of e.g. 'status' or 'error' when only displaying messages of that type.
Comment #21
star-szrThis seems to work:
Comment #21.0
star-szrUpdate remaining
Comment #22
star-szrTagging.
Comment #23
shanethehat CreditAttribution: shanethehat commentedComment #24
shanethehat CreditAttribution: shanethehat commentedChanges implemented and tested against local install
Comment #26
star-szr#24: twig-status-messages-1939082-24.patch queued for re-testing.
Comment #27
star-szrThanks @shanethehat! The changes look great, the only hiccup is that status-messages.html.twig ended up with an extra newline at the end of the file, please remove that.
Comment #28
chrisjlee CreditAttribution: chrisjlee commentedManual testing passed. Attached are screenshots. Easiest manual testing ever with my favorite button... Clear cache.
Status has been updated although the patch did have a newline when applied as per #27
Comment #29
shanethehat CreditAttribution: shanethehat commentedremoved extra newline
Comment #31
shanethehat CreditAttribution: shanethehat commented#29: twig-status-messages-1939082-29.patch queued for re-testing.
Comment #32
star-szrThanks for the patches @shanethehat, and thanks for reviewing and testing @chrisjlee!
Attached a screenshot showing two status messages and a warning message, markup still checks out.
I agree with the RTBC after one more documentation tweak:
"Default theme implementation for status and/or…" per Twig coding standards: http://drupal.org/node/1823416
Comment #33
shanethehat CreditAttribution: shanethehat commentedDocumentation changed
Comment #34
star-szrThanks @shanethehat! I should have checked this, but the 'summary line' will need to be under 80 characters (http://drupal.org/node/1354#file), so we'll need to shorten that description.
We can add a line after to expand on the description given in the summary line, so I suggest something like this:
(Also removing manual testing tag.)
Comment #35
shanethehat CreditAttribution: shanethehat commentedDone
Comment #36
star-szr@shanethehat - Thanks for your patience. Two tiny things:
Trailing whitespace on the first line needs to be removed per http://drupal.org/coding-standards#indenting, and there should be a comma after 'error' (serial comma) per http://drupal.org/style-guide/content#english (from my understanding the style guide applies to our documentation as well - http://drupal.org/node/1134088#comment-6370252). Or if you can think of better wording, by all means :)
Comment #37
shanethehat CreditAttribution: shanethehat commented@Cottser Thank you for your patience! I really need to test my patches before posting.
Comment #38
star-szrThat looks great to me, RTBC if it comes back green :)
Comment #39
LewisNyman CreditAttribution: LewisNyman commentedRELATED: #1986408: Message style update
Comment #40
alexpott#1986408: Message style update should go in first
Comment #41
LewisNyman CreditAttribution: LewisNyman commentedHere's a patch update that brings the mark up inline with the latest patch in #1986408: Message style update
Once that's been committed, we should be able to RTBC this.
Comment #42
tlattimore CreditAttribution: tlattimore commentedTagging for review so the bot will test #41.
Comment #44
joelpittetpostponed on #1986408: Message style update
Comment #45
star-szrLooks like most of #1986408: Message style update got committed and just small tweaks now, so this should be ready to re-activate soon.
However #1843678: Move building of messages render array from template_process_page() to template_preprocess_page() is very close to this patch. Should the two issues be merged?
Comment #46
star-szrThere was a commit on #1986408: Message style update 30 minutes ago so going back to CNW for now.
Comment #47
jenlamptonreroll.
Comment #48
jenlamptona bit more cleanup.
Comment #49
azinoman CreditAttribution: azinoman commentedI reviewed the patch and created a article. The message still says status message at the beginning. Changing to needs work.
Comment #50
joelpittet@azinoman thanks, that's due to #1363112: Simplify names of "element-x" helper classes
This needs to be rolled with that in there.
Comment #51
adamcowboy CreditAttribution: adamcowboy commentedThe other issue has already been committed and this issue does not require re-roll. How do we get rid of the extra status message here? Do we need to open another issue?
Comment #52
star-szrThanks for taking a look @adamcowboy :)
The patch does need a reroll:
…and then the change to the element-invisible class in the Twig template as @joelpittet mentioned. Re-tagging for manual testing so we can double check this after things are updated.
Comment #53
star-szr.
Comment #54
duellj CreditAttribution: duellj commentedRerolled patch from #48 and renamed
element-invisible
class tovisually-hidden
Comment #55
duellj CreditAttribution: duellj commentedHmm, not sure why the tag wasn't removed, let's try again
Comment #56
waynethayer CreditAttribution: waynethayer commentedCompleted manual testing. Here's the comparison:
Before:
After:
Comment #57
joelpittetJust need to remove this bit.
Comment #58
markhalliwellRemoved
@see template_preprocess()
from doc per #57.Comment #59
joelpittetThank you @Mark Carver now all we need is a round or two of profiling and this is good to go.
Comment #60
joelpittetAssigning for profiling.
Comment #61
joelpittetLooks like about 0.463ms difference between the two runs on wall time. Looks good to me:)
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5230a98f3b26c&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5230a98f3b26c&...
Comment #62
joelpittetOh I forgot the Scenario for the above.
Attached is a patch I used to setup the tests.
10 status messages with different status types and text length.
Comment #63
star-szr+1, looks good to me.
Comment #64
webchickCommitted and pushed to 8.x. Thanks!
Comment #65
webchickCommitted and pushed to 8.x. Thanks!
Comment #66.0
(not verified) CreditAttribution: commentedUpdate remaining