Issue #1987400 by shanethehat, TrevorBradley | Cottser: Forum.module - Convert theme_ functions to Twig.

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_forum_form converted

#1898418: forum.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

Comments

Cottser’s picture

Issue summary: View changes

Updated issue summary.

c4rl’s picture

Title: forum.module - Convert PHPTemplate templates to Twig » forum.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 #1898418: forum.module - Convert PHPTemplate templates to Twig for template conversion.

c4rl’s picture

Issue summary: View changes

Updated issue summary.

shanethehat’s picture

Status: Active » Needs review
FileSize
2.22 KB
PASSED: [[SimpleTest]]: [MySQL] 55,678 pass(es). View
hanpersand’s picture

OpenChimp’s picture

profiling this now

OpenChimp’s picture

=== 8.x..8.x compared (519fe63632755..519feb8ccb3f5):

ct : 40,580|40,580|0|0.0%
wt : 377,694|376,751|-943|-0.2%
cpu : 335,787|335,069|-718|-0.2%
mu : 50,206,616|50,206,616|0|0.0%
pmu : 50,271,728|50,271,728|0|0.0%

Profiler output

=== 8.x..1987400 compared (519fe63632755..519febd624b4e):

ct : 40,580|40,580|0|0.0%
wt : 377,694|377,921|227|0.1%
cpu : 335,787|336,040|253|0.1%
mu : 50,206,616|50,206,712|96|0.0%
pmu : 50,271,728|50,272,256|528|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

removing Profiling tag

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. After a bit of digging, determined that admin/structure/forum/add/forum needed to be my home page. Allowed Anonymous to modify forums. Tweaked bbranches to restart apache after each test to clear APC cache.

Forgive my flakey little laptop, it's 8.x vs 8.x numbers have been consistently non-zero.

=== 8.x..8.x compared (51a27ce83196d..51a27d3f1f1ae):

ct : 38,632|38,632|0|0.0%
wt : 163,432|166,046|2,614|1.6%
cpu : 160,000|164,000|4,000|2.5%
mu : 11,223,208|11,223,208|0|0.0%
pmu : 11,337,400|11,337,400|0|0.0%

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

=== 8.x..twig-forum-theme-only-1987400-2 compared (51a27ce83196d..51a27d81b1262):

ct : 38,632|38,720|88|0.2%
wt : 163,432|160,821|-2,611|-1.6%
cpu : 160,000|156,000|-4,000|-2.5%
mu : 11,223,208|11,275,312|52,104|0.5%
pmu : 11,337,400|11,389,952|52,552|0.5%

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

TrevorBradley’s picture

Issue summary: View changes

List of functions

Cottser’s picture

Status: Needs review » Needs work

We can also remove the preprocess function here now that #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead is resolved, and that would be worth re-profiling. Thanks for all the profiling so far!

drupalninja99’s picture

Remove which preprocess function?

Cottser’s picture

@drupalninja99 - template_preprocess_forum_form(). It only contains a workaround for the issue that #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead solved.

Great work lately BTW :) hope to see you in #drupal-twig sometime.

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
1.69 KB
PASSED: [[SimpleTest]]: [MySQL] 56,697 pass(es). View

Thanks! Okay here is the re-rolled patch without "template_preprocess_forum_form" and I also removed a doc reference to template_preprocess_forum_form() in one of the twig files.

Cottser’s picture

Issue tags: +Needs manual testing

Great, thanks @drupalninja99! Tagging this for manual testing.

joelpittet’s picture

thedavidmeister’s picture

Issue tags: +Novice

manual testing is a novice task

Cottser’s picture

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

Needs a reroll before it can be tested or profiled.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
640 bytes
885 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,919 pass(es). View

re-rolled and removed @see template_preprocess() from the twig template.

Cottser’s picture

Status: Needs review » Needs work

Latest patch doesn't remove theme_forum_form(), will be looking at this and hopefully manual testing during a sprint tomorrow.

Cottser’s picture

Scratch my last comment, not enough sprinters showed up… anyone can work on updating the patch and manual testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
831 bytes
1.37 KB
PASSED: [[SimpleTest]]: [MySQL] 58,427 pass(es). View

Removed theme_forum_form

CaptainWonky’s picture

FileSize
1.49 KB
PASSED: [[SimpleTest]]: [MySQL] 58,923 pass(es). View

rerolled

joelpittet’s picture

Issue tags: -Novice, -Needs manual testing

Thanks @CaptainWonky

Manual testing looks good.

+<!-- BEGIN OUTPUT from 'core/modules/forum/templates/forum-form.html.twig' -->
 <div class="form-item form-type-textfield form-item-name">
  <label for="edit-name">Forum name<abbr class="form-required" title="This field is required.">*</abbr></label> <input aria-describedby="edit-name--description" type="text" id="edit-name" name="name" value="General discussion" size="60" maxlength="255" class="form-text required" required="required" aria-required="true" />
 <div class="description" id="edit-name--description">Short but meaningful name for this collection of threaded discussions.</div>
