language.module - Convert theme_ functions to Twig

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.

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

#4: drupal-twig-language-1898422-4.patch queued for re-testing.

@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

Waiting on these two issues:
#1938912: Convert language content setting table theme to a twig template
#1876718: Allow modules to inject columns into tables more easily

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

#4: drupal-twig-language-1898422-4.patch queued for re-testing.

#4: drupal-twig-language-1898422-4.patch queued for re-testing.

profiling...

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]

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 :-/

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

@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

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.

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

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

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

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 }}

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

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

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.

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

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.

Forgot the patch.

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

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

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

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

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

Ok here's some fixes to language_negotiation_configure_form

language-negotiation-configure form fixes.

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

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

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

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.

Re-roll.

Tagging for reroll.

rerolled

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

@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

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.

Tagging for reroll.

Re-rolled.

74: 1898422-twig-language-74-reroll.patch queued for re-testing.

Tagging for another reroll.

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

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

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

Reroll

Manually re-tested and verified. Moving back to RTBC

Committed dbedb42 and pushed to 8.x. Thanks!