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:
-
+++ 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. -
+++ 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?
-
+++ 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. -
+++ 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.
-
+++ 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<
Comment | File | Size | Author |
---|---|---|---|
#15 | Screen Shot 2016-11-17 at 11.26.10.png | 115.17 KB | alexpott |
#15 | Screen Shot 2016-11-17 at 11.12.09.png | 160.36 KB | alexpott |
#15 | 2679913-15.patch | 2.95 KB | alexpott |
#15 | 8-15-interdiff.txt | 2.15 KB | alexpott |
#8 | interdiff-5-8.txt | 785 bytes | kekkis |
Comments
Comment #2
xjmFrom #2281691-142: User interface for migration-based upgrades:
Comment #5
quietone CreditAttribution: quietone as a volunteer commentedItems 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.
Comment #6
kekkisPatch 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.
Comment #7
kekkisOn second thought, why not fix the patch. One coming right up.
Comment #8
kekkisAnd here we go with the new patch. The typo that was introduced got removed, so the patch got smaller too.
Comment #9
heddnMarking 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.
Comment #10
phenaproximaAssigning to myself for review.
Comment #11
heddnSeeing as no one has picked this up, I'll take a look at this shortly.
Comment #12
heddn~ 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.
Comment #13
heddnComment #14
quietone CreditAttribution: quietone as a volunteer commentedThe handbook page,
Dynamic or static links and HTML in translatable strings, has an example using
<strong>
.According to that this is good to go. Setting to RTBC.
Comment #15
alexpottI'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.Whilst we're fixing this we should fix it to use formatPlural()
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
Comment #16
mikeryanComment #17
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedComment #18
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedComment #19
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedI have checked this with the last patch and I think that is ok
Comment #22
catchI 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!