Task

Convert theme_ functions to Twig templates.

Remaining

  • theme_ function -> Twig conversions (and preprocess function additions or changes) moved over from the existing conversion issue
  • Patch review
  • Profiling
Theme function name Conversion status
theme_overlay_disable_message @todo move from #1898436: overlay.module - Convert PHPTemplate templates to Twig

#1898436: overlay.module - Convert PHPTemplate templates to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
#1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch

CommentFileSizeAuthor
#21 interdiff.txt408 byteschrisjlee
#21 1987408-overlay-convert-theme-21.patch2.71 KBchrisjlee
#19 interdiff-1987408-18.txt3.62 KBchrisjlee
#18 1987408-overlay-convert-theme-18.patch2.76 KBchrisjlee
#16 1987408-overlay-profiling-tweak-do-not-test.patch404 bytesstar-szr
#10 convert-overlay-theme-functions-to-twig-1987408-10.patch4.48 KBjstoller
#9 convert-overlay-theme-functions-to-twig-1987408-9.patch3.59 KBjstoller
#5 1987408-convert-theme-functions-to-twig-5.patch4.31 KBgnuget
#2 1987408-convert-theme-functions-to-twig-2.patch3.49 KBchrisjlee
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue summary: View changes

Updated issue summary.

c4rl’s picture

c4rl CreditAttribution: c4rl commented
Title: overlay.module - Convert PHPTemplate templates to Twig » overlay.module - Convert theme_ functions to Twig

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions rather than templates. See #1898436: overlay.module - Convert PHPTemplate templates to Twig for template conversion.

c4rl’s picture

c4rl CreditAttribution: c4rl commented
Issue summary: View changes

Updated issue summary.

chrisjlee’s picture

chrisjlee CreditAttribution: chrisjlee commented
Status: Active » Needs review
FileSize
1987408-convert-theme-functions-to-twig-2.patch3.49 KB

Patch split for #1898436: overlay.module - Convert PHPTemplate templates to Twig as requested by Cottser in #30.

Status: Needs review » Needs work

The last submitted patch, 1987408-convert-theme-functions-to-twig-2.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

As discussed with @chrisjlee this just looks to be missing overlay-disable-message.html.twig from #1898436-22: overlay.module - Convert PHPTemplate templates to Twig.

gnuget’s picture

gnuget
Primary language Spanish
CreditAttribution: gnuget commented
Status: Needs work » Needs review
FileSize
1987408-convert-theme-functions-to-twig-5.patch4.31 KB

The same patch as in #2 but including the overlay-disable-message.html.twig file

jstoller’s picture

jstoller
he/him
CreditAttribution: jstoller commented
Issue tags: -needs profiling, -Twig

#5: 1987408-convert-theme-functions-to-twig-5.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +needs profiling, +Twig

The last submitted patch, 1987408-convert-theme-functions-to-twig-5.patch, failed testing.

jstoller’s picture

jstoller
he/him
CreditAttribution: jstoller commented
Assigned: Unassigned » jstoller

Working on a re-roll now.

jstoller’s picture

jstoller
he/him
CreditAttribution: jstoller commented
Status: Needs work » Needs review
FileSize
convert-overlay-theme-functions-to-twig-1987408-9.patch3.59 KB

Re-roll.

jstoller’s picture

jstoller
he/him
CreditAttribution: jstoller commented
Assigned: jstoller » Unassigned
FileSize
convert-overlay-theme-functions-to-twig-1987408-10.patch4.48 KB

Oops. Missed something.

misselbeck’s picture

misselbeck CreditAttribution: misselbeck commented
Issue tags: -needs profiling

Profiling output

=== 8.x..8.x compared (519ff2cc37c4c..519ff4f45552e):

ct  : 35,477|35,477|0|0.0%
wt  : 460,098|459,571|-527|-0.1%
cpu : 436,967|438,305|1,338|0.3%
mu  : 34,266,872|34,266,872|0|0.0%
pmu : 34,346,128|34,346,128|0|0.0%

<a href="http://local.d8.com/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=519ff2cc37c4c&run2=519ff4f45552e&extra=8.x..8.x">Profiler output</a>

Warning: Must specify directory location for XHProf runs. Trying /tmp as default. You can either pass the directory location as an argument to the constructor for XHProfRuns_Default() or set xhprof.output_dir ini param.
=== 8.x..overlay_profiler_1987408 compared (519ff2cc37c4c..519ff53016b3c):

ct  : 35,477|35,477|0|0.0%
wt  : 460,098|460,169|71|0.0%
cpu : 436,967|437,025|58|0.0%
mu  : 34,266,872|34,267,232|360|0.0%
pmu : 34,346,128|34,346,584|456|0.0%

