Task

Convert the following theme functions to use the new table #type:

Theme function name Where in Code What is it really? Fixed in
theme_language_content_settings_table function form table this issue
theme_language_admin_overview_form_table function form table draggable #1877338: Convert language admin form to new #type 'table'
theme_language_negotiation_configure_browser_form_table function form table #2422481: Convert language negotiation theme table to table #type
theme_language_negotiation_configure_form function form table draggable #1898422: language.module - Convert theme_ functions to Twig

Remaining tasks

  • Remove theme function
  • Remove theme preprocess function
  • Fix the UI on this page: admin/config/regional/content-language.
  • Review patch
  • Manual testing

User interface changes

prior to this patch the tables are hidden until the checkbox is clicked. But with the patch, they are all visible initially. UI: admin/config/regional/content-language

Manual testing steps

  1. Install Drupal 8.
  2. Enable the language module
  3. Visit the Content language page at admin/config/regional/content-language
  4. Checking the boxes under the " Custom language settings" heading will reveal the elements using '#type' => 'table'

Related issues

CommentFileSizeAuthor
#71 convert_language-1938912-71.patch3.52 KBjoelpittet
#71 interdiff.txt1.21 KBjoelpittet
#69 convert_language-1938912-69.patch3.56 KBjoelpittet
#69 interdiff.txt1.99 KBjoelpittet
#65 convert_language-1938912-65.patch3.2 KBjoelpittet
#61 1938912-61-convert-theme-language-content-setting-table.patch6.95 KBRainbowArray
#57 1938912-57.patch6.98 KBlokapujya
#57 interdiff-1938912-57.txt1.85 KBlokapujya
#52 interdiff.txt1.79 KBstar-szr
#52 convert_language-1938912-52.patch5.73 KBstar-szr
#50 convert_language-1938912-50.patch5.54 KBakalata
#47 convert_language-1938912-47.patch5.52 KBManuel Garcia
#47 interdiff.txt1.51 KBManuel Garcia
#43 1938912-43.patch5.21 KBlokapujya
#43 interdiff-1938912-43.txt933 byteslokapujya
#40 interdiff.txt2.01 KBManuel Garcia
#40 convert_language-1938912-40.patch4.3 KBManuel Garcia
#39 Screen Shot 2015-06-10 at 10.07.43 AM.png115.16 KBlokapujya
#29 convert_language-1938912-29.patch5.05 KBManuel Garcia
#27 interdiff.txt773 bytesManuel Garcia
#27 convert_language-1938912-27.patch0 bytesManuel Garcia
#26 1938912-26.patch5.06 KBrpayanm
#20 1938912-20.patch5.12 KBlokapujya
#14 1938912-language-type-table-14.patch9.02 KBjoelpittet
#9 language-tables-1938912-9.patch1.67 KBpplantinga
#5 1938912-3-language-tables.patch28.05 KBduellj
#1 1938912-1-language-tables.patch27.44 KBduellj
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

duellj’s picture

Status: Active » Needs review
FileSize
27.44 KB

First pass at converting the three language tables to table #types

Status: Needs review » Needs work

The last submitted patch, 1938912-1-language-tables.patch, failed testing.

duellj’s picture

Status: Needs work » Needs review

This patch fixes most of the failing tests. There's still one problem though:

_translation_entity_form_language_content_settings_form_alter() alters the language_content_form to inject columns into the table. Should probably wait until #1876718: Allow modules to inject columns into tables more easily is figured out before fixing.

star-szr’s picture

@duellj - I think we're missing a patch upload here for #3, if you still have it please upload :)

duellj’s picture

Whoops, thanks Cottser! Here's the patch I intended to upload. Let's see if it still applies or needs a reroll

Status: Needs review » Needs work

The last submitted patch, 1938912-3-language-tables.patch, failed testing.

jibran’s picture

Issue tags: +Novice

Tagging.

jibran’s picture

Tagging.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
1.67 KB

It looked like there were quite a few unrelated changes in the last patch, so I just rolled my own.

Status: Needs review » Needs work

The last submitted patch, language-tables-1938912-9.patch, failed testing.

pplantinga’s picture

um. This is obviously more complicated than I thought it was, so someone else should take a look, and probably ignore my patch.

jibran’s picture

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

#9: language-tables-1938912-9.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, language-tables-1938912-9.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Add related Twig conversion

Hydra’s picture

Issue summary: View changes

Strike theme_language_admin_overview_form_table, since it has become an _entity_list: 'language_entity'

joelpittet’s picture

Assigned: duellj » joelpittet
Issue summary: View changes
Status: Needs work » Needs review
FileSize
9.02 KB

This should do the trick.

Status: Needs review » Needs work

The last submitted patch, 14: 1938912-language-type-table-14.patch, failed testing.

joelpittet’s picture

Assigned: joelpittet » Unassigned

Ok I think we need sun's help for this one... It's a bit of inception going on here with the form alter manipulating the table. And I'm not sure if I dealt with the keys right. Needs content_translation's theme functions removed and merged in some how.

akalata’s picture

Issue summary: View changes
akalata’s picture