@@ -17,8 +18,10 @@
  <label for="edit-parent-0">Parent<abbr class="form-required" title="This field is required.">*</abbr></label> <select aria-describedby="edit-parent-0--description" id="edit-parent-0" name="parent[0]" class="form-select required" required="required" aria-required="true"><option value="0" selected="selected">&lt;root&gt;</option></select>
 <div class="description" id="edit-parent-0--description">Forums may be placed at the top (root) level, or inside another container or forum.</div>
 </div>
-<input type="hidden" name="form_build_id" value="form-LfltB-bkQb37R9Kgf3WklYCxLOA-lLv7I-3-x9CFb78" /><input type="hidden" name="form_token" value="vMOzncStTLdU8IxgvwUd69C8ixMZoS1xiOARLt9Cds8" /><input type="hidden" name="form_id" value="forums_taxonomy_term_forum_form" /><div class="form-item form-type-textfield form-item-path-alias">
+<input type="hidden" name="form_build_id" value="form-r00KvyqgPQyfySY4NdC9gRucGcFciYNhZ8vnDikg2oQ" /><input type="hidden" name="form_token" value="vMOzncStTLdU8IxgvwUd69C8ixMZoS1xiOARLt9Cds8" /><input type="hidden" name="form_id" value="forums_taxonomy_term_forum_form" /><div class="form-item form-type-textfield form-item-path-alias">
  <label for="edit-path-alias">URL alias</label> <input aria-describedby="edit-path-alias--description" type="text" id="edit-path-alias" name="path[alias]" value="" size="60" maxlength="255" class="form-text" />
 <div class="description" id="edit-path-alias--description">Optionally specify an alternative URL by which this term can be accessed. Use a relative path and don't add a trailing slash or the URL alias won't work.</div>
 </div>
 <div class="form-actions form-wrapper" id="edit-actions"><input type="submit" id="edit-submit" name="op" value="Save" class="button button-primary form-submit" /><input type="submit" id="edit-delete" name="op" value="Delete" class="button button-danger form-submit" /></div>
+
+<!-- END OUTPUT from 'core/modules/forum/templates/forum-form.html.twig' -->

Btw that ^^ is a diff between 8.x and this branch. The only diff being the form_build_id and a newline character extra at the EOF.

joelpittet’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Assigned: Unassigned » joelpittet

Going to profile this on the plane tomorrow :)

Cottser’s picture

Nothin' like a good plane profiling :D

joelpittet’s picture

Issue tags: -Needs profiling

Scenario:

  • Forum module turned on
  • Frontpage set to admin/structure/forum/add/forum
  • Administer forums permissions turned on
=== 8.x..8.x compared (523bd0c5929e9..523bd1075886b):

ct  : 47,754|47,754|0|0.0%
wt  : 324,917|326,667|1,750|0.5%
cpu : 292,222|293,061|839|0.3%
mu  : 14,855,544|14,855,544|0|0.0%
pmu : 14,945,824|14,945,824|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523bd0c5929e9&...

=== 8.x..1987400-twig-theme-forum compared (523bd0c5929e9..523bd15c0308e):

ct  : 47,754|47,842|88|0.2%
wt  : 324,917|327,140|2,223|0.7%
cpu : 292,222|294,689|2,467|0.8%
mu  : 14,855,544|14,908,288|52,744|0.4%
pmu : 14,945,824|15,006,088|60,264|0.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523bd0c5929e9&...

joelpittet’s picture

Assigned: joelpittet » Unassigned
FileSize
1.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,721 pass(es). View

Here's a minor re-roll too that I needed to do before profiling.

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

The last submitted patch, 1987400-27-twig-theme-forum.patch, failed testing.

Cottser’s picture

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

#27: 1987400-27-twig-theme-forum.patch queued for re-testing.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks good it is ready to fly. Minor issue.

+++ b/core/modules/forum/templates/forum-form.html.twig
@@ -0,0 +1,13 @@
+ * @see

Should be @file can be fixed at the time of commit.

joelpittet’s picture

FileSize
1.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987400-31-twig-forum-theme.patch. Unable to apply patch. See the log in the details link for more information. View
521 bytes

Thanks at @jibran

David Hernández’s picture

#31: 1987400-31-twig-forum-theme.patch queued for re-testing.

joelpittet’s picture

Status: Reviewed & tested by the community » Postponed

Postponing this on #2092343: Consolidate forum.module and remove call to _theme_table_cell() within template_preprocess_forum_topic_list()

Discussed with @markcarver don't want to send another single variable template conversion through to core.

