Issue #1898430 by EVIIILJ, Cottser, TrevorBradley | c4rl: Convert menu module to Twig.

Task

Use Twig instead of PHPTemplate

Remaining

* replace all tpl.php files with .html.twig equivalents
* replace all theme functions with .html.twig equivalent templates
* add new preprocess functions for the .html.twig equivalent templates
* update all hook_theme definitions

#1757550: [Meta] Convert core theme functions to Twig templates
#1938918: Convert menu theme tables to table #type

Files: 
CommentFileSizeAuthor
#7 1898430-7.patch5.02 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,859 pass(es). View
#7 interdiff.txt1.77 KBCottser
#5 1898430-5.patch4.33 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 52,274 pass(es). View

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

meeli’s picture

Assigned: Unassigned » meeli

i'm having a look at this today

meeli’s picture

Assigned: meeli » Unassigned

bailing because of http://drupal.org/node/1824542 ... sorry!

Cottser’s picture

Assigned: Unassigned » Cottser
Cottser’s picture

Status: Active » Needs review
FileSize
4.33 KB
PASSED: [[SimpleTest]]: [MySQL] 52,274 pass(es). View

I took the patch from #1824542: Convert theme_menu_overview_form to twig in the sandbox and reworked to get rid of theme() and drupal_render() calls per http://drupal.org/node/1920746. I had to insert some #weight's in a few spots to get the elements to render in the correct order. Rendering the form children didn't seem necessary so I took that out.

I diff'ed the markup before and after conversion and everything lines up.

The only other thing of note, the empty text should probably be moved to the Twig template. I added a @todo for this in the patch, I'm not sure whether we want to handle the empty text in this issue or not, because in doing so we probably wouldn't render the empty text inside a table. In the meantime, I updated the colspan of the empty text, something that I think was missed in the dropbutton conversion issue - #1480854: Convert operation links to '#type' => 'operations'.

Cottser’s picture

Status: Needs review » Needs work

After talking to @jenlampton I think we do need to render the form children.

Cottser’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
5.02 KB
PASSED: [[SimpleTest]]: [MySQL] 55,859 pass(es). View

Not sure if this is the best way to handle the (potential) children, see the interdiff.

artofeclipse’s picture

Status: Needs review » Reviewed & tested by the community

works great, +1 :)

xjm’s picture

#7: 1898430-7.patch queued for re-testing.

joelpittet’s picture

@Cottser There is a related issue at #1938918: Convert menu theme tables to table #type that I tried taking a stab at, since you got half way and I got half way maybe you could take a look at the 'theme_menu_overview_form' convert to #type=>table and see if you can make the weights work and I will gladly re-roll this without that twig conversion:)

Cottser’s picture

@joelpittet - thanks, left a comment there.

Cottser’s picture

Status: Reviewed & tested by the community » Postponed

I've started looking into #1938918: Convert menu theme tables to table #type, and since that is the only theme function being converted here, I'm postponing this issue on #1938918: Convert menu theme tables to table #type. Ultimately this issue can probably be closed as a duplicate.

Cottser’s picture

Issue summary: View changes

Add commit message to issue summary.

Cottser’s picture

Issue summary: View changes

Add #type table issue to related

c4rl’s picture

Title: Convert menu module to Twig » foo.menu - 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, which are lower in priority than PHPTemplate conversion issues.

Cottser’s picture

Title: foo.menu - Convert theme_ functions to Twig » menu.module - Convert theme_ functions to Twig

Fixing title :) thanks @c4rl!

Cottser’s picture

Assigned: Cottser » Unassigned
jstoller’s picture

Status: Postponed » Needs review

#7: 1898430-7.patch queued for re-testing.

cellear’s picture

Assigned: Unassigned » cellear
Issue tags: +Needs profiling

I'm working on this one (at the DrupalCon PDX Twig sprint)

cellear’s picture

Assigned: cellear » Unassigned

Out of time. Releasing, still untested :(

OpenChimp’s picture

profiling

OpenChimp’s picture

Issue tags: -Needs profiling

=== 8.x..8.x compared (519ff86781c9d..519ff8d91be1f):

ct : 40,580|40,580|0|0.0%
wt : 377,172|377,275|103|0.0%
cpu : 335,048|335,967|919|0.3%
mu : 50,206,616|50,206,616|0|0.0%
pmu : 50,271,728|50,271,728|0|0.0%

Profiler output

=== 8.x..1898430 compared (519ff86781c9d..519ff937164bc):

ct : 40,580|40,580|0|0.0%
wt : 377,172|377,130|-42|-0.0%
cpu : 335,048|335,630|582|0.2%
mu : 50,206,616|50,207,120|504|0.0%
pmu : 50,271,728|50,272,744|1,016|0.0%

Profiler output

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
The registry has been rebuilt. [success]
'all' cache was cleared in self

OpenChimp’s picture

=== 8.x..8.x compared (519ff86781c9d..519ff8d91be1f):

ct : 40,580|40,580|0|0.0%
wt : 377,172|377,275|103|0.0%
cpu : 335,048|335,967|919|0.3%
mu : 50,206,616|50,206,616|0|0.0%
pmu : 50,271,728|50,271,728|0|0.0%

Profiler output

=== 8.x..1898430 compared (519ff86781c9d..519ff937164bc):

ct : 40,580|40,580|0|0.0%
wt : 377,172|377,130|-42|-0.0%
cpu : 335,048|335,630|582|0.2%
mu : 50,206,616|50,207,120|504|0.0%
pmu : 50,271,728|50,272,744|1,016|0.0%

Profiler output

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
The registry has been rebuilt. [success]
'all' cache was cleared in self

OpenChimp’s picture

Issue tags: +Needs profiling

profiling not run correctly. Needs to be redone

TrevorBradley’s picture

Assigned: Unassigned » TrevorBradley

I'll profile this.

TrevorBradley’s picture

Assigned: TrevorBradley » Unassigned

Stark theme. Set home page to admin/config/system/site-information - the admin menu is the most complex. Had to turn on pretty much every administer permission to allow anonymous to allow the menu to populate.

=== 8.x..8.x compared (51a287416d8a8..51a287c51097c):

ct : 137,949|137,949|0|0.0%
wt : 509,128|508,053|-1,075|-0.2%
cpu : 504,000|504,000|0|0.0%
mu : 14,976,184|14,976,184|0|0.0%
pmu : 15,827,728|15,827,728|0|0.0%

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

=== 8.x..menu-1898430-7 compared (51a287416d8a8..51a28832b7403):

ct : 137,949|139,116|1,167|0.8%
wt : 509,128|512,664|3,536|0.7%
cpu : 504,000|508,000|4,000|0.8%
mu : 14,976,184|15,043,888|67,704|0.5%
pmu : 15,827,728|15,309,080|-518,648|-3.3%

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

I'll leave it up to you if a 1% increase is bad or not.

TrevorBradley’s picture

Issue summary: View changes

Remove sandbox link

joelpittet’s picture

Status: Needs review » Postponed
Issue tags: -Needs profiling
BarisW’s picture

BarisW’s picture

Issue summary: View changes

Updated issue summary.

Cottser’s picture

mgifford’s picture

Status: Postponed » Active

Seems this isn't held up any longer.

lauriii’s picture

Assigned: Unassigned » lauriii

Gonna check whats happening there

lauriii’s picture

Assigned: lauriii » Unassigned
Status: Active » Closed (won't fix)

I dont think this is needed anymore. There's no more menu.module and no menu related theme functions.

joelpittet’s picture

Status: Closed (won't fix) » Closed (duplicate)

Yes, fixed elsewhere, thanks for checking up on this issue:)