Issue summary: View changes
akalata’s picture

Issue summary: View changes
lokapujya’s picture

Title: Convert language theme tables to table #type » Convert language content setting theme table to table #type
Status: Needs work » Needs review
FileSize
5.12 KB

Conversion of just the content language settings page. To simplify the conversion, moving the other table to a new issue.

To simplify review, purposely not making any changes that would require a change to the tests.

Todo:
Remove duplicated "label" field.
Add table dragging and other jQuery enhancements.
Make changes that might require modifications to the test.

lokapujya’s picture

Issue summary: View changes
lokapujya’s picture

Issue summary: View changes
lokapujya’s picture

lokapujya’s picture

+++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
@@ -102,32 +102,52 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      $form['settings'][$entity_type_id . '-label'] = [
+        '#markup' => '<h4>' . $label . '</h4>',
+      ];

Created the label field like this, to get the heading.

+++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
@@ -102,32 +102,52 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+          '#markup' => $bundle_info['label'],

but, then it gets repeated here, in order to satisfy existing tests.

akalata’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.06 KB
Manuel Garcia’s picture

Minor change for arrays to short syntax.

Status: Needs review » Needs work

The last submitted patch, 27: convert_language-1938912-27.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
5.05 KB

Oops! - here's the proper patch... the interdiff on #27 is good.

Status: Needs review » Needs work

The last submitted patch, 29: convert_language-1938912-29.patch, failed testing.

Manuel Garcia queued 26: 1938912-26.patch for re-testing.

The last submitted patch, 26: 1938912-26.patch, failed testing.

lokapujya’s picture

Did anyone look into the 1 failing test at all?

Manuel Garcia’s picture

@lokapujya looks like the faling test is from a change in core since the patch on #26 was created.

lokapujya’s picture

+++ b/core/modules/language/language.admin.inc
@@ -142,65 +142,4 @@ function theme_language_negotiation_configure_browser_form_table($variables) {
-  return '<h4>' . $variables['build']['#title'] . '</h4>' . drupal_render($variables['build']);

Looks like the part after the <h4> is no longer getting included. Not sure why yet.

joelpittet’s picture

One fail left that's encouraging!

This won't help that but please in the next version consider this:

  1. +++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
    @@ -102,32 +102,52 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -      $form['settings'][$entity_type_id] = array(
    +      $form['settings'][$entity_type_id] = [
    ...
    -        '#states' => array(
    -          'visible' => array(
    +        '#states' => [
    +          'visible' => [
    ...
    -          ),
    -        ),
    -      );
    +          ],
    +        ],
    +      ];
    

    Same here. +1 for smaller patch!

  2. +++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
    @@ -102,32 +102,52 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +        $form['settings'][$entity_type_id][$bundle]['settings'] = [
    ...
    -          'language' => array(
    +          'language' => [
    ...
    -            '#entity_information' => array(
    +            '#entity_information' => [
    ...
    -            ),
    +            ],
    ...
    -          ),
    -        );
    +          ],
    +        ];
    

    As much as I love short syntax we should avoid making these changes outside the hunks because if this gets committed it could make another patch need an unnsesasary reroll or visa versa this patch may need a re-roll if this hunk changes in another patch.

lokapujya’s picture

Actually, it's not the part after the h4, it's part of the title (inside the h4) that is missing.

This is where the text that is missing gets added:
in content_translation.admin.inc, inside _content_translation_form_language_content_settings_form_alter()

<?php
$form['settings'][$entity_type_id]['#title'] = t('@label (Translation is not supported).', array('@label' => $entity_type->getLabel()));<
?>

It modifies the #title, so got to make that work with the table.

joelpittet’s picture

SafeMarkup::format() should help that out eh?

lokapujya’s picture

Issue summary: View changes
FileSize
115.16 KB

Yes, could be related to SafeMarkup.

Also, prior to this patch the tables are hidden until the checkbox is clicked. But with the patch, they are all visible initially. I'm just adding ideas that might help incase anyone else is looking at this. Otherwise, I might have time to dig into it in a few days.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
4.3 KB
2.01 KB

Taking care of #36 first.

Status: Needs review » Needs work

The last submitted patch, 40: convert_language-1938912-40.patch, failed testing.

The last submitted patch, 40: convert_language-1938912-40.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
933 bytes
5.21 KB

Still needs work, but this fixes the failing test.

lokapujya’s picture