Once that one get's sorted this can be closed as duplicate or won't fix.

joelpittet’s picture

Issue summary: View changes

Update conversion table

Cottser’s picture

joelpittet’s picture

Status: Postponed » Needs work

The last submitted patch, 31: 1987400-31-twig-forum-theme.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
PASSED: [[SimpleTest]]: [MySQL] 64,290 pass(es). View

Re-roll. Maybe consider just removing this?

Cottser’s picture

IMO this should just be a theme_form/form.html.twig suggestion if possible.

form__forum maybe.

joelpittet’s picture

I tried that and it failed on both attempts locally.

diff --git a/core/modules/forum/lib/Drupal/forum/Form/ForumFormController.php b/core/modules/forum/lib/Drupal/forum/Form/ForumFormController.php
index ae16f90..3c267ef 100644
--- a/core/modules/forum/lib/Drupal/forum/Form/ForumFormController.php
+++ b/core/modules/forum/lib/Drupal/forum/Form/ForumFormController.php
@@ -54,7 +54,7 @@ public function form(array $form, array &$form_state) {
     $form['parent']['#tree'] = TRUE;
     $form['parent'][0] = $this->forumParentSelect($taxonomy_term->id(), $this->t('Parent'));

-    $form['#theme'] = 'forum_form';
+    $form['#theme_wrappers'] = 'form__forum';
     $this->forumFormType = $this->t('forum');
     return $form;
   }

and

diff --git a/core/modules/forum/lib/Drupal/forum/Form/ForumFormController.php b/core/modules/forum/lib/Drupal/forum/Form/ForumFormController.php
index ae16f90..3c267ef 100644
--- a/core/modules/forum/lib/Drupal/forum/Form/ForumFormController.php
+++ b/core/modules/forum/lib/Drupal/forum/Form/ForumFormController.php
@@ -54,7 +54,7 @@ public function form(array $form, array &$form_state) {
     $form['parent']['#tree'] = TRUE;
     $form['parent'][0] = $this->forumParentSelect($taxonomy_term->id(), $this->t('Parent'));

-    $form['#theme'] = 'forum_form';
+    $form['#theme'] = 'form__forum';
     $this->forumFormType = $this->t('forum');
     return $form;
   }

Maybe you want to take a stab at this @Cottser?

Cottser’s picture

Status: Needs review » Needs work

Let's go needs work for now.

jerdavis’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,216 pass(es), 46 fail(s), and 21 exception(s). View

Re-rolling based on #39. This seems to be working locally, let's see what test bot has to say

Status: Needs review » Needs work

The last submitted patch, 41: 1987400-twig-forum-theme-41.patch, failed testing.

joelpittet’s picture

@jerdavis just noticed that this is missing the twig template. Would you mind adding in?

@Cottser, I think we should push this one in and we can still work on that other issue for consolidation of these.
#2265991: Replace theme_*_form() with theme suggestion for #theme => form.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,154 pass(es). View

I think no reason to consolidate removals by allowing module maintainers to review the changes.
Suppose better to re-title this to "remove theme_forum_form()"

+++ b/core/modules/forum/src/Form/ForumForm.php
@@ -54,7 +54,7 @@ public function form(array $form, array &$form_state) {
+    $form['#theme_wrappers'] = 'form__forum';

should be array

andypost’s picture

FileSize
862 bytes

interdiff

joelpittet’s picture

@andypost this seems like a good solution... just can't seem to test it out. I'd assume you could just go to admin/structure/forum/add/forum after installing the forum module and see the form. or admin/structure/taxonomy/manage/forums/add

But none of them produce this form from what I can see. I wanted to just see if the seven or bartik template override of form.html.twig as form--forum.html.twig would pick up. Any idea?

andypost’s picture

FileSize
1.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,969 pass(es). View
29.89 KB
31.23 KB

reroll

this form used in forum.add_forum route /admin/structure/forum/add/forum

no difference

before

after

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
103.41 KB

Thanks for the re-roll @andypost and for confirming the path for me. I was looking for the string "Short but meaningful name for this collection of threaded discussions." But that didn't show up, but either something else is broken or it's getting overwritten someplace else, nevertheless out of the scope of this.

I gave this a test and it works like a charm:

  1. Added a copy of form.html.twig to: /themes/seven/templates/form--forum.html.twig
  2. Turned on twig debug.
  3. Rebuilt cache.
  4. Went to /admin/structure/forum/add/forum

Just a note, the before and after screenshots you posted don't give much detail though they are appreciated, showing that nothing has changed can yield false positives.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5a07eb9 and pushed to 8.x. Thanks!

  • alexpott committed 5a07eb9 on 8.x
    Issue #1987400 by joelpittet, andypost, drupalninja99, jerdavis,...

Status: Fixed » Closed (fixed)

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