Add ability to view migrate_message table data

We can un-postpone this issue now that 🐛 SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled Fixed has been fixed.

I tested locally, and the patch in #136 applies to 10.1.x. I will queue a re-test.

The patch in #120 was thoroughly reviewed (by me) and tested (by @bsnodgrass (he/him)) and RTBC in #123.

@alexpott raised some objections in #128, but @quietone and I replied to them in #129 and #130.

In #130, I raised the issue of the WSOD when the database connection is not available. That was addressed at first in #131 and then in 🐛 SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled Fixed .

My patch in #136 is just a re-roll of the one from #120.

Given all that, I think it is OK for me to return the status to RTBC. If the testbot finds a problem, then it will overrule me.

1. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -99,13 +101,21 @@ public function fields() {
   +    // The database connection may not exist, for example, when building
   +    // the Migrate Message form.
   +    try {
   +      $schema = $this->getDatabase()->schema();
   +      if ($schema->fieldExists('menu_links', 'language')) {
   +        $fields['language'] = $this->t("Menu link language code.");
   +      }
   +      if ($schema->fieldExists('menu_links', 'i18n_tsid')) {
   +        $fields['i18n_tsid'] = $this->t("Translation set id.");
   +      }
        }
   -    if ($schema->fieldExists('menu_links', 'i18n_tsid')) {
   -      $fields['i18n_tsid'] = $this->t("Translation set id.");
   +    catch (DatabaseNotFoundException | RequirementsException | \PDOException $e) {
   

   This construction looks really odd. Eating the error here feels wrong. Why can't we eat the error on the MigrateMessage form somehow.... OH see next point.

2. +++ b/core/modules/migrate/src/Controller/MigrateMessageController.php
   @@ -0,0 +1,325 @@
   +    // Create the column header names from the source plugin fields() method.
   +    // Fallback to the source_id name when the source ID is missing from
   +    // fields() method.
   +    try {
   +      $fields = $source_plugin->fields();
   +    }
   +    catch (DatabaseNotFoundException | RequirementsException | \PDOException $e) {
   +    }
   

   So we've already got a fallback in place. This makes me even less sure that changes to the source plugins are correct. If we do want to do this then we should implement a flag that determines if we should throw exceptions or not. And default to throwing exceptions.

   So the fields code should look something like:

     /**
      * {@inheritdoc}
      */
     public function fields(bool $throw_exception = TRUE) {
       $fields = [
         'menu_name' => $this->t('The menu name. Primary key.'),
         'title' => $this->t('The human-readable name of the menu.'),
         'description' => $this->t('A description of the menu'),
       ];
   
       // The database connection may not exist, for example, when building
       // the Migrate Message form.
       try {
         if ($this->getDatabase()
           ->schema()
           ->fieldExists('menu_custom', 'language')) {
           $fields += [
             'language' => $this->t('Menu language.'),
             'i8n_mode' => $this->t('Menu i18n mode.'),
           ];
         }
       }
       catch (DatabaseNotFoundException | RequirementsException | \PDOException $e) {
         if ($throw_exception) {
           throw $e;
         }
       }
   
       return $fields;
     }
   

   We can change the interface to:

     /**
      * Returns available fields on the source.
      *
      * @return array
      *   Available fields in the source, keys are the field machine names as used
      *   in field mappings, values are descriptions.
      */
     public function fields(bool $throw_exception = TRUE);
   

   We'll have to update the core implementations because of PHPStan but this API change is allowed since we're adding an argument with a default.

3. +++ b/core/modules/migrate/src/Controller/MigrateMessageController.php
   @@ -0,0 +1,325 @@
   +    foreach ($source_id_field_names as $source_id_field_name) {
   +      $pattern = [
   +        '/^[Tt]he /',
   +        '/\.$/',
   +      ];
   +      $display_name[$source_id_field_name] = preg_replace(
   +        $pattern, '', $fields[$source_id_field_name] ?? $source_id_field_name);
   +    }
   +
   +    $count = 1;
   +    foreach ($source_id_field_names as $source_id_field_name) {
   +      $header[] = [
   +        'data' => ucfirst($display_name[$source_id_field_name]),
   +        'field' => 'sourceid' . $count++,
   +        'class' => [RESPONSIVE_PRIORITY_MEDIUM],
   +      ];
   +    }
   

   We're looping around the same array here twice for no reason. Add we're building a display_name array for no reason. This could be written like:

       $count = 1;
       foreach ($source_id_field_names as $source_id_field_name) {
         $display_name = preg_replace([
           '/^[Tt]he /',
           '/\.$/',
         ], '', $fields[$source_id_field_name] ?? $source_id_field_name);
         $header[] = [
           'data' => ucfirst($display_name[$source_id_field_name]),
           'field' => 'sourceid' . $count++,
           'class' => [RESPONSIVE_PRIORITY_MEDIUM],
         ];
       }
   

   There's also no need $pattern variable - it's not variable.

#139.3. The only thing fixed in this patch.

Setting back to postponed on 🐛 SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled Fixed

We can un-postpone this issue now that 🐛 SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled Fixed is fixed.

#141 cc failure.

Please review.

Reroll LGTM.

I did another review and found a few things that should be fixed: see the comments on the MR. I would call it a Novice task to apply those updates, but I am afraid that a novice might accept the suggestions without considering whether I am right or not.

I confirmed that the MR (based on the patch in #141) addresses the "minor point" made in #130 and also #139.3. (The "major point" from #130 was addressed in 🐛 SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled Fixed .)

That leaves @alexpott's suggestion in #139.1 and #139.2, which we have not (yet) implemented. @quietone and I discussed this in Comments #54 and #60. Or maybe the more relevant discussion is #86.1 and #89.1. And we seem to have repeated that discussion in #128, #129, and #130.

My opinion is that we should leave the try/catch blocks as is.

Does the fact "view migrate messages" was passing also mean it needs additional tests?

@quietone:

Thanks for updating the MR! Are you still working on this, or is it ready for review?

I think this issue deserves a change record. I am adding the tag for that.

I see one more commit since my last comment. I will pretend that the status is now NR.

I reviewed the changes, and all the suggestions I made on the MR have been implemented. Thanks!

Since it has been a while, I repeated the tests from #117. It still works as expected. (The error messages are now warning messages, since #2953111: Only migrate role permissions that exist on the destination → has now been fixed.

Back to RTBC!

I made some updates to the issue summary, including a summary of the discussion on whether to catch exceptions twice (in the source plugin and in the page controller). There is one remaining item on the list of remaining tasks, but I think we agreed it could be a follow-up issue.

I added a stub change record, and I will work on that now. I will be optimistic and remove the issue tag now.

Thanks for the RTBC!

However, I had a few more changes to make. I should have left a comment but I was running late for an appointment. Here are the things I have changed and questions.

1. While testing I was getting yet another Exception. DatabaseConnectionRefusedException, and I have added that to the catch in MigrateMessageController. This exception was added in March 2023. Is there a way to catch all Core database exceptions without listing each one? Is that even a good idea?

2. I removed the changes to ProfileFieldValues because it will call \Drupal\migrate\Plugin\migrate\source\SqlBase::setUpDatabase which tries to make the connection and will throw an exception that the Controller should catch.

3. I tested the use of the permission, 'view migration messages'. I created an authenticated user and gave them that permission and they were not able to access the migration messages pages. A 403 was logged but the user was sent to the home page with no message. I tried adding more 'system' permissions but it never worked. Am I missing something here?

1. I think it is fine to add the new exception class to the catch block. These exceptions, like RequirementsException, extend \RuntimeException. So there is no base class we can use. We could match $e::class against the namespace, but that seems hacky. Let's leave this as it is.
2. I guess most of the others call $this->getDatabase()->schema()->fieldExists() or $this->database->schema()->fieldExists(), but this one does not, so that makes sense. Should we also revert the changes to d7_user?
3. Can you test again? That is the behavior I would expect before applying the changes I suggested in the MR. After those changes, we test exactly this in the PHPUnit test, and I just tested manually and got the expected result.

Back to NW for (2).

1. Yes, leaving as is.
2. And I made a point of checking the source plugins and I still missed one! The changes are removed now.
3. Retesting and working as expected now. Yay!

Back to NR

I am going to break protocol: the last commit removed a comment line, and I am restoring it. The rest of the change looks good. Back to RTBC.

The restoration of the comment that I accidentally deleted is correct. Thanks @benjifisher.

Hoping for a better title.

I triaged this RTBC issue → sometime in the last weeks but didn't leave a comment.

I didn't find any questions unanswered, that was made easier because benjifisher is very good at ensuring everything is addressed. Thanks @benjifisher!

The IS didn't have a release note snippet, so I have added one.

Leaving at RTBC

Rebase and hide file.

I've fixed the MR to not use $_SESSION - see #2473875: Convert uses of $_SESSION to symfony session retrieved from the request → for prior art. However we I did this I intentionally broke it and ran the tests to see if we have coverage of \Drupal\migrate\Form\MessageForm and I do not think that we do.

I've also updated the MR for 📌 [Needs backport] Use autowiring for core controllers RTBC and adding typehints everywhere.

@alexpott, thanks for the review.

Added a test, which included making a test base class.

Appeared feedback for additional test was added in https://git.drupalcode.org/project/drupal/-/merge_requests/4195/diffs?co...

There has been a lot of activity in the last day or two. After sitting in RTBC for 5 or 6 months, it looks like +384/-275 in 7 files. I am moving the issue back to NR and assigning it to myself.

I made some suggestions on the MR. I think I have reviewed all the recent changes, but not done a comprehensive re-review.

@alexpott, thanks for removing the use of $_SESSION. When @quietone and I were working on this, we were not sure how to do that, and we figured "if it is good enough for dblog, then it is good enough for us." (See Comments #13, #42.6, #42.15, ...)

That is a big enough change that I want to repeat the manual testing (as in Comment #117). I will not do anything as comprehensive as #125.

@benjifisher, thanks for the review.

I think I have responded to to all the issues in the MR. Ready for review again. :-)

I did a little manual testing, just to make sure that sorting and filtering are not completely broken. With the custom migration from Comment #117, I get two messages, both warnings. I tested what I could, and it still works.

I made two tiny suggestions on the MR, and I do not insist on either one. Other than that, I am ready to move this issue back to RTBC.

@benjifisher, thanks again. I have made the suggested changes.

@quietone:

Thanks! I am happy to set this issue back to RTBC.

Committed d4abe11 and pushed to 11.x. Thanks!

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