Problem/Motivation

From #2281691: User interface for migration-based upgrades. There are a few minor uses of markup in translatable strings that can be cleaned up:

  1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -0,0 +1,1227 @@
+        '#prefix' => $this->t('<p>An upgrade has already been performed on this site.</p>'),
+        '#description' => $this->t('<p>Last upgrade: @date</p>', ['@date' => $this->dateFormatter->format($date_performed)]),
...
+    return $this->t('<p><strong>Upgrade analysis report</strong></p>');

    I think usually we don't include <p> tags inside translatable strings. 

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -0,0 +1,1227 @@
+    catch (\Exception $e) {
+      $error_message = [
+        '#type' => 'inline_template',
+        '#template' => '{% trans %}Resolve the issue below to continue the upgrade.{% endtrans%}{{ errors }}',
+        '#context' => [
+          'errors' => [
+            '#theme' => 'item_list',
+            '#items' => [$e->getMessage()],
+          ],
+        ],
+      ];

    Using an item list and an inline template for a single exception message seems like overkill. I guess this came from the install form? 

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -0,0 +1,1227 @@
+        '#title' => '<ul><li>' . $this->t('@count available upgrade paths', ['@count' => $available_count]) . '</li><li>' . $this->t('@count missing upgrade paths', ['@count' => $missing_count]) . '</li></ul>',

    This seems like a misues of #title and of the HTML <ul> tag, and the concatenation does not seem best practice based on https://www.drupal.org/node/2296163. However, I can't actually remember how #title specifically behaves. Still, item lists should be item lists. I guess some depends on how exactly this is used in the UI. 

  4. +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
@@ -0,0 +1,369 @@
+        $context['message'] = $context['sandbox']['messages'][$index] . "<br />\n" . $context['message'];

    Concatenating the messages like this is a little weird and makes me wonder about the markup escaping strategy. 

  5. +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
@@ -0,0 +1,369 @@
+          $context['message'] = t('Currently rolling back @migration (@current of @max total tasks)', [
+            '@migration' => $migration_name,
+            '@current' => $context['sandbox']['current'],
+            '@max' => $context['sandbox']['max'],
+          ]) . "<br />\n" . $context['message'];

    Same "hmm" about markup escaping due to the concatenation.

Proposed resolution

Move block-level markup out of translatable strings where appropriate and use the markup and render APIs more correctly, with less direct concatenation. Ensure there are no escaping bugs.

Remaining tasks

Attached patch was removed from #2281691: User interface for migration-based upgrades because it wasn't tested yet.

User interface changes

Maybe some string fixes.

API changes

N/A

Data model changes

N.A<

CommentFileSizeAuthor
#15 Screen Shot 2016-11-17 at 11.26.10.png115.17 KBalexpott
#15 Screen Shot 2016-11-17 at 11.12.09.png160.36 KBalexpott
#15 2679913-15.patch2.95 KBalexpott
#15 8-15-interdiff.txt2.15 KBalexpott
#8 interdiff-5-8.txt785 byteskekkis
#8 2679913-8.patch2.79 KBkekkis
#5 2679913-5.patch3.26 KBquietone
strings_markup.patch1.36 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

From #2281691-142: User interface for migration-based upgrades:

Batch messages are treated as markup... see _batch_progress_page()...

    '#message' => array('#markup' => $message),

So this will be xss filtered so all of this concatenation is fine - batches are special. This is documented somewhere...

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
2679913-5.patch3.26 KB

Items 1, 2, and 3 are fixed in this patch. Items 4 and 5 are works as designed as per #2. Although #5 is about rollback which isn't currently in.

kekkis’s picture

kekkis
he/him
Primary language Finnish
Location Pirkkala
CreditAttribution: kekkis as a volunteer commented

Patch applies cleanly to 8.3.x but there is a small typo introduced by the patch. A follow-up issue is recommended to fix this.

-        // Get the system data from source database.
+        // Get the system data fqrom source database.
kekkis’s picture

kekkis
he/him
Primary language Finnish
Location Pirkkala
CreditAttribution: kekkis as a volunteer commented
Assigned: Unassigned » kekkis
Status: Needs review » Needs work

On second thought, why not fix the patch. One coming right up.

kekkis’s picture

kekkis
he/him
Primary language Finnish
Location Pirkkala
CreditAttribution: kekkis as a volunteer commented
Assigned: kekkis » Unassigned
Status: Needs work » Needs review
FileSize
2679913-8.patch2.79 KB
interdiff-5-8.txt785 bytes

And here we go with the new patch. The typo that was introduced got removed, so the patch got smaller too.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Issue tags: +Novice

Marking as novice to do the review of the fairly actionable requirements. Please review all 5 points from the IS and make sure they are addressed.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Assigned: Unassigned » phenaproxima

Assigning to myself for review.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Assigned: phenaproxima » heddn

Seeing as no one has picked this up, I'll take a look at this shortly.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Needs work

~ line 758: $info[] = $this->t('<strong>Back up the database for this site</strong>. Upgrade will change the database for this site.');.

Does anyone know, are strong tags also not supposed to be inside the translation? Me thinks they are not. For this, marking NW. Otherwise, I went through the entire class MigrateUpgradeForm and checked all translatable strings and now all other markup is fixed.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Assigned: heddn » Unassigned
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Reviewed & tested by the community

The handbook page,
Dynamic or static links and HTML in translatable strings, has an example using <strong>.

When it comes to inline markup however, things are different. It would not be wise to split up a sentence just because you try to emphasize a part:

$DO_NOT_DO_THIS .= t('This information is ') .'<strong>'. t(' very important') .'</strong>.';

This achieves HTML and text separation but makes life hard for you and translators alike. In these simple cases, just include the inline markup, and let the translators live with the fact. It provides them more context instead of the two unique strings you'd have with the above example. Do this instead:

$output .= t('This information is <strong>very important</strong>.');

According to that this is good to go. Setting to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Chapter Three commented
Status: Reviewed & tested by the community » Needs review
FileSize
8-15-interdiff.txt2.15 KB
2679913-15.patch2.95 KB
Screen Shot 2016-11-17 at 11.12.09.png160.36 KB
Screen Shot 2016-11-17 at 11.26.10.png115.17 KB 
  1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -736,7 +736,7 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) {
-        '#description' => $this->t('<p>Last upgrade: @date</p>', ['@date' => $this->dateFormatter->format($date_performed)]),
+        '#description' => '<p>' . $this->t('Last upgrade: @date', ['@date' => $this->dateFormatter->format($date_performed)]) . '</p>',

    I'm not sure that the p tags are even required here. Testing by setting \Drupal::state()->set('migrate_drupal_ui.performed', 1); shows me this. 

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -1075,8 +1069,11 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
+        $this->t('@count available upgrade paths', ['@count' => $available_count]),
+        $this->t('@count missing upgrade paths', ['@count' => $missing_count]),

    Whilst we're fixing this we should fix it to use formatPlural() 

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -1145,7 +1142,7 @@ public function getCancelUrl() {
-    return $this->t('<p><strong>Upgrade analysis report</strong></p>');
+    return '<p><strong>' . $this->t('Upgrade analysis report') . '</strong></p>';

    This is a really poor description. It's more of a title. I think this should be text describing what is below. See screenshot attached. However a simple fix would be to return nothing - just do return; and add '#title' => 'Upgrade analysis report', to the counts in the \Drupal\migrate_drupal_ui\Form\MigrateUpgradeForm::buildConfirmForm() method.

    Before patch

    After patch

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented
Status: Needs review » Needs work
Ada Hernandez’s picture

Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commented
Assigned: Unassigned » Ada Hernandez
Ada Hernandez’s picture

Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commented
Assigned: Ada Hernandez » Unassigned
Ada Hernandez’s picture

Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commented
Status: Needs work » Reviewed & tested by the community

I have checked this with the last patch and I think that is ok

  • catch committed 45b87d8 on 8.3.x 
    Issue #2679913 by kekkis, xjm, quietone: Fix use of markup in strings in...

  • catch committed 3b0d1de on 8.2.x 
    Issue #2679913 by kekkis, xjm, quietone: Fix use of markup in strings in...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.3.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Fixed

I messed up alexpott's commit attribution due to the core committer attribution curse.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Status: Fixed » Closed (fixed)

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