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] Convert core theme functions to Twig templates

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

zakiya’s picture

Assigned: Unassigned » zakiya
chrisjlee’s picture

Assigned: zakiya » chrisjlee
Status: Active » Needs review
FileSize
4.56 KB

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.

chrisjlee’s picture

This patch includes the template_preprocess_overlay_disable_message function.

zakiya’s picture

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.

webchick’s picture

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.

joelpittet’s picture

Assigned: c4rl » joelpittet
FileSize
3.42 KB
7.65 KB

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

star-szr’s picture

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

star-szr’s picture

Assigned: joelpittet » Unassigned

Got the okay from @joelpittet to unassign.

star-szr’s picture

Issue tags: +Novice

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

chrisjlee’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
7.67 KB
2.19 KB
7.69 KB

Attempt to reroll.

@cottser Thanks for the helpful and thorough review

chrisjlee’s picture

oops forgot to remove @themeable

chrisjlee’s picture

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

chrisjlee’s picture

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

star-szr’s picture

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.

chrisjlee’s picture

Status: Needs work » Needs review
FileSize
1 KB
7.83 KB

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

star-szr’s picture

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.

star-szr’s picture

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!

chrisjlee’s picture

Overlay works better for me as well after the patch.

Changes as per #15

chrisjlee’s picture

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

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!

chrisjlee’s picture

remove offending change reference according to #21

star-szr’s picture

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.

star-szr’s picture

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

Status: Needs review » Reviewed & tested by the community

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

star-szr’s picture

Issue summary: View changes

Add conversion summary table

c4rl’s picture

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

Per #1757550-44: [Meta] 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.

c4rl’s picture

Issue summary: View changes

Remove sandbox link

gnuget’s picture

Assigned: Unassigned » gnuget
gnuget’s picture

Assigned: gnuget » Unassigned
Status: Needs work » Needs review
FileSize
2.97 KB

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

star-szr’s picture

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

chrisjlee’s picture

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

star-szr’s picture

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.

gnuget’s picture

Status: Needs work » Needs review
FileSize
912 bytes
2.96 KB

Removing the render_var

star-szr’s picture

Issue tags: +needs profiling

Tagging for profiling.

star-szr’s picture

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

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

star-szr’s picture

Assigned: star-szr » 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.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
1.26 KB
3.07 KB

Changes from #36

joelpittet’s picture

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

star-szr’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
621 bytes
4.03 KB

Wow, even better. Thanks @joelpittet!

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

star-szr’s picture

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.

star-szr’s picture

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

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, back to rtbc.

alexpott’s picture

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

Issue summary: View changes

Add profiling results to summary, update remaining

alexpott’s picture

Title: [READY] overlay.module - Convert PHPTemplate templates to Twig » overlay.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.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.