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

Files: 
CommentFileSizeAuthor
#62 test-status_message-data.txt1.81 KBjoelpittet
#58 drupal-convert-theme_status_messages-1939082-58.patch4.25 KBMark Carver
PASSED: [[SimpleTest]]: [MySQL] 56,818 pass(es).
[ View ]
#58 interdiff.txt532 bytesMark Carver
#54 1939082-54-status-twig.patch3.69 KBduellj
PASSED: [[SimpleTest]]: [MySQL] 57,946 pass(es).
[ View ]
#54 interdiff.txt648 bytesduellj
#48 twig-status-messages-1939082-48.patch3.65 KBjenlampton
PASSED: [[SimpleTest]]: [MySQL] 57,765 pass(es).
[ View ]
#47 twig-status-messages-1939082-47.patch3.62 KBjenlampton
PASSED: [[SimpleTest]]: [MySQL] 57,735 pass(es).
[ View ]
#41 twig-status-messages-1939082-41.patch3.56 KBLewisNyman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-status-messages-1939082-41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#37 interdiff.txt832 bytesshanethehat
#37 twig-status-messages-1939082-37.patch3.51 KBshanethehat
PASSED: [[SimpleTest]]: [MySQL] 54,307 pass(es).
[ View ]
#35 twig-status-messages-1939082-35.patch3.51 KBshanethehat
PASSED: [[SimpleTest]]: [MySQL] 54,254 pass(es).
[ View ]
#35 interdiff.txt853 bytesshanethehat
#33 interdiff.txt796 bytesshanethehat
#33 twig-status-messages-1939082-33.patch3.47 KBshanethehat
PASSED: [[SimpleTest]]: [MySQL] 54,357 pass(es).
[ View ]
#32 1939082-32.png77.46 KBCottser
#29 interdiff.txt505 bytesshanethehat
#29 twig-status-messages-1939082-29.patch3.45 KBshanethehat
PASSED: [[SimpleTest]]: [MySQL] 54,186 pass(es).
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 54,238 pass(es).
[ View ]
#24 interdiff.txt1.14 KBshanethehat
#18 1939082-18.patch3.34 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,117 pass(es).
[ View ]
#18 interdiff.txt3.68 KBCottser
#16 twig-status-messages-1939082.16.interdiff.txt1.12 KBlarowlan
#16 twig-status-messages-1939082.20.patch2.84 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 54,231 pass(es).
[ View ]
#12 twig-status-messages-1939082.12.interdiff.txt2.03 KBlarowlan
#12 twig-status-messages-1939082.12.patch4.39 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 53,224 pass(es).
[ View ]
#3 twig-status-messages-1939082.3.interdiff.txt888 byteslarowlan
#3 twig-status-messages-1939082.3.patch2.36 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 53,636 pass(es).
[ View ]
#2 twig-status-messages-1939082.2.patch2.04 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 51,108 pass(es), 1,465 fail(s), and 26 exception(s).
[ View ]

Comments

Assigned:Unassigned» larowlan

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

Status:Active» Needs review
StatusFileSize
new2.04 KB
FAILED: [[SimpleTest]]: [MySQL] 51,108 pass(es), 1,465 fail(s), and 26 exception(s).
[ View ]

Something like this?

StatusFileSize
new2.36 KB
PASSED: [[SimpleTest]]: [MySQL] 53,636 pass(es).
[ View ]
new888 bytes

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.

Status:Needs work» Needs review

#3: twig-status-messages-1939082.3.patch queued for re-testing.

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

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

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.

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?

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

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

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

Status:Needs work» Needs review
StatusFileSize
new4.39 KB
PASSED: [[SimpleTest]]: [MySQL] 53,224 pass(es).
[ View ]
new2.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.

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

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

Assigned:larowlan» Unassigned

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

Status:Needs work» Needs review
StatusFileSize
new2.84 KB
PASSED: [[SimpleTest]]: [MySQL] 54,231 pass(es).
[ View ]
new1.12 KB

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.68 KB
new3.34 KB
PASSED: [[SimpleTest]]: [MySQL] 54,117 pass(es).
[ View ]

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.

Status:Needs review» Needs work

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.

This seems to work:

    {% if status_headings[type] %}
      <h2 class="element-invisible">{{ status_headings[type] }}</h2>
    {% endif %}

Issue summary:View changes

Update remaining

Issue tags:+Needs manual testing

Tagging.

Assigned:Unassigned» shanethehat

Assigned:shanethehat» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.14 KB
new3.45 KB
PASSED: [[SimpleTest]]: [MySQL] 54,238 pass(es).
[ View ]

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.

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

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.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new148.93 KB
new120.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

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new3.45 KB
PASSED: [[SimpleTest]]: [MySQL] 54,186 pass(es).
[ View ]
new505 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.

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

Status:Needs review» Needs work
StatusFileSize
new77.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

Status:Needs work» Needs review
StatusFileSize
new3.47 KB
PASSED: [[SimpleTest]]: [MySQL] 54,357 pass(es).
[ View ]
new796 bytes

Documentation changed

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

StatusFileSize
new853 bytes
new3.51 KB
PASSED: [[SimpleTest]]: [MySQL] 54,254 pass(es).
[ View ]

Done

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

StatusFileSize
new3.51 KB
PASSED: [[SimpleTest]]: [MySQL] 54,307 pass(es).
[ View ]
new832 bytes

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

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs profiling

#1986408: Message style update should go in first

StatusFileSize
new3.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-status-messages-1939082-41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

Status:Needs work» Postponed

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?

Status:Postponed» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.62 KB
PASSED: [[SimpleTest]]: [MySQL] 57,735 pass(es).
[ View ]

reroll.

StatusFileSize
new3.65 KB
PASSED: [[SimpleTest]]: [MySQL] 57,765 pass(es).
[ View ]

a bit more cleanup.

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.

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.

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?

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.

Issue tags:+Needs reroll

.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new648 bytes
new3.69 KB
PASSED: [[SimpleTest]]: [MySQL] 57,946 pass(es).
[ View ]

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

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

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>

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.

Status:Needs work» Needs review
StatusFileSize
new532 bytes
new4.25 KB
PASSED: [[SimpleTest]]: [MySQL] 56,818 pass(es).
[ View ]

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

Issue tags:-Novice

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

Assigned:Unassigned» joelpittet

Assigning for profiling.

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

StatusFileSize
new1.81 KB

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.

+1, looks good to me.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Committed and pushed to 8.x. Thanks!

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

Issue summary:View changes

Update remaining