Issue #1898436 by chrisjlee, joelpittet, gnuget, Cottser: overlay.module - Convert PHPTemplate templates to Twig.
(as of #43)

Task

Convert PHPTemplate templates to Twig templates.

Remaining

n/a

Template path Conversion status
core/modules/overlay/templates/overlay.tpl.php converted

Profiling results

=== 8.x..8.x compared (51981beb768ad..51981e001fd46):
ct  : 36,140|36,140|0|0.0%
wt  : 345,481|345,338|-143|-0.0%
cpu : 316,694|316,391|-303|-0.1%
mu  : 28,878,264|28,878,264|0|0.0%
pmu : 28,993,328|28,993,328|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51981beb768ad&...

=== 8.x..overlay-1898436-33 compared (51981beb768ad..51981e3ae1e6c):
ct  : 36,140|36,220|80|0.2%
wt  : 345,481|345,604|123|0.0%
cpu : 316,694|317,226|532|0.2%
mu  : 28,878,264|28,913,256|34,992|0.1%
pmu : 28,993,328|29,022,712|29,384|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51981beb768ad&...

#1987408: overlay.module - Convert theme_ functions to Twig
#1757550: [META-63] Convert core theme functions to Twig templates

Files: 
CommentFileSizeAuthor
#39 1898436-39.patch4.03 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,717 pass(es).
[ View ]
#39 interdiff.txt621 bytesCottser
#38 interdiff.txt2.37 KBjoelpittet
#38 1898436-38-overlay-twig.patch3.94 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 57,215 pass(es).
[ View ]
#37 1898436-37-overlay-twig.patch3.07 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 57,086 pass(es).
[ View ]
#37 interdiff.txt1.26 KBjoelpittet
#33 1898436-convert-overlay-to-twig-33.patch2.96 KBgnuget
PASSED: [[SimpleTest]]: [MySQL] 55,798 pass(es).
[ View ]
#33 interdiff.txt912 bytesgnuget
#29 1898436-convert-overlay-to-twig-29.patch2.97 KBgnuget
PASSED: [[SimpleTest]]: [MySQL] 55,956 pass(es).
[ View ]
#22 1898436-convert-overlay-to-twig-22.patch7.28 KBchrisjlee
PASSED: [[SimpleTest]]: [MySQL] 54,306 pass(es).
[ View ]
#22 interdiff.txt786 byteschrisjlee
#19 1898436-convert-overlay-to-twig-19.patch7.85 KBchrisjlee
PASSED: [[SimpleTest]]: [MySQL] 54,312 pass(es).
[ View ]
#19 interdiff.txt729 byteschrisjlee
#16 1898436-convert-overlay-to-twig-16.patch7.83 KBchrisjlee
PASSED: [[SimpleTest]]: [MySQL] 54,360 pass(es).
[ View ]
#16 interdiff.txt1 KBchrisjlee
#14 interdiff.txt711 byteschrisjlee
#14 1898436-convert-overlay-to-twig-15.patch7.64 KBchrisjlee
PASSED: [[SimpleTest]]: [MySQL] 54,380 pass(es).
[ View ]
#13 1898436-convert-overlay-to-twig-13.patch7.64 KBchrisjlee
FAILED: [[SimpleTest]]: [MySQL] 54,319 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#13 interdiff.txt542 byteschrisjlee
#12 interdiff.txt529 byteschrisjlee
#12 1898436-convert-overlay-to-twig-12.patch7.65 KBchrisjlee
PASSED: [[SimpleTest]]: [MySQL] 54,328 pass(es).
[ View ]
#11 1898436-convert-overlay-reroll-11.patch7.69 KBchrisjlee
PASSED: [[SimpleTest]]: [MySQL] 54,344 pass(es).
[ View ]
#11 interdiff.txt2.19 KBchrisjlee
#11 1898436-convert-overlay-to-twig-11.patch7.67 KBchrisjlee
PASSED: [[SimpleTest]]: [MySQL] 54,377 pass(es).
[ View ]
#7 1898436-overlay-twig-7.patch7.65 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 53,361 pass(es).
[ View ]
#7 interdiff.txt3.42 KBjoelpittet
#4 1898436-overlay-twig-4.patch6 KBchrisjlee
PASSED: [[SimpleTest]]: [MySQL] 51,290 pass(es).
[ View ]
#4 interdiff.txt1.64 KBchrisjlee
#3 1898436-overlay-twig-3.patch4.56 KBchrisjlee
PASSED: [[SimpleTest]]: [MySQL] 51,302 pass(es).
[ View ]

Comments

Issue tags:+Twig

Tagging

Assigned:Unassigned» zakiya

Assigned:zakiya» chrisjlee
Status:Active» Needs review
StatusFileSize
new4.56 KB
PASSED: [[SimpleTest]]: [MySQL] 51,302 pass(es).
[ View ]

not sure if i should add back in template_preprocess_overlay_disable_message function. if so #4 includes adding back in that function to core.

StatusFileSize
new1.64 KB
new6 KB
PASSED: [[SimpleTest]]: [MySQL] 51,290 pass(es).
[ View ]

This patch includes the template_preprocess_overlay_disable_message function.

Status:Needs review» Reviewed & tested by the community

Patch works as expected when applied to core. Marked as reviewed but should probably use classes instead of ids.

Assigned:chrisjlee» c4rl
Status:Reviewed & tested by the community» Needs review

Awesome!!

I would love a quick sign-off on this from one of the Twigluminati ;), so arbitrarily assigning to c4rl since he was up above.

