Task

Use Twig instead of PHPTemplate

Twig sandbox: http://drupal.org/sandbox/pixelmord/1750250

Remaining

  • Update Twig template based on #20 and #21

Related

#1885560: [meta] Convert everything in theme.inc to Twig

CommentFileSizeAuthor
#62 test-status_message-data.txt1.81 KBjoelpittet
#58 drupal-convert-theme_status_messages-1939082-58.patch4.25 KBmarkhalliwell
#58 interdiff.txt532 bytesmarkhalliwell
#54 1939082-54-status-twig.patch3.69 KBduellj
#54 interdiff.txt648 bytesduellj
#48 twig-status-messages-1939082-48.patch3.65 KBjenlampton
#47 twig-status-messages-1939082-47.patch3.62 KBjenlampton
#41 twig-status-messages-1939082-41.patch3.56 KBLewisNyman
#37 interdiff.txt832 bytesshanethehat
#37 twig-status-messages-1939082-37.patch3.51 KBshanethehat
#35 twig-status-messages-1939082-35.patch3.51 KBshanethehat
#35 interdiff.txt853 bytesshanethehat
#33 interdiff.txt796 bytesshanethehat
#33 twig-status-messages-1939082-33.patch3.47 KBshanethehat
#32 1939082-32.png77.46 KBstar-szr
#29 interdiff.txt505 bytesshanethehat
#29 twig-status-messages-1939082-29.patch3.45 KBshanethehat
#28 status-twig-before-patch.png120.37 KBchrisjlee
#28 status-twig-after-patch.png148.93 KBchrisjlee
#24 twig-status-messages-1939082-24.patch3.45 KBshanethehat
#24 interdiff.txt1.14 KBshanethehat
#18 1939082-18.patch3.34 KBstar-szr
#18 interdiff.txt3.68 KBstar-szr
#16 twig-status-messages-1939082.16.interdiff.txt1.12 KBlarowlan
#16 twig-status-messages-1939082.20.patch2.84 KBlarowlan
#12 twig-status-messages-1939082.12.interdiff.txt2.03 KBlarowlan
#12 twig-status-messages-1939082.12.patch4.39 KBlarowlan
#3 twig-status-messages-1939082.3.interdiff.txt888 byteslarowlan
#3 twig-status-messages-1939082.3.patch2.36 KBlarowlan
#2 twig-status-messages-1939082.2.patch2.04 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Assigned: Unassigned » larowlan

Guess I should have a crack at at least one of these.

larowlan’s picture

Status: Active » Needs review
FileSize
2.04 KB

Something like this?

larowlan’s picture

Missed @ingroup themeable and declaring the template name

Status: Needs review » Needs work
Issue tags: -Twig

The last submitted patch, twig-status-messages-1939082.3.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Twig

The last submitted patch, twig-status-messages-1939082.3.patch, failed testing.

star-szr’s picture

Thanks 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.

rszrama’s picture

We 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?

star-szr’s picture

@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.

star-szr’s picture

Drat, core uses Twig 1.12.1 and that filter was added in 1.12.2 :)

jcisio’s picture

Issue created #1946244: theme_status_messages expects the first message is $messages[0]. I think it is not part of the conversion.

larowlan’s picture

Status: Needs work » Needs review
FileSize
4.39 KB
2.03 KB

So 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.

star-szr’s picture

#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.

star-szr’s picture

Status: Needs review » Needs work

Here'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!

+++ b/core/includes/theme.incundefined
@@ -1673,6 +1673,23 @@ function theme_status_messages($variables) {
+ * Preprocess variables for theme_status_messages().

Prepares variables for status message templates.

+++ b/core/modules/system/templates/status-messages.html.twigundefined
@@ -0,0 +1,36 @@
+ * Available variables:
+ *   - messages_display: List of messages of all statuses which attempts to be
+ *     displayed.
+ *   - status_heading: List of all status types.

The available variables don't need to be indented here, the indent level can match "Available variables:".

+++ b/core/modules/system/templates/status-messages.html.twigundefined
@@ -0,0 +1,36 @@
+ * @see template_preprocess_status_messages()

We've been adding template_preprocess() where missing here, so:

@see template_preprocess()
@see template_preprocess_status_messages()

larowlan’s picture

Assigned: larowlan » Unassigned

Will try and get to it next weekish, but if anyone beats me - go for it!

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.84 KB
1.12 KB

Looks like Twig is registered in the installer already now so that hunk went after merging HEAD.

jcisio’s picture

Status: Needs review » Needs work

The interdiff seems not correct. BTW the "use... Definition" is no longer needed.

star-szr’s picture

Status: Needs work » Needs review
FileSize
3.68 KB
3.34 KB

Thanks @larowlan! This patch:

  • Removes the theme function
  • Removes the install.core.inc change (use…Definition)
  • Changes message[0] to messages.0
  • Tweaks some documentation

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.

star-szr’s picture

Status: Needs review » Needs work
star-szr’s picture

Also 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.

star-szr’s picture

This seems to work:

    {% if status_headings[type] %}
      <h2 class="element-invisible">{{ status_headings[type] }}</h2>
    {% endif %}
star-szr’s picture

Issue summary: View changes

Update remaining

star-szr’s picture

Issue tags: +Needs manual testing

Tagging.

shanethehat’s picture

Assigned: Unassigned » shanethehat
shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
1.14 KB
3.45 KB

Changes implemented and tested against local install

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -Twig

The last submitted patch, twig-status-messages-1939082-24.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +Twig
star-szr’s picture

Thanks @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.

chrisjlee’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
148.93 KB
120.37 KB

Manual 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

shanethehat’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.45 KB
505 bytes

removed extra newline

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -Twig

The last submitted patch, twig-status-messages-1939082-29.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +Twig
star-szr’s picture

Status: Needs review » Needs work
FileSize
77.46 KB

Thanks 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:

+++ b/core/modules/system/templates/status-messages.html.twigundefined
@@ -0,0 +1,37 @@
+ * @file
+ * Returns HTML for status and/or error messages, grouped by type.

"Default theme implementation for status and/or…" per Twig coding standards: http://drupal.org/node/1823416

shanethehat’s picture

Status: Needs work » Needs review
FileSize
3.47 KB
796 bytes

Documentation changed

star-szr’s picture

Issue tags: -Needs manual testing

Thanks @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.

+++ b/core/modules/system/templates/status-messages.html.twigundefined
@@ -0,0 +1,38 @@
+ * @file
+ * Default theme implementation for status and/or error messages, grouped by
+ * type.
+ *
+ * An invisible heading identifies the messages for assistive technology.
+ * Sighted users see a colored box. See http://www.w3.org/TR/WCAG-TECHS/H69.html
+ * for info.

We can add a line after to expand on the description given in the summary line, so I suggest something like this:

@file
Default theme implementation for status messages.

Displays status, error, and warning messages, grouped by type.

An invisible heading…

(Also removing manual testing tag.)

shanethehat’s picture

star-szr’s picture

@shanethehat - Thanks for your patience. Two tiny things:

+++ b/core/modules/system/templates/status-messages.html.twigundefined
@@ -0,0 +1,39 @@
+ * ¶
+ * Displays status, error and warning messages, grouped by type.

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 :)

