Task

Use Twig instead of PHPTemplate

Remaining

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

These two themed tables will be converted in separate issues:
language_negotiation_configure_browser_form_table
language_content_settings_table

This is the only conversion in this issue:
language_negotiation_configure_form

Steps to test

  1. Enable all multilingual language modules (shows more negotiation types)
  2. Go to /admin/config/regional/language/detection
  3. Compare the markup before and after the patch.
Files: 
CommentFileSizeAuthor
#83 1898422-83.patch6.87 KBlokapujya
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,945 pass(es). View

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

duellj’s picture

Given #1876712: [meta] Convert all tables in core to new #type 'table', this issue should be closed, since there's no other theme functions in language module outside of table themes.

c4rl’s picture

Let's wait until #1876712: [meta] Convert all tables in core to new #type 'table' is marked fixed before deciding whether we should close this one.

joelpittet’s picture

Assigned: Unassigned » joelpittet
Status: Active » Needs review
FileSize
9.92 KB
FAILED: [[SimpleTest]]: [MySQL] 55,750 pass(es), 16 fail(s), and 65 exception(s). View

Passed tests locally. This needs some docblock and preprocess doc help.

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

The last submitted patch, drupal-twig-language-1898422-4.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

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

The last submitted patch, drupal-twig-language-1898422-4.patch, failed testing.

duellj’s picture

@joelpittet, thanks for the work! We should probably hold off on this, though, until it's decided we actually need dedicated theme functions for the language module.

See #1938912: Convert language content setting table theme to a twig template where all of the theme functions have been removed

joelpittet’s picture

joelpittet’s picture