Issue summary: View changes
+++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
@@ -118,9 +117,30 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      $form['settings'][$entity_type_id] = [
+        '#title' => $label,
+        '#type' => 'table',

These attributes are already set earlier in the function, need to remove the existing code.

lokapujya’s picture

Issue summary: View changes
lokapujya’s picture

Issue summary: View changes
Manuel Garcia’s picture

Here's a go at #44, moved the reminding stuff into the new table type form element.

UI is still broken although we are now pulling in the states. I realize the markup is different now as the h4 label on top is not within a wrapping div as before (which is where the states got attached before).

akalata’s picture

Issue summary: View changes
Issue tags: +Twig, +Twig conversion, +Needs manual testing
akalata’s picture

akalata’s picture

Posting a reroll.

akalata’s picture

Status: Needs review » Needs work

Won't we need that wrapper around the <h4> and table? Even if we could get the #states working, they're applied to the table -- which means that all of the <h4>s would show up, even if their trigger is not checked.

star-szr’s picture

Status: Needs work » Needs review
FileSize
5.73 KB
1.79 KB

This might get us a bit closer, the states seem to work now.

Status: Needs review » Needs work

The last submitted patch, 52: convert_language-1938912-52.patch, failed testing.

The last submitted patch, 52: convert_language-1938912-52.patch, failed testing.

The last submitted patch, 52: convert_language-1938912-52.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
1.85 KB
6.98 KB

Not sure about this, but it might help see the problem.

Status: Needs review » Needs work

The last submitted patch, 57: 1938912-57.patch, failed testing.

The last submitted patch, 57: 1938912-57.patch, failed testing.

joelpittet’s picture

I was playing around with the label at DrupalCon and it would be nice to just extend table.html.twig to add a title/label on top with an h4 wrapping it. That seemed the cleanest approach otherwise you are throwing around render array for some simple text to wrap in an h4 which gets messy fast.

#title property on the #type=>table would be nice. But then I ran into those silly suggestion things not working as I'd expect and having to do the stuff we do with blocks.

RainbowArray’s picture

Status: Needs review » Needs work

joelpittet’s picture

Issue tags: +rc target triage
joelpittet’s picture

Title: Convert language content setting theme table to table #type » Convert language content setting table theme to a twig template
Status: Needs work » Needs review
FileSize
3.2 KB

I think we may be doing this too complicated of a conversion. Here's the simplest fix-up to a template.

It can be refactored further I'm sure but this at least removes the theme function.

star-szr’s picture

Status: Needs review » Needs work

I really really like that idea, thanks @joelpittet! Just some docs/TX thoughts below:

  1. +++ b/core/modules/content_translation/content_translation.module
    @@ -534,7 +534,7 @@ function content_translation_form_language_content_settings_form_alter(array &$f
    - * Implements hook_preprocess_HOOK() for theme_language_content_settings_table().
    + * Implements hook_preprocess_HOOK() for language-content-settings-table.html.twig.
    
    +++ b/core/modules/language/language.admin.inc
    @@ -94,7 +94,7 @@ function template_preprocess_language_negotiation_configure_form(&$variables) {
    - * Implements hook_preprocess_HOOK() for theme_language_content_settings_table().
    + * Implements hook_preprocess_HOOK() for language-content-settings-table.html.twig.
    

    Per our standards these should be more like "Prepares variables for language content settings table templates." with the default template line per https://www.drupal.org/node/1354#themepreprocess.

  2. +++ b/core/modules/language/templates/language-content-settings-table.html.twig
    @@ -0,0 +1,16 @@
    + * @see template_preprocess_table()
    

    Should this point to template_preprocess_language_content_settings_table() instead?

  3. +++ b/core/modules/language/templates/language-content-settings-table.html.twig
    @@ -0,0 +1,16 @@
    +{{ build }}
    

    Maybe we can add another variable in preprocess called 'table' that is the same as 'build' (so we're not breaking BC) and print that instead? Build is a weird var name.

joelpittet’s picture

+++ b/core/modules/language/language.admin.inc
@@ -132,24 +132,10 @@ function template_preprocess_language_content_settings_table(&$variables) {
-  return '<h4>' . theme_render_and_autoescape($variables['build']['#title']) . '</h4>' . theme_render_and_autoescape($variables['build']);

+++ b/core/modules/language/templates/language-content-settings-table.html.twig
@@ -0,0 +1,16 @@
+{{ build }}

The variable was previously named build, but I prefer 'table' too.

star-szr’s picture

Yeah :) so we shouldn't change it because BC but that doesn't stop us from doing… $variables['table'] = $variables['build']; …in preprocess.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
3.56 KB

How does this play?

Status: Needs review » Needs work

The last submitted patch, 69: convert_language-1938912-69.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.21 KB
3.52 KB

There is some inappropriate stuff happening to 'build' in _content_translation_preprocess_language_content_settings_table().

I'd rather not pass around two big arrays just for the sake of a better named variable. If this wasn't RC I'd say we just change it in all the places.

star-szr’s picture

Right I didn't think of that. Well thanks for trying :)

lauriii’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Tested the markup manually and it looks good. Also @Cottsers nitpicks has been fixed and I can't find any new ones.

The last submitted patch, 69: convert_language-1938912-69.patch, failed testing.

joelpittet’s picture

No harm in trying, thanks for reviewing you two!

Manuel Garcia’s picture

I like it @joelpittet - much simpler and to the point this patch now. +1 on RTBC

star-szr’s picture

Priority: Normal » Major
xjm’s picture

Issue tags: -rc target triage +rc target

Discussed with all the committers and we agreed this is an RC target!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 046477e on 8.0.x
    Issue #1938912 by Manuel Garcia, joelpittet, lokapujya, duellj, Cottser...

Status: Fixed » Closed (fixed)

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