<a href="http://local.d8.com/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=519ff2cc37c4c&run2=519ff53016b3c&extra=8.x..overlay_profiler_1987408">Profiler output</a>

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

The last submitted patch, convert-overlay-theme-functions-to-twig-1987408-10.patch, failed testing.

ezeedub’s picture

ezeedub CreditAttribution: ezeedub commented
Status: Needs work » Needs review
Issue tags: +Twig

#10: convert-overlay-theme-functions-to-twig-1987408-10.patch queued for re-testing.

ezeedub’s picture

ezeedub CreditAttribution: ezeedub commented
Issue tags: +needs profiling

Same number of function calls in last test, re-tagging...

=== 8.x..8.x compared (519ff2cc37c4c..519ff4f45552e):

ct  : 35,477|35,477|0|0.0%

...

=== 8.x..overlay_profiler_1987408 compared (519ff2cc37c4c..519ff53016b3c):

ct  : 35,477|35,477|0|0.0%
ezeedub’s picture

ezeedub CreditAttribution: ezeedub commented

i'm trying to hit the path overlay/dismiss-message and getting access denied. even as admin (though i need to hit it as anon to profile)
i've set overlay_user_dismiss_message_access() to return true and called menu_router_rebuild(), but still access denied (still, even as admin). any ideas?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: -needs profiling
FileSize
1987408-overlay-profiling-tweak-do-not-test.patch404 bytes

Profiling results:

=== overlay-8.x..overlay-8.x compared (51abc4cb0a647..51abc563dc23f):

ct  : 29,769|29,769|0|0.0%
wt  : 168,292|168,089|-203|-0.1%
cpu : 145,790|145,667|-123|-0.1%
mu  : 11,181,088|11,181,088|0|0.0%
pmu : 11,269,632|11,269,632|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51abc4cb0a647&...

=== overlay-8.x..overlay-1987408-10 compared (51abc4cb0a647..51abc588c7973):

ct  : 29,769|29,869|100|0.3%
wt  : 168,292|168,237|-55|-0.0%
cpu : 145,790|147,469|1,679|1.2%
mu  : 11,181,088|11,208,568|27,480|0.2%
pmu : 11,269,632|11,296,248|26,616|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51abc4cb0a647&...

Markup testing checked out, performed visual diff and DaisyDiff for /node/add?render=overlay.

For overlay profiling I set the path in find-min-web.sh to /node/add?render=overlay and gave anonymous users the following permissions:

  • Bypass content access control
  • Administer content
  • Access the administrative overlay

Should have noted this in #1898436: overlay.module - Convert PHPTemplate templates to Twig.

In addition to the steps above, a small tweak was needed here to make the overlay disable message show up for anonymous users, since it normally only shows up for authenticated users. Patch attached was applied on both branches to fake an authenticated user for the purposes of the overlay disable message.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Needs work
Issue tags: +Novice

Two small tweaks I found:

1. Can we move the new preprocess function so that it's in the same place as the old theme function in overlay.module? i.e. directly after overlay_disable_message(). This will make the patch smaller and easier to review.

2. 

+++ b/core/modules/overlay/templates/overlay-disable-message.html.twigundefined
@@ -0,0 +1,19 @@
+  <h3 class="element-invisible">{{ "Options for the administrative overlay"|t }}</h3>

Single quotes here please per https://drupal.org/node/1823416#filters.

chrisjlee’s picture

chrisjlee CreditAttribution: chrisjlee commented
Status: Needs work » Needs review
Issue tags: -Novice
FileSize
1987408-overlay-convert-theme-18.patch2.76 KB

Changes as requested by @Cottser in #17

chrisjlee’s picture

chrisjlee CreditAttribution: chrisjlee commented
FileSize
interdiff-1987408-18.txt3.62 KB

Forgot Interdiff

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Needs work
Issue tags: +Novice

Nice, thanks @chrisjlee :)

Super uber nitpick then I will happily RTBC:

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -439,19 +440,11 @@ function theme_overlay_disable_message($variables) {
 }
 
+
 /**
  * Implements hook_block_access().
  */

Extra blank line added before the next function can be removed.

chrisjlee’s picture

chrisjlee CreditAttribution: chrisjlee commented
Status: Needs work » Needs review
Issue tags: -Novice
FileSize
1987408-overlay-convert-theme-21.patch2.71 KB
interdiff.txt408 bytes

Nice catch.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Reviewed & tested by the community

Thanks, looks great now :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 23357e2 and pushed to 8.x. Thanks!

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Clarify tasks