Status: Postponed » Closed (won't fix)

This will be covered by #type=>table conversion in #1938912: Convert language content setting table theme to a twig template

c4rl’s picture

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

joelpittet’s picture

Assigned: joelpittet » Unassigned
jstoller’s picture

Status: Closed (won't fix) » Needs review
Issue tags: -Twig

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

The last submitted patch, drupal-twig-language-1898422-4.patch, failed testing.

johannez’s picture

Assigned: Unassigned » johannez
johannez’s picture

Status: Needs work » Needs review
johannez’s picture

Assigned: johannez » Unassigned

Status: Needs review » Needs work

The last submitted patch, drupal-twig-language-1898422-4.patch, failed testing.

johannez’s picture

Assigned: Unassigned » johannez
johannez’s picture

Assigned: johannez » Unassigned
Status: Needs work » Needs review
FileSize
10.55 KB
None View
ernie-g’s picture

profiling...

ernie-g’s picture

profiling bbranches output for : twig-language-1898422-20.patch

=== 8.x..8.x compared (519fe99884ff9..51a002e89a693):

ct  : 71,226|71,226|0|0.0%
wt  : 466,475|483,746|17,271|3.7%
cpu : 419,918|418,487|-1,431|-0.3%
mu  : 16,002,240|16,002,240|0|0.0%
pmu : 16,130,312|16,130,312|0|0.0%

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

=== 8.x..profiling-1898422 compared (519fe99884ff9..51a003112df7e):

ct  : 71,226|71,226|0|0.0%
wt  : 466,475|497,844|31,369|6.7%
cpu : 419,918|418,120|-1,798|-0.4%
mu  : 16,002,240|16,002,176|-64|-0.0%
pmu : 16,130,312|16,129,984|-328|-0.0%

<a href="http://drupal8.local/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=519fe99884ff9&run2=51a003112df7e&extra=8.x..profiling-1898422">Profiler output</a>

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
The drush command 'rr' could not be found.  Run `drush cache-clear drush` to clear the commandfile cache if you have installed new extensions.    [error]
'all' cache was cleared in self                                                                                                                   [success]
ernie-g’s picture

MANY APOLOGIES
It turns out, the profiling instructions I received at the DrupalCon Code Sprint was incomplete.
I am going to be re-running all my profiling :-/

tlattimore’s picture

FileSize
10.55 KB
PASSED: [[SimpleTest]]: [MySQL] 55,815 pass(es). View

Re-queueing the patch from #20 for testing. It didn't ever get run due to infrastructure going down last Friday at code sprint.

joelpittet’s picture

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

@ernie-g jump on IRC and we can probably fill in the gaps. It looks like your system is setup but the baseline was off, try to get the baseline 8.x vs 8.x runs between 0.5% and -0.5% wall time. Also, so people can repeat your profiling tests, could you post the Scenario you ran to get the results.

3% variation is not good but maybe there are some improvements we can help it with some preprocess cleanup?


+++ b/core/modules/language/language.admin.inc
@@ -514,21 +518,30 @@ function theme_language_negotiation_configure_form($variables) {
+      '#header' => $header,
+      '#rows' => $rows,
+      '#attributes' => array('id' => "language-negotiation-methods-$type"),

I am pretty sure type table doesn't use theme table's #rows because the rows are built like a #tree for the form api. Maybe this just works now but would like to confirm the form is saving as it should be.

+++ b/core/modules/language/language.admin.inc
@@ -514,21 +518,30 @@ function theme_language_negotiation_configure_form($variables) {
+    $table_children = drupal_render_children($form[$type]);

Should remove drupal_render_children() re #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead

drupalninja99’s picture

Ok I am attaching a mini-patch with some potential fixes:

1. I patched language_theme to include the line - 'file' => 'language.admin.inc' for language_negotiation_configure_form. This was left out previously
2. I noticed that the configure form is all screwed up even with the first fix. The table gets rendered but then you get the same form elements in a non-table.
3. $variables['children'] = drupal_render_children($form); wasn't doing anything (that I saw) and I saw that you removed it so I removed that line and {% if children %}{{ children }}{% endif %} from the twig template.
4. I removed the $table_children = drupal_render_children($form[$type]); stuff and {{ form_item.children }} from the twig template for language_negotiation_configure_form bc it was spitting out the form output without rendering it in a proper table.

I don't know enough ab the code to know what purpose it's supposed to serve. If the idea is just to render the table that I am attaching in the screengrab, then we don't seem to need this extra children rendering stuff. It is my understanding we are trying to get rid of drupal_render and drupal_render_children so since we are building a table in the preprocess function it seems like we don't need those other drupal_render calls.

drupalninja99’s picture

I can re-roll this into the #24 patch if it makes sense to you. Let me know, thanks!

jenlampton’s picture

@drupalninja99 the reason for having children printed in the template is to have a place to print other things that are added into the form via form_alter, that shouldn't be in the 'table'. I think we still need that there for extendability - but we can get rid of the call to drupal_render_children() and just pass the data straight through.

Can someone put this and your other changes inta a .patch file so test bot can test it for us?

drupalninja99’s picture

@jenlampton, how would we pass the data straight through?

jenlampton’s picture

Status: Needs work » Needs review
FileSize
9.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-language-1898422-20_0_0.patch. Unable to apply patch. See the log in the details link for more information. View

Giving this a shot...

Rerolled the patch in #24 since it didn't apply anymore, then removed drupal_render_children as well as @drupalninja99's fix for #1, and removed * @see template_preprocess() from the template files.

I can see now what you mean, @drupalninja99, about children in 3 and 4. The form doesn't look "all screwed up" anymore, however, we were missing the submit buttons by removing the final children in the template, and that will also remove anything else that anyone form-alter's into this form.

I've temporarily added the button back to the form by adding a line...

$variables['children'] = $variables['form']['actions'];

But I think this whole preprocess function is going to need to be re-worked in order for us to be able have a fully functional form. I think the problem comes from creating a copy of $variables['form'] and inserting that back inside $variables['form_items']. In a perfect world we would just rearrange the form we had, and we wouldn't end up printing all the elements twice when we render the rest of the form.

Also, templates like this should be removed:

+{#
+/**
+* @file
+* Default theme implementation for a browser configuration form as table.
+*
+* Available variables:
+* - table: @todo.
+*
+* @see template_preprocess_language_negotiation_configure_browser_form_table()
+*
+* @ingroup themeable
+*/
+#}
+{{ table }}

Status: Needs review » Needs work

The last submitted patch, twig-language-1898422-20_0.patch, failed testing.

drupalninja99’s picture

@Jen I can't get the language.admin.inc to apply, do you know what commit id you created your patch against?

drupalninja99’s picture

Hi Jen, can you try to re-roll your patch?

pplantinga’s picture

Status: Needs work » Needs review
FileSize
7.66 KB
FAILED: [[SimpleTest]]: [MySQL] 57,050 pass(es), 161 fail(s), and 52 exception(s). View

Manual re-roll, tried to keep it as consistent as possible, but I'm running into lots of errors... Someone who knows more should work on this.

Also #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead is in, so the @todo will need to be done.

Status: Needs review » Needs work

The last submitted patch, twig-language-1898422-34.patch, failed testing.

stevector’s picture

Assigned: Unassigned » stevector

I'm looking at this patch right now at the Twin Cities Drupalcamp sprint.

stevector’s picture

Status: Needs work » Needs review

This should get test passing again. The last patch was missing the .twig files added in previous patches. I copied those files from comment 20.

The todo for "children" is still in there. I'm not sure of the best way to resolve that todo. The interdiff also shows that I added back more drupal_render() calls. I did this to prevent individual form items from rendering again at the end of the form when form.children prints.

stevector’s picture

FileSize
3.7 KB
10.18 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/language/language.admin.inc. View

Forgot the patch.

stevector’s picture

Assigned: stevector » Unassigned

Status: Needs review » Needs work

The last submitted patch, twig-language-1898422-38.patch, failed testing.

Les Lim’s picture

Status: Needs work » Needs review
FileSize
10.19 KB
FAILED: [[SimpleTest]]: [MySQL] 57,364 pass(es), 115 fail(s), and 114 exception(s). View

Same patch as #38, fixing an uncommented @todo line.

Status: Needs review » Needs work

The last submitted patch, twig-language-1898422-41.patch, failed testing.

jenlampton’s picture

@stevector we need to remove the calls to drupal_render, specifically so that "the rest" of the form *will* render at the bottom as part of children. Nothing should render twice, since each item gets marked as printed when it's rendered. If that's not happening we should figure out why (are we creating a copy of the renderable somewhere and rendering that instead of the actual form?).

stevector’s picture

@jenlampton Yes, the previous patch in 34 has this section which copies section of the $form array into the array sent to the table.

$row = array(
           'data' => array(
-            '<strong>' . drupal_render($form[$type]['title'][$id]) . '</strong>',
-            drupal_render($form[$type]['description'][$id]),
-            drupal_render($form[$type]['enabled'][$id]),
-            drupal_render($form[$type]['weight'][$id]),
+            array('data' => array(
+              '#prefix' => '<strong>',
+              $form[$type]['title'][$id]),
+              '#suffix' => '</strong>',
+            ),
+            array('data' => $form[$type]['description'][$id]),
+            array('data' => $form[$type]['enabled'][$id]),
+            array('data' => $form[$type]['weight'][$id]),
           ),

So descriptions, 'enabled' and weights were all rendered in the table (as their copied selves) and at the bottom of the output as the unrendered pieces of $form.

Perhaps #1938912: Convert language content setting table theme to a twig template needs to happen before the twig conversion, or both at the same time.

jenlampton’s picture

Our work-around in other issues has been to unset specific keys from the $form if we had to pull them out and render them separately. Maybe we should do that here to get this in.

We can revisit again after #1938912: Convert language content setting table theme to a twig template

joelpittet’s picture

Status: Needs work » Needs review
FileSize
11.11 KB
FAILED: [[SimpleTest]]: [MySQL] 59,291 pass(es), 142 fail(s), and 128 exception(s). View
5.33 KB

Converted language-negotiation-configure-browser-form-table into a proper #type=>table and removed the template. It was type=>table already actually just when you pass in #rows it's actually just doing #theme=>table without any of the supposed helpful stuff that #type=>table does...

joelpittet’s picture

FileSize
6.8 KB
12.02 KB
FAILED: [[SimpleTest]]: [MySQL] 59,185 pass(es), 137 fail(s), and 153 exception(s). View

Ok here's some fixes to language_negotiation_configure_form

joelpittet’s picture

FileSize
3.68 KB
13.55 KB
FAILED: [[SimpleTest]]: [MySQL] 59,298 pass(es), 50 fail(s), and 126 exception(s). View

language-negotiation-configure form fixes.

Status: Needs review » Needs work

The last submitted patch, 1898422-48-twig-language.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
4.24 KB
16.76 KB
FAILED: [[SimpleTest]]: [MySQL] 59,400 pass(es), 42 fail(s), and 445 exception(s). View

language_content_settings_table gets abused by _content_translation_preprocess_language_content_settings_table so I've done a couple things I think is in the right direction... we'll see...

Status: Needs review » Needs work

The last submitted patch, 1898422-50-twig-language.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
20.59 KB
FAILED: [[SimpleTest]]: [MySQL] 57,949 pass(es), 2 fail(s), and 0 exception(s). View
5.2 KB

Further down the rabbit hole. I expect less failures and exceptions:) But should be a few left still.

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -Needs profiling, -Twig

The last submitted patch, 1898422-52-twig-language.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Remove link to sandbox, add related #type table conversion

joelpittet’s picture

Status: Needs work » Needs review

52: 1898422-52-twig-language.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 52: 1898422-52-twig-language.patch, failed testing.

joelpittet’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.6 KB
20.74 KB
FAILED: [[SimpleTest]]: [MySQL] 57,976 pass(es), 2 fail(s), and 0 exception(s). View

This fixes some issues with rendering but doesn't fix the two bugs, I think they need to be fixed with #parents keys on the columns but not sure which form yet.

joelpittet’s picture

Issue tags: +Twig
joelpittet’s picture

Status: Needs review » Needs work

The last submitted patch, 56: 1898422-56-twig-language.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
20.75 KB
FAILED: [[SimpleTest]]: [MySQL] 60,138 pass(es), 7 fail(s), and 0 exception(s). View

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 60: 1898422-60-twig-language.patch, failed testing.

Cottser’s picture

Issue tags: +Needs reroll

Tagging for reroll.

Salah Messaoud’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
17.31 KB
FAILED: [[SimpleTest]]: [MySQL] 62,914 pass(es), 189 fail(s), and 3 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 63: twig-language-1898422.patch, failed testing.

The last submitted patch, 63: twig-language-1898422.patch, failed testing.

Salah Messaoud’s picture

Status: Needs work » Needs review
FileSize
19.05 KB
FAILED: [[SimpleTest]]: [MySQL] 63,317 pass(es), 108 fail(s), and 2 exception(s). View

rerolled

Status: Needs review » Needs work

The last submitted patch, 66: twig-language-1898422-66.patch, failed testing.

rteijeiro’s picture

Status: Needs work » Needs review

66: twig-language-1898422-66.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 66: twig-language-1898422-66.patch, failed testing.

joelpittet’s picture

@Salah Messaoud Thanks for the re-roll. Would you mind removing the language-content-settings-table conversion from this patch?

It should have less or no errors then and I've done that conversion over at #1938912: Convert language content setting table theme to a twig template

Cottser’s picture

Issue tags: +Twig conversion
joelpittet’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Needs manual testing, +Novice
FileSize
6.91 KB
PASSED: [[SimpleTest]]: [MySQL] 64,593 pass(es). View

Here's a re-roll and added some steps to test in the issue summary.

I'm going to omit the Needs profiling on this one because it's highly unlikely this will have a greater than 1% performance regression and is for an admin page.

Cottser’s picture

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

Tagging for reroll.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,416 pass(es). View

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 74: 1898422-twig-language-74-reroll.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Cottser’s picture

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

Tagging for another reroll.

lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1898422-78.patch. Unable to apply patch. See the log in the details link for more information. View

The only difference I see in the markup is the white space between some divs, and that's fine.

joelpittet’s picture

Did manual testing and the before and after relevant code. Both with and without whitespace normalization.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, manually tested in in #79 and doesn't need profiling as mentioned in #72

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 78: 1898422-78.patch, failed testing.

lokapujya’s picture

Issue tags: +Needs reroll
lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,945 pass(es). View

Reroll

jerdavis’s picture

Status: Needs review » Reviewed & tested by the community

Manually re-tested and verified. Moving back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed dbedb42 and pushed to 8.x. Thanks!

  • Commit dbedb42 on 8.x by alexpott:
    Issue #1898422 by joelpittet, lokapujya, Salah Messaoud, stevector, Les...

Status: Fixed » Closed (fixed)

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