Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because clean ups code by using new APIs
Issue priority Normal because replaces code that works with code that is the current best practice
Disruption No disruption

Problem/Motivation

In TranslateEditForm::buildForm() and PluralString::getTranslationElement(), the use of '#markup' and string concatenation could be replaced with inline_templates.

Proposed resolution

Replace string concatenation with inline_templates where necessary in TranslateEditForm::buildForm() and PluralString::getTranslationElement()

Remaining tasks

Implement. Review.

User interface changes

None.

API changes

None.

Comments

maxocub’s picture

Gábor Hojtsy’s picture

Issue tags: +language-ui, +language-config
penyaskito’s picture

Issue tags: +SafeMarkup
maxocub’s picture

Status: Postponed » Active
andypost’s picture

Gábor Hojtsy’s picture

Issue tags: -sprint

Looks like this is not being worked on, so removing from sprint. Please add it back if so.

maxocub’s picture

Issue tags: +sprint
maxocub’s picture

Assigned: Unassigned » maxocub
maxocub’s picture

Component: config_translation.module » markup
Status: Active » Needs review
FileSize
7.76 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,126 pass(es). View

Here's a first patch. I never used #inline_template before and it's not documented on the Form API page (should I open an issue about that?), so I looked at other usage in core.

andypost’s picture

  1. +++ b/core/modules/config_translation/src/FormElement/PluralVariants.php
    @@ -23,23 +23,28 @@ class PluralVariants extends FormElementBase {
    +      '#title' => $environment->renderInline($title_template, $title_context),
    ...
    +        '#markup' => $environment->renderInline($template, $context),
    
    @@ -51,12 +56,15 @@ protected function getSourceElement(LanguageInterface $source_language, $source_
    +      '#title' => $environment->renderInline($title_template, $title_context),
    
    +++ b/core/modules/locale/src/Form/TranslateEditForm.php
    @@ -72,9 +76,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +            '#markup' => $environment->renderInline($value_template, array('value' => $source_array[0])),
    
    @@ -83,21 +85,21 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +            '#markup' => $environment->renderInline($value_template, array('value' => $source_array[0])),
    ...
    +            '#markup' => $environment->renderInline($value_template, array('value' => $source_array[1])),
    
    @@ -132,7 +138,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +              '#prefix' => $i == 0 ? $environment->renderInline($prefix_template, $prefix_context) : '',
    

    I think in most places you can leave that as render array (no need to render)

  2. +++ b/core/modules/config_translation/src/FormElement/PluralVariants.php
    @@ -23,23 +23,28 @@ class PluralVariants extends FormElementBase {
         for ($i = 0; $i < $plurals; $i++) {
    +      $template = '<span lang="{{ langcode }}">{{ value }}</span>';
    
    +++ b/core/modules/locale/src/Form/TranslateEditForm.php
    @@ -125,6 +127,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
               for ($i = 0; $i < $plurals; $i++) {
    +            $prefix_template = '<span class="visually-hidden">{{ translation_language }}</span>';
    

    makes sense to move template definition out of loop

maxocub’s picture

Status: Needs review » Needs work
maxocub’s picture

@andypost: Do you mean something like this:

'#markup' => array(
  '#type' => 'inline_template',
  '#template' => '{{ value }}',
  '#context' => array(
    'value' => $source_array[0],
  ),
),

I tried that and it just prints 'array' on the page. I guess I could just use the 'render' function instead of calling the 'renderInline' from twig, would that makes it better?
Or maybe I am not using the render array correctly?

andypost’s picture

@maxocub please try to change render array to prevent rendering, it looks really strange that we need render in build

maxocub’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,884 pass(es), 438 fail(s), and 22 exception(s). View
9.69 KB

New patch where I replace the 'item' form elements by 'inline_template'. We do lose some auto generated CSS classes and IDs, but AFAIK it doesn't matter, since it's just markup to show the source string to be translated.

Status: Needs review » Needs work

The last submitted patch, 14: use_inline_templates-2499651-14.patch, failed testing.

The last submitted patch, 14: use_inline_templates-2499651-14.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review
FileSize
8.51 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,452 pass(es), 7 fail(s), and 0 exception(s). View
788 bytes

This test was failing on #2571655: ConfigNamesMapper::hasTranslatable has flawed logic too, and I corrected it there too, not sure what's the best approach for this situation.

Status: Needs review » Needs work

The last submitted patch, 17: use_inline_templates-2499651-17.patch, failed testing.

maxocub’s picture

Oh, silly me, the test is looking for CSS IDs that no longer exists.

The last submitted patch, 17: use_inline_templates-2499651-17.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review
FileSize
8.89 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,514 pass(es). View
2.5 KB

I really don't know if it's OK to set an ID this way, but I added it on the inline template to be used with the test.

penyaskito’s picture

It's totally fine, nice cleanup :)

penyaskito’s picture

Issue summary: View changes

Added beta evaluation template.

penyaskito’s picture

Tested that works correctly.

penyaskito’s picture

+++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
@@ -665,10 +665,10 @@ public function testPluralConfigStringsSourceElements() {
-          $this->assertRaw('edit-source-config-names-viewsviewfiles-display-default-display-options-fields-count-format-plural-string-' . $index);
+          $this->assertRaw('edit-source-config-format-plural-string-' . $index);

Why is this needed? If we are changing the HTML element ids this way, we may have duplicated HTML ids which makes our HTML invalid.

maxocub’s picture

Status: Needs review » Needs work

Right, I forgot there may be more than one plural variant field on this long form, so there would be duplicated IDs. I'll find another way to test it.

maxocub’s picture

Status: Needs work » Needs review
FileSize
9.57 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,503 pass(es). View
2.83 KB

I removed the potentially duplicated IDs and modified the test to count the number of source elements with xpath.

maxocub’s picture

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/config_translation/src/FormElement/PluralVariants.php
    @@ -25,21 +25,26 @@ protected function getSourceElement(LanguageInterface $source_language, $source_
         $element = array(
           '#type' => 'fieldset',
    -      '#title' => SafeMarkup::format('@label <span class="visually-hidden">(@source_language)</span>', array(
    -        '@label' => $this->t($this->definition->getLabel()),
    -        '@source_language' => $source_language->getName(),
    -      )),
    +      '#title' => $this->t($this->definition->getLabel()),
           '#tree' => TRUE,
         );
    +    $element['source_language'] = array(
    +      '#type' => 'inline_template',
    +      '#template' => '<span class="visually-hidden">{{ source_language }}</span>',
    +      '#context' => array(
    +        'source_language' => $this->t('Source string (@language)', array('@language' => $source_language->getName())),
    +      ),
    +    );
    
    @@ -53,12 +58,16 @@ protected function getTranslationElement(LanguageInterface $translation_language
         $element = array(
           '#type' => 'fieldset',
    -      '#title' => SafeMarkup::format('@label <span class="visually-hidden">(@translation_language)</span>', array(
    -        '@label' => $this->t($this->definition->getLabel()),
    -        '@translation_language' => $translation_language->getName(),
    -      )),
    +      '#title' => $this->t($this->definition->getLabel()),
           '#tree' => TRUE,
         );
    +    $element['translation_language'] = array(
    +      '#type' => 'inline_template',
    +      '#template' => '<span class="visually-hidden">{{ translation_language }}</span>',
    +      '#context' => array(
    +        'translation_language' => $this->t('Translated string (@language)', array('@language' => $translation_language->getName())),
    +      ),
    +    );
    

    The modified code seems to be doing a different thing now? The fieldset title is the definition label and then it has a child element instead of the title contain the text?

  2. +++ b/core/modules/locale/src/Form/TranslateEditForm.php
    @@ -110,13 +102,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +        $form['strings'][$string->lid]['translations'] = array(
    +          '#type' => 'inline_template',
    +          '#template' => '<span class="visually-hidden">{{ translation_language }}</span>',
    +          '#context' => array(
    +            'translation_language' => $this->t('Translated string (@language)', array('@language' => $langname)),
    +          ),
    +        );
    +
             // Approximate the number of rows to use in the default textarea.
             $rows = min(ceil(str_word_count($source_array[0]) / 12), 10);
             if (!$plural) {
               $form['strings'][$string->lid]['translations'][0] = array(
                 '#type' => 'textarea',
    -            '#title' => $this->t('Translated string (@language)', array('@language' => $langname)),
    -            '#title_display' => 'invisible',
                 '#rows' => $rows,
                 '#default_value' => $translation_array[0],
                 '#attributes' => array('lang' => $langcode),
    @@ -132,7 +131,6 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    
    @@ -132,7 +131,6 @@ public function buildForm(array $form, FormStateInterface $form_state) {
                   '#rows' => $rows,
                   '#default_value' => isset($translation_array[$i]) ? $translation_array[$i] : '',
                   '#attributes' => array('lang' => $langcode),
    -              '#prefix' => $i == 0 ? ('<span class="visually-hidden">' . $this->t('Translated string (@language)', array('@language' => $langname)) . '</span>') : '',
                 );
               }
    

    Same here, the template seems to be in a different structure?!

maxocub’s picture

Status: Needs work » Needs review
FileSize
11.14 KB
10.95 KB
8.95 KB
8.86 KB
5.54 KB

Some before/after screenshots showing the visual haven't changed.
Also, a diff of the HTML structure changes.

PlurialVariants.php
PluralVariants_before.png
PluralVariants_after.png
What changed here is that <span class="visually-hidden>(English)</span> moved from the fieldset-lengend span to the fieldset-wrapper div. Also, some CSS ids ans classes are gone, but I don't see any side effects.

TranslateEditForm.php
TranslateEditForm_before.png
TranslateEditForm_after.png
What changed here is that the <span lang="en"> is not anymore around the whole div, but around each source strings, which is more like normal singular strings are presented in this form. There's also some CSS ids ans classes that are gone.

Status: Needs review » Needs work

The last submitted patch, 30: html.diff, failed testing.

maxocub’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

All righto, I misread something then, let's get this in :) Thanks a lot!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/PluralVariants.html
@@ -2,15 +2,15 @@
+      <label>Singular form</label>
...
+      <label>First plural form</label>

+++ b/TranslateEditForm.html
@@ -1,11 +1,11 @@
+      <label>Singular form</label>
...
+      <label>Plural form</label>

Aren't these missing the for attribute - I think that for was wrong before though - so not sure that this matters.

maxocub’s picture

The label tags without a for attribute are on the source side. So they are not associated with an input field, but with a span. The label tags on the translation side have a for attribute with the id of their input field.

Before this patch, the two label tags (the source one and the translation one) had both the same for attribute pointing to the translation's input field.

Maybe we should use a tag other than label for those without an input field?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

maxocub’s picture

Is there still work to be done here or can it be RTBC?

maxocub’s picture

Assigned: maxocub » Unassigned

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.