Assigned:c4rl» joelpittet
StatusFileSize
new3.42 KB
new7.65 KB
PASSED: [[SimpleTest]]: [MySQL] 53,361 pass(es).
[ View ]

@zakiya Totally agree, we should get rid of the ID, although not in this issue, this is meant to be a as straight forward conversion as possible. Could you open another issue as maybe a follow-up to this one to look at removing the ID and fixing the JS/CSS that that change may affect?

+  $variables['profile_link'] = drupal_render($element['profile_link']);
+  $variables['dismiss_link'] = drupal_render($element['dismiss_message_link']);

Removed the drupal_render()

And removed theme_overlay_disable_message() because from what I remember(unless things have changed) we are getting rid of these.

And just some spacing nitpicks are in here too.

Functionally I would give this an RTBC.

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

Found some documentation tweaks that can be done to move this along further, and tagging for manual testing.

  1. +++ b/core/modules/overlay/overlay.moduleundefined
    @@ -546,6 +510,36 @@ function template_preprocess_overlay(&$variables) {
    + * Preprocess variables for the message about how to disable the overlay.
    + *
    + * @param $variables
    + *   An associative array with an 'element' element, which itself is an
    + *   associative array containing:
    + *   - profile_link: The link to this user's account.
    + *   - dismiss_message_link: The link to dismiss the overlay.
    + *
    + * @ingroup themeable

    This should be updated to match #1913208: [policy] Standardize template preprocess function documentation, something like:

    Prepares variables for overlay disable message templates.
    Default template: overlay-disable-message.html.twig.
    @param array $variables

    …and so on. No @ingroup needed.

  2. +++ b/core/modules/overlay/overlay.moduleundefined
    @@ -546,6 +510,36 @@ function template_preprocess_overlay(&$variables) {
    +  // Render the links.

    This comment should be removed, since we're not rendering yet.

  3. +++ b/core/modules/overlay/templates/overlay-disable-message.html.twigundefined
    @@ -0,0 +1,20 @@
    + * @file
    + * Default theme implementation for the message about how to disable the overlay.

    If we change "how to disable" to "disabling" this will fit within the 80 character limit for summaries (http://drupal.org/node/1354#file).

  4. +++ b/core/modules/overlay/templates/overlay-disable-message.html.twigundefined
    @@ -0,0 +1,20 @@
    + * - attributes: An array of element attributes.

    We can probably remove this line, the attributes are not output in the template. If we keep it, we should say something like "HTML attributes for the containing element.", assuming the attributes are for the <div>.

  5. +++ b/core/modules/overlay/templates/overlay.html.twigundefined
    @@ -0,0 +1,34 @@
    + * - tabs: Tabs linking to any sub-pages beneath the current page (e.g., the view
    + * and edit tabs when displaying a node).

    First line is 81 characters, should wrap to 80 per http://drupal.org/node/1354#drupal. The second line should match the indent of the first, lining up with the 't' in 'tabs'.

Assigned:joelpittet» Unassigned

Got the okay from @joelpittet to unassign.

Issue tags:+Novice

Creating a new patch and interdiff based on #7 with the documentation changes in #8 would be a good novice task.

Status:Needs work» Needs review
Issue tags:-Novice
StatusFileSize
new7.67 KB
PASSED: [[SimpleTest]]: [MySQL] 54,377 pass(es).
[ View ]
new2.19 KB
new7.69 KB
PASSED: [[SimpleTest]]: [MySQL] 54,344 pass(es).
[ View ]

Attempt to reroll.

@cottser Thanks for the helpful and thorough review

StatusFileSize
new7.65 KB
PASSED: [[SimpleTest]]: [MySQL] 54,328 pass(es).
[ View ]
new529 bytes

oops forgot to remove @themeable

StatusFileSize
new542 bytes
new7.64 KB
FAILED: [[SimpleTest]]: [MySQL] 54,319 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Changes as per conversation with @Cottser
- Remove line above where @themeable in docblock was

StatusFileSize
new7.64 KB
PASSED: [[SimpleTest]]: [MySQL] 54,380 pass(es).
[ View ]
new711 bytes

Realized i forgot to wrap one of the docblock lines as per comment #8.5.

Status:Needs review» Needs work

Thanks for keeping this moving @chrisjlee! I just tested this patch manually and found that overlay-disable-message.html.twig was not being picked up, we need to add the 'template' declaration in overlay_theme() for that template to be used.

And some more documentation tweaks:

+++ b/core/modules/overlay/templates/overlay-disable-message.html.twigundefined
@@ -0,0 +1,19 @@
+ * - profile_link: Rendered link.
+ * - dismiss_link: Rendered link to disable the message.

In overlay-disable-message.html.twig, let's use the same documentation for these variables as in template_preprocess_overlay_disable_message(), the preprocess function docs are much better.

+++ b/core/modules/overlay/templates/overlay.html.twigundefined
@@ -0,0 +1,34 @@
+ * - title: the (sanitized) title of the page.

In overlay.html.twig, the description of the 'title' variable should start with a capital letter (capitalize the first 'the') per http://drupal.org/node/1354#drupal.

Status:Needs work» Needs review
StatusFileSize
new1 KB
new7.83 KB
PASSED: [[SimpleTest]]: [MySQL] 54,360 pass(es).
[ View ]

@Cottser thanks for the feedback again it was very helpful and thorough.

Looks good, just needs this docs tweak from #15 then I think this one is ready! I tested this one manually, works much better now :)

In overlay-disable-message.html.twig, let's use the same documentation for these variables as in template_preprocess_overlay_disable_message(), the preprocess function docs are much better.

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

The above change is a good novice task, tagging. Once that's done this patch is RTBC in my opinion.

If you'd like to work on this, please assign the issue to yourself. If you get stuck, drop by #drupal-twig on IRC or post your questions here. Thanks!

StatusFileSize
new729 bytes
new7.85 KB
PASSED: [[SimpleTest]]: [MySQL] 54,312 pass(es).
[ View ]

Overlay works better for me as well after the patch.

Changes as per #15

Status:Needs work» Needs review

I've been eyeing the first hunk of this patch and I finally tracked it down to being a reversal of part of http://drupalcode.org/project/drupal.git/commit/b574b9e. @chrisjlee is on it!

StatusFileSize
new786 bytes
new7.28 KB
PASSED: [[SimpleTest]]: [MySQL] 54,306 pass(es).
[ View ]

remove offending change reference according to #21

Status:Needs review» Reviewed & tested by the community

Ah, much better. This is good to go when it comes back green, thanks for all your work on this @chrisjlee!

Status:Reviewed & tested by the community» Needs work
Issue tags:-Novice, -Needs manual testing, -Twig

The last submitted patch, 1898436-convert-overlay-to-twig-22.patch, failed testing.

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

Status:Needs review» Reviewed & tested by the community

Testbot decided it didn't want to apply the patch, back to RTBC :)

Issue summary:View changes

Add conversion summary table

Title:Convert overlay module to Twigoverlay.module - Convert PHPTemplate templates to Twig
Status:Reviewed & tested by the community» Needs work

Per #1757550-44: [META-63] Convert core theme functions to Twig templates, retitling to indicate this issue applies to templates rather than theme_ functions. See #1987408: overlay.module - Convert theme_ functions to Twig for theme_ function conversion.

Issue summary:View changes

Remove sandbox link

Assigned:Unassigned» gnuget

Assigned:gnuget» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.97 KB
PASSED: [[SimpleTest]]: [MySQL] 55,956 pass(es).
[ View ]

Adding a patch only with the .tpl.php conversions.

Thanks @gnuget! We'll need a patch with only the theme_ function conversions uploaded at #1987408: overlay.module - Convert theme_ functions to Twig to complete the patch split. One way of doing this is by applying #22 locally then reverse applying #29 (git apply -R patchname.patch).

Since gnuget assigned it to anonymous, I took care of #30. Let me know if there are any issues.

Status:Needs review» Needs work

+++ b/core/modules/overlay/templates/overlay.html.twigundefined
@@ -0,0 +1,34 @@
+      <h2 class="element-invisible">{{ 'Primary tabs'|t }}</h2><ul id="overlay-tabs">{{ render_var(tabs) }}</ul>

We should be able to lose the render_var here, just {{ tabs }} please.

Status:Needs work» Needs review
StatusFileSize
new912 bytes
new2.96 KB
PASSED: [[SimpleTest]]: [MySQL] 55,798 pass(es).
[ View ]

Removing the render_var

Issue tags:+Needs profiling

Tagging for profiling.

Assigned:Unassigned» Cottser
Issue tags:-Novice, -Needs manual testing

I'll be profiling this and giving it another review.

Assigned:Cottser» Unassigned
Status:Needs review» Needs work
Issue tags:-Needs profiling+Novice

Profiling results look great.

=== 8.x..8.x compared (51981beb768ad..51981e001fd46):
ct  : 36,140|36,140|0|0.0%
wt  : 345,481|345,338|-143|-0.0%
cpu : 316,694|316,391|-303|-0.1%
mu  : 28,878,264|28,878,264|0|0.0%
pmu : 28,993,328|28,993,328|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51981beb768ad&...

=== 8.x..overlay-1898436-33 compared (51981beb768ad..51981e3ae1e6c):
ct  : 36,140|36,220|80|0.2%
wt  : 345,481|345,604|123|0.0%
cpu : 316,694|317,226|532|0.2%
mu  : 28,878,264|28,913,256|34,992|0.1%
pmu : 28,993,328|29,022,712|29,384|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51981beb768ad&...


This needs a few small tweaks and then it's RTBC from my perspective:

+++ b/core/modules/overlay/templates/overlay.html.twigundefined
@@ -0,0 +1,34 @@
+      <a id="overlay-close" href="#" class="overlay-close"><span class="element-invisible">{{ 'Close overlay'|t }}</span></a>
+++ /dev/nullundefined
@@ -1,38 +0,0 @@
-      <a id="overlay-close" href="#" class="overlay-close" role="button" aria-controls="overlay-content"><span class="element-invisible"><?php print t('Close overlay'); ?></span></a>

We need to add the 'role' and 'aria-controls' attributes as in the .tpl.php version.

+++ b/core/modules/overlay/templates/overlay.html.twigundefined
@@ -0,0 +1,34 @@
+{{ disable_overlay }}
+<div id="overlay"{{ attributes }}>
...
+      <h1 id="overlay-title"{{ title_attributes }}>{{ title }}</h1>
...
+  <div id="overlay-content"{{ content_attributes }}>

We should document at least 'disable_overlay' - something like "the message about how to disable the overlay."?, but we can probably document the attributes ones too by copying and pasting from existing conversions.

Status:Needs work» Needs review
Issue tags:-Novice
StatusFileSize
new1.26 KB
new3.07 KB
PASSED: [[SimpleTest]]: [MySQL] 57,086 pass(es).
[ View ]

Changes from #36

StatusFileSize
new3.94 KB
PASSED: [[SimpleTest]]: [MySQL] 57,215 pass(es).
[ View ]
new2.37 KB

few more things, moved ids to the preprocess and added attributes comments.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new621 bytes
new4.03 KB
PASSED: [[SimpleTest]]: [MySQL] 55,717 pass(es).
[ View ]

Wow, even better. Thanks @joelpittet!

Just rolling in a typo fix for pre-existing docs. RTBC :)

Issue summary:View changes

Clarify issue summary

Status:Reviewed & tested by the community» Needs work
Issue tags:-Twig

The last submitted patch, 1898436-39.patch, failed testing.

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

#39: 1898436-39.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Looks good, back to rtbc.

Title:overlay.module - Convert PHPTemplate templates to Twig[READY] overlay.module - Convert PHPTemplate templates to Twig
Status:Reviewed & tested by the community» Closed (duplicate)

Issue summary:View changes

Add profiling results to summary, update remaining

Title:[READY] overlay.module - Convert PHPTemplate templates to Twigoverlay.module - Convert PHPTemplate templates to Twig
Status:Closed (duplicate)» Fixed

Committed b171982 and pushed to 8.x. Thanks!

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

Issue summary:View changes

Updated issue summary.