shanethehat’s picture

@Cottser Thank you for your patience! I really need to test my patches before posting.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

That looks great to me, RTBC if it comes back green :)

LewisNyman’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +needs profiling
LewisNyman’s picture

Here'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.

tlattimore’s picture

Status: Needs work » Needs review

Tagging for review so the bot will test #41.

Status: Needs review » Needs work

The last submitted patch, twig-status-messages-1939082-41.patch, failed testing.

joelpittet’s picture

Status: Needs work » Postponed
star-szr’s picture

Looks 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?

star-szr’s picture

Status: Postponed » Needs work

There was a commit on #1986408: Message style update 30 minutes ago so going back to CNW for now.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
3.62 KB

reroll.

jenlampton’s picture

a bit more cleanup.

azinoman’s picture

Status: Needs review » Needs work

I reviewed the patch and created a article. The message still says status message at the beginning. Changing to needs work.

joelpittet’s picture

Issue tags: +Novice, +Needs reroll

@azinoman thanks, that's due to #1363112: Simplify names of "element-x" helper classes

This needs to be rolled with that in there.

adamcowboy’s picture

The 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?

star-szr’s picture

Thanks for taking a look @adamcowboy :)

The patch does need a reroll:

error: while searching for:
    ),
    'status_messages' => array(
      'variables' => array('display' => NULL),
    ),
    'link' => array(
      'variables' => array('text' => NULL, 'path' => NULL, 'options' => array()),

error: patch failed: core/includes/theme.inc:3119
error: core/includes/theme.inc: patch does not apply

…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.

star-szr’s picture

Issue tags: +Needs reroll

.

duellj’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
648 bytes
3.69 KB

Rerolled patch from #48 and renamed element-invisible class to visually-hidden

duellj’s picture

Hmm, not sure why the tag wasn't removed, let's try again

waynethayer’s picture

Issue tags: -Needs manual testing

Completed manual testing. Here's the comparison:

Before:

<div class="messages messages--error">
<h2 class="visually-hidden">Error message</h2>
One or more problems were detected with your Drupal installation. Check the <a href="/admin/reports/status">status report</a> for more information.</div>
<div class="messages messages--status">
<h2 class="visually-hidden">Status message</h2>
Caches cleared.</div>

After:

<div class="messages messages--error">
          <h2 class="visually-hidden">Error message</h2>
              One or more problems were detected with your Drupal installation. Check the <a href="/admin/reports/status">status report</a> for more information.
      </div>
  <div class="messages messages--status">
          <h2 class="visually-hidden">Status message</h2>
              Caches cleared.
      </div>
joelpittet’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/templates/status-messages.html.twig
@@ -0,0 +1,39 @@
+ * @see template_preprocess()

Just need to remove this bit.

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
532 bytes
4.25 KB

Removed @see template_preprocess() from doc per #57.

joelpittet’s picture

Issue tags: -Novice

Thank you @Mark Carver now all we need is a round or two of profiling and this is good to go.

joelpittet’s picture

Assigned: Unassigned » joelpittet

Assigning for profiling.

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -needs profiling

Looks like about 0.463ms difference between the two runs on wall time. Looks good to me:)

=== 8.x..8.x compared (5230a98f3b26c..5230a9efcd55c):

ct  : 39,779|39,779|0|0.0%
wt  : 419,293|417,982|-1,311|-0.3%
cpu : 396,288|395,101|-1,187|-0.3%
mu  : 36,333,312|36,333,312|0|0.0%
pmu : 36,372,688|36,372,688|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5230a98f3b26c&...

=== 8.x..1939082-twig-theme-status_messages compared (5230a98f3b26c..5230aa47d621c):

ct  : 39,779|39,925|146|0.4%
wt  : 419,293|418,445|-848|-0.2%
cpu : 396,288|395,375|-913|-0.2%
mu  : 36,333,312|36,399,408|66,096|0.2%
pmu : 36,372,688|36,438,240|65,552|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5230a98f3b26c&...

joelpittet’s picture

Oh 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.

star-szr’s picture

+1, looks good to me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

webchick’s picture

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Update remaining