Simplify ReviewForm::buildForm() and the resulting markup

The patch fails to apply because the first line of context does not match the current comment:

        // Get the migration status for this $source_module, if a module of the

We wanted to update that comment anyway. Maybe we can do it as part of this issue.

The patch in #7 looks good, but I would like one small change:

+++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
@@ -154,19 +154,13 @@ public function buildForm(array $form, FormStateInterface $form_state) {
...
           'source_module' => [
@@ -201,16 +195,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-          'source_module' => [
...
+          'source_module_name' => [

In the first render array, the patch leaves the key as source_module, but in the second array the key is changed to source_module_name. I would like to keep these consistent.

@quietone, thanks for the update!

The patch in #9 looks good to me: RTBC.

The patch saves a few lines of code, which I like, but that is not the reason for this issue. Each row of the table consists of three render arrays. Before this patch, two of them are '#type' => 'markup' (the default, thus omitted) with a single '#plain_text' key. The third is different: '#type' => 'html_tag' with three other keys. It takes thought to figure out that the output will be pretty similar to the other two entries, but with some CSS classes attached. I do not like to think that hard. ;)

After the patch, it is much easier to see that the three render arrays are similar, except that one of them has some extra CSS classes.

OK, I do like to think that hard, but I want to save the effort for when it matters.

+++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
@@ -154,19 +154,13 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-            '#type' => 'html_tag',
-            '#tag' => 'span',
-            '#value' => $data['source_module_name'],
-            '#attributes' => [
-              'class' => [
-                'upgrade-analysis-report__status-icon',
-                'upgrade-analysis-report__status-icon--error',
-              ],
-            ],
+            '#prefix' => '<span class="upgrade-analysis-report__status-icon upgrade-analysis-report__status-icon--error">',
+            '#suffix' => '</span>',
+            '#plain_text' => $data['source_module_name'],
           ],

@@ -202,15 +196,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-            '#type' => 'html_tag',
-            '#tag' => 'span',
-            '#value' => $data['source_module_name'],
-            '#attributes' => [
-              'class' => [
-                'upgrade-analysis-report__status-icon',
-                'upgrade-analysis-report__status-icon--checked',
-              ],
-            ],
+            '#prefix' => '<span class="upgrade-analysis-report__status-icon upgrade-analysis-report__status-icon--checked">',
+            '#suffix' => '</span>',
+            '#plain_text' => $data['source_module_name'],
           ],

Potentially a better change then this would be to do something like

          'source_module_name' => [
            'data' => ['#plain_text' => $data['source_module_name']],
            '#wrapper_attributes' => ['class' => ['upgrade-analysis-report__status-icon', 'upgrade-analysis-report__status-icon--checked']],
          ],

This way we have less html in this form and we're not dependent on arbitrary spans. All the cells are plain td elements with no html inside them. And it's easier to alter because the attributes are not part of an html string.

Not that making this change will require test changes since we do:

      $session->elementExists('xpath', "//span[contains(@class, 'checked') and text() = '$available']");
      $session->elementNotExists('xpath', "//span[contains(@class, 'error') and text() = '$available']");

But this can be changed to

      $session->elementExists('xpath', "//td[contains(@class, 'checked') and text() = '$available']");
      $session->elementNotExists('xpath', "//td[contains(@class, 'error') and text() = '$available']");

Where are these options documented?

According to https://api.drupal.org/api/drupal/elements/9.0.x,

Usage and properties are documented on the individual classes

so I expect to see data and wrapper_attributes in the API docs for the Table element. Instead, the only place I find them is in the API docs for the Twig template table.html.twig.

Do we need a documentation issue to add something to Table.php? (I have not yet searched for an existing issue.)

Another mystery: what class handles '#type' => 'markup'?

Update: on second look, I see that there is a link from the Table api docs to the table.html.twig API docs.

@quietone:

I did not see your question until I wrote the comment below (including the "undocumented feature" part) and had to reload the page. I only found #wrapper_attributes by searching the codebase. It is in the usage example of class Table. Looking again, I do not see it in the Twig template, despite what I said in #12.

I like the idea in #11 (implemented in #13) of adding CSS classes to the existing HTML elements instead of creating a new <span>. But this is a step backwards in terms of making the PHP easy to read. Also, I am beginning to think that the use of #wrapper_attributes is an undocumented feature.

I would rather move the CSS classes to the <tr> element, or maybe even the <table> element. Is there a semantic reason for attaching them to the <td>?

Moving the CSS classes to the row would look something like this:

        $missing_module_list['module_list'][] = [
          '#attributes' => ['class' => [
            'upgrade-analysis-report__status-icon',
            'upgrade-analysis-report__status-icon--error',
          ]],
          'source_module_name' => [
            '#plain_text' => $data['source_module_name'],
          ],
          'source_machine_name' => [
            '#plain_text' => $data['source_machine_name'],
          ],
          'destination_module' => [
            '#plain_text' => $data['destination'],
          ],
        ];

Going back to a multi-line array for the CSS classes makes the BEM structure clear, and moving the classes away from the <td> elements makes that structure easier to understand.

It is not hard to adjust the CSS selectors:

.upgrade-analysis-report__status-icon > :first-child:before

or

.upgrade-analysis-report__status-icon td:first-child:before

I think the second version would also work if we moved the CSS classes to the <table> element.

This will require updating the corresponding CSS in the stable and stable9 themes and in the tests.

The next issue on my list is #3088215: Use styling from Upgrade Status module on Review form. Maybe we should abandon this issue and make the improvements as part of that one.

P.S. Or else postpone this issue on that one.

Spent sometime reading the table theme docs on template_preprocess_table().

It turns out we can do even simpler if we use #rows ... less stuff.

I think it would be even simpler to move the CSS classes to the row, as I suggested in #15. But maybe we can consider that as part of #3088215: Use styling from Upgrade Status module on Review form. (I have decided that that issue still needs some work, not just tweaks, so I am less reluctant to require a reroll there.)

There is a trade-off: putting the CSS classes in one long line makes it easier to scan the code but harder to see the BEM structure of the classes.

I think the patch in #18 is an improvement over the current code. Let's get it in in the spirit of incremental improvement. RTBC.

I am updating the issue summary, using the standard template.

Nice clean-up.

Committed 940e609 and pushed to 9.1.x. Thanks!