Follow-up to #1938912: Convert language content setting table theme to a twig template

Task

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

Module Theme function name Where in Code What is it really? Fixed in
language theme_language_negotiation_configure_browser_form_table function form table

Steps to reproduce/review

  1. Install Drupal 8, and enable the language module.
  2. Visit admin/config/regional/language/detection/browser.
  3. Confirm that the Browser Language Detection Configuration table displays correctly in HEAD and with the patch applied.
  4. Delete all of the default browser language items so that the table contains no items.
  5. Confirm that the empty message text "No browser language mappings available." is correctly displayed in HEAD and with the patch applied.

Next steps

Before

Empty:

Two Languages:

After

Empty:

Two Languages:

Related issues

CommentFileSizeAuthor
#24 interdiff-2422481-12-24.txt597 bytesakalata
#24 convert_language_negotiation_table-2422481-24.patch4.49 KBakalata
#17 interdiff-2422481-12-16.txt597 bytesakalata
#17 convert_language_negotiation_table-2422481-16.patch4.49 KBakalata
#15 interdiff-2422481-12-15.txt76.56 KBakalata
#15 convert_language_negotiation_table-2422481-15.patch89.57 KBakalata
#12 convert_language_negotiation_table-2422481-12.patch4.52 KBakalata
#12 interdiff-2422481-7-12.txt755 bytesakalata
#7 interdiff-2422481-4-7.txt1.92 KBakalata
#7 convert_language_negotiation_table-2422481-7.patch4.51 KBakalata
#4 2422481-4-reroll.patch4.96 KBakalata
#2 2422481-2.patch5 KBlokapujya
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Related issues: +#1938912: Convert language content setting table theme to a twig template
lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Status: Active » Needs review
FileSize
2422481-2.patch5 KB

Starter patch.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented 
+++ b/core/modules/language/language.admin.inc
@@ -96,55 +95,6 @@ function template_preprocess_language_negotiation_configure_form(&$variables) {
-  $table = array(
-    '#type' => 'table',
-    '#header' => $header,
-    '#rows' => $rows,
-    '#empty' => t('No browser language mappings available.'),
-    '#attributes' => array('id' => 'language-negotiation-browser'),
-  );

Todo: Add #empty and #attributes back in.

akalata’s picture

akalata CreditAttribution: akalata commented
Issue summary: View changes
Issue tags: +Needs manual testing, +Barcelona2015
FileSize
2422481-4-reroll.patch4.96 KB

Updating issue summary with steps to reproduce and issue next steps, along with posting a reroll.

akalata’s picture

akalata CreditAttribution: akalata commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three for Drupal Association commented 
+++ b/core/modules/language/src/Form/NegotiationBrowserForm.php
@@ -87,31 +88,44 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      $form['mappings'][$browser_langcode] = array(
-        'browser_langcode' => array(
+      $form['mappings'][$browser_langcode] = [
+        'browser_langcode' => [
           '#title' => $this->t('Browser language code'),
           '#title_display' => 'invisible',
           '#type' => 'textfield',
           '#default_value' => $browser_langcode,
           '#size' => 20,
           '#required' => TRUE,
-        ),
-        'drupal_langcode' => array(
+        ],
+        'drupal_langcode' => [
           '#title' => $this->t('Site language'),
           '#title_display' => 'invisible',
           '#type' => 'select',
           '#options' => $language_options,
           '#default_value' => $drupal_langcode,
           '#required' => TRUE,
-        ),
-      );
+        ],
+      ];

These changes are out of scope, IMHO. Its just converting to short array syntax

akalata’s picture

akalata CreditAttribution: akalata commented
Issue summary: View changes
Issue tags: +Novice
FileSize
convert_language_negotiation_table-2422481-7.patch4.51 KB
interdiff-2422481-4-7.txt1.92 KB

Thanks @dawehner! The attached patch addresses feedback in #3 and #6.

Status: Needs review » Needs work

The last submitted patch, 7: convert_language_negotiation_table-2422481-7.patch, failed testing.

The last submitted patch, 7: convert_language_negotiation_table-2422481-7.patch, failed testing.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya at Babson College commented 
+++ b/core/modules/language/src/Form/NegotiationBrowserForm.php
@@ -94,29 +95,31 @@
+      '#attributes' => new Attribute(array('id' => 'language-negotiation-browser')),
+      '#empty' => t('No browser language mappings available.'),

Probably need to add this in a different way.

akalata’s picture

akalata CreditAttribution: akalata commented

@lokapujya there are notices for the #attributes, which I will work on now, but the #empty seems to be working fine?

akalata’s picture

akalata CreditAttribution: akalata commented
Status: Needs work » Needs review
FileSize
interdiff-2422481-7-12.txt755 bytes
convert_language_negotiation_table-2422481-12.patch4.52 KB

According to this change record, the Attributes object must be initialized with an empty class array.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue tags: -Novice 
+++ b/core/modules/language/src/Form/NegotiationBrowserForm.php
@@ -87,10 +89,16 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      '#attributes' => new Attribute(['id' => 'language-negotiation-browser', 'class' => []]),

Only reason to use this is if we are adding classes later on. But a better approach would be to use addClass() and it will ensure the AttributeArray class gets created.

But recommendation that this is not needed because I can't see where a class is being set on that table.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Needs work

I guess the answer here is we don't need to instantiate an Attribute object with #attributes, we can just pass it an array.

akalata’s picture

akalata CreditAttribution: akalata commented
Status: Needs work » Needs review
FileSize
convert_language_negotiation_table-2422481-15.patch89.57 KB
interdiff-2422481-12-15.txt76.56 KB
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs manual testing
FileSize
after-patch-two-langauges.png173.77 KB
empty-with-patch.png147.51 KB
2-without-patch.png183.75 KB
empty-without-patch.png146 KB
akalata’s picture

akalata CreditAttribution: akalata commented
Status: Needs work » Needs review
FileSize
convert_language_negotiation_table-2422481-15.patch89.57 KB
interdiff-2422481-12-15.txt76.56 KB
convert_language_negotiation_table-2422481-16.patch4.49 KB
interdiff-2422481-12-16.txt597 bytes

Let's try this one instead.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

RTBC for convert_language_negotiation_table-2422481-16.patch

As it was a minor change that already proved to pass and I've manually tested with screenshots in #16

The last submitted patch, 15: convert_language_negotiation_table-2422481-15.patch, failed testing.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya at Babson College commented

What just happened between #12 and #15? NM, I see that #16 ignores it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: convert_language_negotiation_table-2422481-16.patch, failed testing.

Cottser queued 17: convert_language_negotiation_table-2422481-16.patch for re-testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs work » Needs review
akalata’s picture

akalata CreditAttribution: akalata commented
FileSize
convert_language_negotiation_table-2422481-24.patch4.49 KB
interdiff-2422481-12-24.txt597 bytes

Renaming/reposting correct patch from #17, since I keep getting confused.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Gave this another spin and I can confirm everything works as expected and the patch looks good. Thanks @akalata!

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya at Babson College commented 
+++ b/core/modules/language/src/Form/NegotiationBrowserForm.php
@@ -10,6 +10,8 @@
+use Drupal\Core\Template\Attribute;

Do we need this anymore?

RTBC++ after that.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Wow, the diffstat here is insanely better! Great work.

PHPStorm agrees with #26, so removed that line and...

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 8bd7a50 on 8.0.x 
    Issue #2422481 by akalata, lokapujya, joelpittet: Convert language...

Status: Fixed » Closed (fixed)

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