Add ability to view migrate_message table data

Related: #2561413: Consider reimplementing message tables using LoggerInterface.

Very rough initial PoC. Based on \Drupal\migrate_tools\Commands\MigrateToolsCommands::status().

#2969551: Migrate messages from caught exceptions need file and line details will probably also help make this more useful!

Drush has a command to view this data, drush mmsg {migration_name}

Adding tag

Having the message available via the UI is a good idea and will help some folks out there. I applied the patch and looked at the messages at admin/reports/upgrade-messages and my initial reaction was, Oh how weird to see these messages when I have only run migrations with drush. After that I noticed that I couldn't see a pattern to the order of the display and I personally don't like the display format, it is too different from the Review Form.

But mostly it needs to be searchable. I'm don't know how to do that so any advice on this will be appreciated.

Here is a different version with a filter and the list is based on the migrations that have existing message tables instead of source plugin requirements and using the same icons and the ReviewForm.

I don't like that the display is filter by migration_id, a value that will have no real meaning to the users of the one click upgrade. The label could be used but there is no guarantee that it will be unique.

I think we should follow the example of /admin/reports/dblog. In the dblog module, see src/Form/DblogFilterForm.php and src/Form/DblogFilterForm.php.

• Refactor into a page controller and a form.
• Use the standard submitForm method for one of the two actions.
• Also see if we can simplify the render arrays and resulting markup following the example in #3134300: Simplify ReviewForm::buildForm() and the resulting markup.

I also think it is in scope for this issue to update MigrateUpgradeImportBatch::finished() so that it shows a link to the new page as well as the upgrade log.

I'm working on it

I borrowed the code from migrate_tools and created a simplified overview/details UI for this. Patch in progress

Tested on 9.1.x

Queued the testing in #17, and changing the status to NR

@fredysan, thanks the screenshots look good.

I applied that patch and navigated to /admin/reports/upgrade-messages and saw a list of the migrations with messages, just like #17 upgrade-overivew.png. Navigating to any details page results in a 404.

Then I looked at the patch and it seems that this is relying on migrations being entities but they aren't in core. What is the next step here?

Right, using config entities for migrations is only possible with the Migrate Plus module. I think that is the main reason that Migrate Tools depends on Migrate Plus.

The fact that we get a 404 response with manual testing after the testbot comes back green is telling us something: we need more test coverage for this patch.

If this is going to rely on migrate plus then it need to be done in contrib. More testing won't solve that.

I was not able to replicate the 404 error.

I'm using docksal to create two sites: One for D9 and one for D7. On the D9 site I truncated the database, I ran the upgrade process and went to admin/reports/upgrade-messages getting the list of migrations and messages count. Navigating to the details page, like /admin/reports/upgrade-messages/d7_menu it returned the list of messages. No contrib modules were installed.

In this new patch I removed the migrate plus dependency, since it was only used by the title callback.

@fredysan, How do you 'run the upgrade process'?

@quietone, you need to enable the Migrate Drupal module and:

1. Navigate to /upgrade/incremental
2. Click on `Import new configuration and content from old site`
3. Fill out the D6/D7 database connection settings
4. Click on `Review upgrade`
5. Click on `I acknowledge I may lose data. Continue anyway.`
6. Click on `Perform upgrade`

@fredysan, Did you run the initial upgrade from /upgrade as well?

I had a quick look at the patch.

From #19:

Then I looked at the patch and it seems that this is relying on migrations being entities but they aren't in core.

In my quick look, I did not see any explicit use of config entities. Perhaps the 404 responses were because of the title callback, fixed in #22?

I need some sleep now, but I will try to test soon and give a proper review. If it looks OK, it will still need

• Code cleanup (remove commented-out lines, for example)
• Test coverage
• Updated issue summary: screenshots in the "User interface changes" section
• Updated issue summary: the "Proposed resolution" should describe the actual solution

Retested and now see the messages. Phew!

Just some minor changes to the patch for coding standards and removing unused variable.

I'd like us to be sure we want to use 'migration' in these forms. I am thinking of a user with a non complicated site that uses /upgrade and then navigates to the logs and gets information about 'migrations' when they are expecting information about 'upgrades'.

For the record, I decided to test the patch in #12. Here are my notes.

1. On a fresh site install, with migration modules enabled, I visit /admin/reports/upgrade-messages and get the error message

    Failed to connect to your database server. The server reports the following message: No database connection configured for source plugin variable.

   We should check for whether migrations have run and give a better message if they have not. The page shows empty filters, which is messy.

2. I ran the migrations. When it finished, I was redirected to the home page with the message "Page not found". The log showed that the referrer was https://drupal.lndo.site/batch?id=2&op=start and the message was "/". There was also a PHP error: "Symfony: No routes found for"/". in Drupal->matchRequest() (line 112 of /app/core/lib/Drupal/Core/Routing/Router.php)."

3. Out of scope, but we should add an issue for this if we do not already have one. The migration creates several blocks for the admin theme: at the top of the page, there is a Search block and two broken blocks. The broken blocks are from the Forum module, and enabling that module fixes them. At the bottom of the page: Recent comments, Who’s new, Who’s online, and User login. Why are all these blocks enabled in the admin theme?

4. When I visited /admin/reports/upgrade-messages, I saw the filters at the top of the page. I did not test them. By default (no filters) the page has information about all migrations, with no paging. That is OK for my little test migration database, but on a real site there might be thousands of messages just from the file migrations. Here are some screenshots.

   

   

I repeated the test with the current HEAD of 9.1.x. The problems in #30.1 and #30.2 are still there, so they are not related to this patch. We should open follow-up issues for those. For now, I will just add the issue tag.

Another problem: we get a link "Review the detailed upgrade log" (status message) going to /admin/reports/upgrade. That path redirects to /admin/reports/dblog and is supposed to set the filter to show messages from migrate_drupal_ui. The redirect works, but the filter does not. Coincidentally, that is the filter that I suggested we use as a model for this issue.

I think I generated the test database with a clean install of Drupal 7 and the Devel Generate module. It is possible that the blocks are enabled in the admin theme after migration because they were enabled in the source database. I do not think so.

re #32: the reason for the discrepancy in the log filters is related to #2904546: admin/reports/upgrade redirect doesn't handle view arguments when enabled.

@heddn, thanks for that reference.

I repeated the test with the patch from #29. There are enough problems here that I will not do a code review yet. NW

1. I get the same error message that I reported in #30.1. Instead of empty filters, I get an empty table with reasonable "no results" test: "No migrate messages available." (Except, as noted in #29, we might want to avoid "migration" in the UI text.)

2. We now have two entries in the admin menu: "Upgrade messages" and "Upgrade log". I think we should just have one, going to the new messages page. We can add a link there to the dblog page. If we fix it now, then we do not need a separate issue. (But the core committers might prefer a separate issue to avoid scope creep.)

3. Here is a screenshot of the new page. Do we want an option to show all migrations, even those with no messages?
   

4. Here are some screenshots of the individual pages. Why is the "Menu_name" column empty? Also, is menu_name a machine name? Should we be using a label instead of an id for the table header?
   
   

5. The severity column should be "error" or "warning", matching what is in the filter, not numeric.

6. The default sort for the File messages is on fid. (Again, is there a label we can use?) I can sort on the Message column. (It seems to be case insensitive.) Paging seems to work. Filtering on the message seems to work. If severity 1 corresponds to Error, then filtering on severity also works.

7. Look at the right side of the table header. I see the problem: there are three columns in the header row but four in the body rows. Looking at the markup, I see that the fourth is like <td 0="migrate-message-1" 1="migrate-message-1"></td>. That looks like a mistake.

Thanks for the reviews.

There is a lot there and this addresses just a few of the items in #33.

1. Changed migration to upgrade.
2. Deleted the upgrade log menu item.
5. Fixed but I think the column should not be shown at all because there is a filter and to be consistent with dblog which doesn't have a severity column.
7. Removed the extra column.

Todo
3. I can't decide. I tried it locally and the list of all those migrations was daunting and didn't add value.
4, 6

And now something, done quickly, for 33.4 and 6. The column headers for the individual migrations now uses the text from the fields() method instead of the source id key from getIds().

33.4 I too had blank entries for the menu name column but I wasn't working from a clean install so that needs more investigation.

From 33 there is still
#3 Decide if there should be an option to show all migrations, even if the message count for the migration is 0.
#4 track the cause why there are empty menu name fields.

setting to NW for 33.3 and 33.4

just a reroll.

Another pass at this.

#30.1. I am not able to reproduce this on a fresh minimal install of 9.2.x. With migrate and migrate_drupal installed navigating to /admin/reports/upgrade-messages results in a 404. With migrate_drupal_ui installed the message 'No upgrade messages available.' Screenshot added.

#30.2 There is an issue for the front page resulting in "Page not found", #2607754: Add intelligence to front page configuration migration. Therefor, a followup isn't necessary, so removing tag.

33.3 No, listing the migration that do not have messages is just clutter. I tried it locally and the list of all those migrations was daunting and didn't add value.
33.4 The menu name is not available because there is no corresponding row in the migrate_map table. 'Config entities can not be stubbed.' is thrown during the processing of the row and save is set to FALSE, therefor no map row. This patch add 'Not available' for any field that does not have a value. There is a screen shot of the in the IS.
33.6 The text for the label 'File ID' is from File::fields(), so it can easily changed. What should it be?

This patch removed the column of Severity level from the display. That was done because that is how the log messages are shown. The filter for severity remains.

The menu source plugin is changed to add a check if the database is set or not to avoid a call on NULL. That should probably change to actually getting the database connection. The field source plugin was changed to add an entry for 'entity_type' in the fields method. Having to make those two changes suggests that other source plugins may need a tweak.

I am reviewing this issue.

I tested the patch in #40. I do not remember when I last ran the update, but I started with a site that already has migrated content.

I think you are right that showing all migrations is overwhelming and does not add anything.

I see that there is just one link in the admin menu, and that the phantom column is no longer there. Thanks!

1. I did a fresh site install, then enabled migrate, migrate_drupal, and migrate_drupal_ui. (I also enabled claro and oliveri.) When I visit /admin/reports/upgrade-messages, I get the same error message that I reported in #30.1 and #33.1. If I reload the page, then the error message goes away, If I clear caches and reload the page, then it comes back. (I did this after the tests described below.) I do not see any log messages after the one for installing migrate_drupal_ui.

2. There are four messages from the menu migration. They are all the same: Menu name "Not available", Message "Config entities can not be stubbed." This is not very actionable. If I understand #40 correctly, the problem is that there are messages in migrate_message_% with no corresponding entry in migrate_map_%. That seems like a bad idea, but fixing it is out of scope for this issue. Can we detect when this happens and add some help text (above or below the table) along the lines of the explanation in #40?

3. I do not think this table is very similar to the dblog one. We do not have type and date, and we do not have a link to a detailed message. I think I would like to see the severity column restored (but with labels rather than keys).

4. My screenshot in #33.4 shows that all the / characters have been removed from file paths on the page for d7_file. In today’s test, most of the / characters are now there, but every message looks like File '//sitesdefault/files/...' does not exist. Can we restore the separator in sites/default?

5. On the page for d7_search_settings, I see the error message

    Notice: Undefined index: id in Drupal\migrate_drupal_ui\Controller\MigrateController->details() (line 178 of core/modules/migrate_drupal_ui/src/Controller/MigrateController.php).

   and the first column has an empty header. Is this a symptom of needing to update all the source plugins? I am not sure whether updating those should be a follow-up issue, but if that is what causes this problem, then I am leaning towards saying that we should fix them in this issue. Or maybe this is the problem with preg_replace() in (12) below.

   I see the same thing on other pages: d7_system_file, d7_system_mail.

6.   +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
     @@ -3,12 +3,88 @@
     ...
      class MigrateController extends ControllerBase {

   The API docs for ControllerBase say this:

   Utility base class for thin controllers.

   Controllers that use this base class have access to a number of utility methods and to the Container, which can greatly reduce boilerplate dependency handling code. However, it also makes the class considerably more difficult to unit test. Therefore this base class should only be used by controller classes that contain only trivial glue code. Controllers that contain sufficiently complex logic that it’s worth testing should not use this base class but use ContainerInjectionInterface instead, or even better be refactored to be trivial glue code.

   The services exposed here are those that it is reasonable for a well-behaved controller to leverage. A controller that needs other services may need to be refactored into a thin controller and a dependent unit-testable service.

   So maybe we should be refactoring. On the other hand, we are not doing anything more complicated than the dblog module, so maybe it is OK.

7.   +  /**
     +   * {@inheritdoc}
     +   */
     +  public static function create(ContainerInterface $container) {
     +    return new static(
     +      $container->get('database'),
     +      $container->get('form_builder'),
     +      $container->get('plugin.manager.migration')
     +    );
     +  }

   Since ControllerBase already implements ContainerInjectionInterface, we can use the simpler pattern of dependency injection. Remove the constructor (and related use statements) and

     public static function create(ContainerInterface $container) {
       $controller = parent::create($container);
       $controller->database = $container->get('database');
       $controller->formBuilder = $container->get('form_builder');
       $controller->migrationPluginManager = $container->get('plugin.manager.migration');
       return $controller;
     }

8. Should these be translated? If so, and if we cannot call t() in here, then we should do it in the constructor and disregard the previous suggestion.

    +  protected $levels = [
    +    MigrationInterface::MESSAGE_ERROR => 'Error',
    +    MigrationInterface::MESSAGE_WARNING => 'Warning',
    +    MigrationInterface::MESSAGE_NOTICE => 'Notice',
    +    MigrationInterface::MESSAGE_INFORMATIONAL => 'Info',
    +  ];

9. If createInstances() caches its results, then this might explain the error message in (1):

    +  public function overview() {
    +    $migrations = $this->migrationPluginManager->createInstances([]);

10.   +    foreach ($migrations as $mid => $migration) {
      +      $map = $migration->getIdMap();
      +      $messages = $map->messageCount();
      +      if ($messages > 0) {
      +        $row['migration'] = $migration->label();
      +        $row['machine_name'] = $mid;
      +        $route_parameters = [
      +          'migration' => $migration->id(),
      +        ];
      +        $row['messages'] = [
      +          'data' => [
      +            '#type' => 'link',
      +            '#title' => $map->messageCount(),
      +            '#url' => Url::fromRoute("migrate_drupal_ui.messages.detail", $route_parameters),
      +          ],
      +        ];
      +        $rows[] = $row;
      +      }
      +    }

    Can you rename $messages to $message_count and use it for #title instead of calling messageCount() again? Alternatively, eliminate $messages by calling messageCount() inside the if().

    Can you initialize $row = [] each time through the loop? I see that it has 4 keys, and each one is set, so the code works as is. But some later edit to this code might set a new key conditionally.

    I would rather exit early: if ($message_count = 0) { break; }.

    Is $migration->id() always the same as $mid?

11. drupal_render()?

     +   * @return array
     +   *   A render array as expected by drupal_render().
     +   */
     +  public function details($migration) {

12.   +    foreach ($source_id_field_names as $source_id_field_name) {
      +      $display_name[$source_id_field_name] = preg_replace(
      +          [
      +            '/^[Tt]he /',
      +            '/\.$/',
      +          ], NULL, $fields[$source_id_field_name]) ?? $source_id_field_name;
      +    }

    I think this would be easier to read if you started with $patterns = ['/^[Tt]he /', '/\.$/']; outside the loop.

    I checked https://www.php.net/preg_replace, and the second argument is declared as string|array, so let’s use '' instead of NULL.

    I think you meant to put ?? $source_id_field_name inside the parentheses.

13. Remove the extra .:

     +    // Get map and message table for each row in the message table..

    Maybe also strike the first "table".

14. Can we simplify this with ?: or ???

     +      for ($count = 1; $count <= $num_ids; $count++) {
     +        $mapkey = 'sourceid' . $count;
     +        $row[$mapkey] = $message_row->$mapkey ? $message_row->$mapkey : 'Not available';
     +      }

15. I guess we are not trying to hide the fact that we are borrowing code from the dblog module, but maybe we should update the comment:

     +  /**
     +   * Builds a query for database log administration filters based on session.
     +   *
     +   * @return array|null
     +   *   An associative array with keys 'where' and 'args' or NULL if there were
     +   *   no filters set.
     +   */
     +  protected function buildFilterQuery() {

16. I owe you a review of the form class.

Here is a review of the form class and the change to the system module.

1. Use MigrationPluginManagerInterface:

    +++ b/core/modules/migrate_drupal_ui/src/Form/MessageForm.php
    @@ -0,0 +1,180 @@
    ...
    +  /**
    +   * The migration plugin manager.
    +   *
    +   * @var \Drupal\migrate\Plugin\MigrationPluginManager
    +   */
    +  protected $migrationPluginManager;

2. Despite the claim in the doc block, this property is never used:

    +  /**
    +   * The options for the select list.
    +   *
    +   * @var array
    +   */
    +  protected $options = [];
    ...
    +  /**
    +   * Sets up the options and migrations array.
    +   *
    +   * @throws \Drupal\Component\Plugin\Exception\PluginException
    +   */
    +  protected function setUp() {

3. As in #42.7, we can use the simpler pattern for dependency injection. Get rid of the constructor and a use statement or two.

    +  /**
    +   * MessageForm constructor.
    +   *
    +   * @param \Drupal\migrate\Plugin\MigrationPluginManager $migrationPluginManager
    +   *   The migration plugin manager.
    +   */
    +  public function __construct(MigrationPluginManager $migrationPluginManager) {
    +    $this->migrationPluginManager = $migrationPluginManager;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function create(ContainerInterface $container) {
    +    return new static(
    +      $container->get('plugin.manager.migration')
    +    );
    +  }

   If you prefer to keep the constructor, then use the interface. We usually use snake_case for the parameter to the constructor and camelCase for class properties. If you want to switch to camelCase for everything, then do it consitently in the file.

4. The create() method should also call setStringTranslation(). From the API docs for FormBase:

   To properly inject services, override create() and use the setters provided by the traits to inject the needed services.

    public static function create($container) {
      $form = new static();
      // In this example we only need string translation so we use the
      // setStringTranslation() method provided by StringTranslationTrait.
      $form->setStringTranslation($container->get('string_translation'));
      return $form;
    }

5. Shouldn’t we inject the database service?

    +  protected function setUp() {
    +    if (!$this->migrations) {
    +      $this->migrations = [];
    +      $migrations = $this->migrationPluginManager->createInstances([]);
    +      foreach ($migrations as $id => $migration) {
    +        $message_table = $migration->getIdMap()->messageTableName();
    +        if (\Drupal::database()->schema()->tableExists($message_table)) {

6. Why new TranslatableMarkup() instead of $this->t()?

    +    if (!$this->levels) {
    +      $this->levels = [
    +        MigrationInterface::MESSAGE_ERROR => new TranslatableMarkup('Error'),
    +        MigrationInterface::MESSAGE_WARNING => new TranslatableMarkup('Warning'),
    +        MigrationInterface::MESSAGE_NOTICE => new TranslatableMarkup('Notice'),
    +        MigrationInterface::MESSAGE_INFORMATIONAL => new TranslatableMarkup('Info'),
    +      ];
    +    }

7. This is one of the changes you mentioned at the end of #40:

    +++ b/core/modules/system/src/Plugin/migrate/source/Menu.php
    @@ -31,11 +31,13 @@ public function fields() {
    ...
    -    if ($this->database->schema()->fieldExists('menu_custom', 'language')) {
    -      $fields += [
    -        'language' => $this->t('Menu language.'),
    -        'i8n_mode' => $this->t('Menu i18n mode.'),
    -      ];
    +    if ($db = $this->database) {
    +      if ($db->schema()->fieldExists('menu_custom', 'language')) {
    +        $fields += [
    +          'language' => $this->t('Menu language.'),
    +          'i8n_mode' => $this->t('Menu i18n mode.'),
    +        ];
    +      }

   In PHP 8, it will be simpler: $this->database?->schema()->fieldExists(...). (I think)

   This fix seems out of scope, and you suggest in #40 that it might not be the right fix. Maybe we should handle this in a separate issue. Or maybe just do the right fix: I think we should replace $this->database with $this->getDatabase() (since this class extends DrupalSqlBase, which extends SqlBase).

This addresses some of the points in #42.

10. Fixed - suggestions implemented.
11. Removed reference to drupal_render().
12. Fixed
13. Fixed
14. Fixed
15. Reference to dblog removed.

Oh bother. I still forget to run commit-code-check.

This should be the rest of #42.

1. The form only displays now if there are migration tables. It will display, 'There are no upgrade tables.'.
2. Added structure to display text explaining 'Not Available' but don't yet know what the text should be. I'd like to know what other cases there are that cause rows to be written without a matching entry in the map table.
3. Restored the severity level to the display.
4. Testing today and files that do not exist have all the '/' characters, ie sites/default/files. If this is still a problem it will be elsewhere and have to be done in a follow up. The work here does not modify the stored message in any way.
5. I can't reproduce that with the d7 fixture.
6. @todo, revisit when writing tests?
7. Fixed
8. Arg. It is now translated when building the row.
9. Follow on from 1, which now had a potential fix.

And removed unused method, public static function getLogLevelClassMap()

Oh bother, again!

No interdiff because it was a problem with file permissions. I have no idea how they changed.

And now changes for all items in #43.

All the items in #42 and #43 should now be addressed, and most of those fixed. It is too late for me to self review or to provide new screenshots. I did add in the remaining tasks that a followup might be needed for the changes in Menu.php.

This still needs tests for the two new forms.

@quietone, thanks for the quick response! I have not re-tested, and I think I want (2) below to be addressed before I do. This review just goes through the updates since my reviews in #42 and #43, so I should do another comprehensive review after the next update.

1.   +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
     @@ -3,12 +3,48 @@
     ...
     +  /**
     +   * The message severity levels.
     +   *
     +   * @var array
     +   */
     +  protected $levels = [];

This variable is set in overview() and used in details(). Does that work? (I have not re-tested.) Even if it does, why not remove the class property? Make it a local variable in details(): set it and use it there.

1.   +    foreach ($source_id_field_names as $source_id_field_name) {
     +      $pattern = [
     +        '/^[Tt]he /',
     +        '/\.$/',
     +      ];
     +      if (isset($fields[$source_id_field_name])) {
     +        $display_name[$source_id_field_name] = preg_replace(
     +            $pattern, '', $fields[$source_id_field_name]) ?? $source_id_field_name;
     +      }
     +      $display_name[$source_id_field_name] = empty($display_name[$source_id_field_name]) ? $source_id_field_name : $display_name[$source_id_field_name];
     +    }

   Why 4 spaces of indentation after preg_replace(?

   I still think that ?? $source_id_field_name should be inside the parentheses.

2. The last line of the same snippet can be simplified with ??.

3. I still do not like this comment. We are getting the map and message tables (plural) and then, for each row in the message table, we are getting … what? Maybe it is not worth saying that we are geting the tables, and the comment should be something like "Get the source ID(s) for each row in the message table."

    +    // Get map and message table for each row in the message table.

4. Initialize $row = []; eachtime through the loop, for the same reason as #42.10.

    +    foreach ($result as $message_row) {
    ...
    +      $rows[] = $row;
    +    }

5. I think you missed #43.4.

6. Missing @var comment:

    +++ b/core/modules/migrate_drupal_ui/src/Form/MessageForm.php
    @@ -0,0 +1,164 @@
    ...
    +  protected $database;

7. Can we make this a one-line change, $this->getDatabase()->schema()->fieldExists(...), or do we still have to test that $this->getDatabase() is not empty?

    +++ b/core/modules/system/src/Plugin/migrate/source/Menu.php
    @@ -31,11 +31,13 @@ public function fields() {
    ...
    -    if ($this->database->schema()->fieldExists('menu_custom', 'language')) {
    -      $fields += [
    -        'language' => $this->t('Menu language.'),
    -        'i8n_mode' => $this->t('Menu i18n mode.'),
    -      ];
    +    if ($db = $this->getDatabase()) {
    +      if ($db->schema()->fieldExists('menu_custom', 'language')) {
    +        $fields += [
    +          'language' => $this->t('Menu language.'),
    +          'i8n_mode' => $this->t('Menu i18n mode.'),
    +        ];
    +      }

@benjifisher, thanks for the quick response and checking if all the points in #42 and #43 were done.

This should address everything in #49.
1 (the first 1). Cleaned up the use of $levels in both forms.
1 (the second 1). PhpStorm does that when it reformats code and I've yet to find a way to fix it except by hand.
2. Changed the logic on that one.
3. Changed the comment, choose this instead, // Gets each message row and the source ID(s) for that message.
4. Done
5. I've added that. Although I don't understand what and there are no other usages like that in core that I could look at.
6. Oops, fixed.
7. Fixed.

I also did a read through and changed some variable names, most notable migration to migration_id when it really was the id, which changed the routing parameters. Also added some comments.

Note. The test should test when the source id is not in fields().

To be honest, I am not sure why we are supposed to use setStringTranslation(). I was just following instructions. I see that it is called in the constructor for Drupal\breakpoint\BreakpointManager, and it is called with some sort of stub implementation in Drupal\Tests\breakpoint\Unit\BreakpointTest. I do not see it used in create() anywhere, but that is not much different from using it in a constructor.

I will try to review the latest patch before Thursday's meeting. We still need some tests.

A start on the tests.

Improvements to the test.

Added test source plugin with a missing source id in the return from fields().

This needs to gracefully handle the situation when the source database is not available or even a migration. I mean the data is there, people should be able to see it.

Sorry, I did not get to this before the weekly migration meeting.

I have not looked at the tests yet, but I notice that the testbot says "Custom Commands Failed". What does that mean?

Follow up to #49.1 (the first one, sorry about that): $levels is now a local variable in the function, but there is also an unused class property named $levels. I think you forgot to remove it.

Follow-up to #49.7: I suggested that you make it a one-line change, but now you have changed an if block to a try/catch block. I assume there is a good reason for that, but I do not know what it is. Can you add a code comment with an explanation? Also, I think we should mention this change in the IS (Proposed resolution section) in order to explain why it is in scope. Same point for the change to the File plugin.

The other points raised in #49 are all fixed. +1

@benjifisher

I have not looked at the tests yet, but I notice that the testbot says "Custom Commands Failed". What does that mean?

Since #3178845: Run same checks as committers do on DrupalCI landed each file in a patch is scrutinized in the Custom Commands phase, which runs a script before any of the tests starts.

This script (core/scripts/dev/commit-code-check.sh) checks for:

# The script makes the following checks:
# - Spell checking.
# - File modes.
# - No changes to core/node_modules directory.
# - PHPCS checks PHP and YAML files.
# - Eslint checks JavaScript files.
# - Checks .es6.js and .js files are equivalent.
# - Stylelint checks CSS files.
# - Checks .pcss.css and .css files are equivalent.

If it fails any of the above, tests aren't being run on Jenkins.

If you look at the results for this patch, you'll quickly see it has found a few PHPCS nitpicks.

Yes, I made that patch late in the day and forgot to run commit-code-check. I also moved the test to a new file and left all the code in the original file. So it is good it wasn't reviewed.

This should be better.

There is also an confusing mix of name when the patch from #2713327: Document ways to remove migration tables (ID map etc.) is applied as well. In one patch they are called Upgrade Messages and in the other Migrate Tables. We need to be consistent.

Spokje: Thanks for the explanation!

I am about to start looking at the tests, but for now I am just following op on #49.1 and #49.7 (continuing from #54).

1. #49.1: done.

2. #49.7: Thanks for adding the comment:

    +    // 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 (RequirementsException $e) {
         }

   I tested this by reinstalling Drupal (drush sql-drop and drush si standard) and enabling the migrate_drupal_ui module. I inserted a line into the catch block, logging a message. Then I visited /admin/reports/upgrade-messages and followed the link to /admin/reports/dblog?type[0]=migrate_drupal_ui ("Review the detailed upgrade log"). I did not see my debugging message, but I did see "Illegal choice migrate_drupal_ui in type element.". I also checked /upgrade/credentials to make sure that my database credentials were not coming from somewhere else (like a cookie or local storage).

   I still do not see why we need the try/catch block instead of a 1-line change. Ideally, we would avoid the "Illegal choice" error, but that is out of scope for this issue. It comes from the unchanged method MigrateController::showLog(), which does not test for existing log messages before setting the query parameter.

I still have not looked at the automated tests, because I noticed a problem in manual testing:

    +    // Build the rows to display.
    +    $rows = [];
    +    $add_explanation = FALSE;
    +    $num_ids = count($source_id_field_names);
    +    foreach ($result as $message_row) {
    +      $new_row = [];
    +      for ($count = 1; $count <= $num_ids; $count++) {
    +        $mapkey = 'sourceid' . $count;
    +        $new_row[$mapkey] = $message_row->$mapkey ?? NULL;
    +        if (empty($row[$mapkey])) {
    +          $new_row[$mapkey] = $this->t('Not available');
    +          $add_explanation = TRUE;
    +        }
    +      }
    +      $new_row['level'] = $levels[$message_row->level];
    +      $new_row['message'] = $message_row->message;
    +      $rows[] = $new_row;
    +    }
    +
    ...
    +
    +    if ($add_explanation) {
    +      $build['explanation'] = [
    +        '#type' => 'item',
    +        '#markup' => $this->t("Sample explanation of 'Not available'"),
    +      ];
    +    }

1. It looks as though there is one place where you forgot to update $row to $new_row. As a result, all the source ID columns in the table body are set to "Not available".

2. We need a real explanation instead of the placeholder text. How about this?

   When there is an error processing a row, the migration system saves the error message but not the source ID(s) of the row. That is why some messages in this table have "Not available" in the source ID column(s).

   In UI text, I do not think we have to distinguish between errors and exceptions. (Or maybe this draft is accurate: does this happen only with errors?) I am a little worried about using "source ID", but maybe it is clear enough in context.

3. Follow-up to #58.2: I reinstalled Drupal and reverted the change to Menu.php. (That is, I restored the 9.2.x version of that file.) I do not see any problems. Do we need to change anything in that file?

This test patch is for showing the error with Menu.php and why I added a try/catch. Two changes to the latest patch are needed to show the error, one is too remove the changes to Menu.php and the other is to remove the tests in MigrateController that checks if there are messages before attempting to display the form. However, the problem will still happen if there are migrate tables for the menu migration and there is no db connection to the source. Actually, as I write this I realize that should have a test but also wonder if the catch should catch all exceptions because \Drupal\migrate\Plugin\MigrateSourceInterface::fields is declared to not throw exceptions.

No need to run tests with this patch but if you apply it and do not have a source connection set and navigate to admin/reports/upgrade-messages you get the error message.

Failed to connect to your database server. The server reports the following message: No database connection configured for source plugin variable.

    Is the database server running?
    Does the database exist, and have you entered the correct database name?
    Have you entered the correct username and password?
    Have you entered the correct database hostname?

59.1 Fixed.
59.2 No change yet. I too am concerned about using 'sourceID'. This will need some thought.
59.3 I hope that #60 addresses that.

I too want to review the test but many family duties at this time of the year.

I repeated the test (#58.2) of reverting the change to Menu.php, reinstalling Drupal, and going to /admin/reports/upgrade-messages. That page is fine, and the only link goes to /admin/reports/dblog?type[0]=migrate_drupal_ui. Any problems on that page are out of scope for this issue.

However, if I visit /admin/reports/upgrade-messages/d7_menu, then I get an error. If I restore Menu.php to the version in this issue and add a log message to the catch clause, then I see my message in the log.

If we do not provide a link to these pages, I am not sure how important it is to catch exceptions on them But if we do, let's try to cover them all.

1. I am adding a list in the issue summary of source plugins that use $this->database or $this->getDatabase() or $this->select() in their fields() methods.
2. When I visit /admin/reports/upgrade-messages/d7_node_complete%3Aforum, I get "Error: Call to a member function getSourcePlugin() on bool in Drupal\migrate_drupal_ui\Controller\MigrateController->details() (line 133 of core/modules/migrate_drupal_ui/src/Controller/MigrateController.php)." Let's add a check there. I guess the problem is that this is a derived migration that has not been derived yet.

I looked at the tests. The main problem is that they are incomplete, as noted by the @todo comments. It looks as though you did the hard part, supplying test data for the map and message tables, but you have not tested that yet.

Besides that, there are a few nits:

1. The description should have ending punctuation. We often use quotation marks for the name and description, but YAML does not need them.

    +++ b/core/modules/migrate_drupal_ui/tests/modules/message_test/message_test.info.yml
    @@ -0,0 +1,5 @@
    +name: Test display of migrate message
    +type: module
    +description: Tests the display of migrate messages

2. Update the comment (copied from another test module).

    +++ b/core/modules/migrate_drupal_ui/tests/modules/message_test/src/Plugin/migrate/source/MenuTest.php
    @@ -0,0 +1,26 @@
    ...
    +/**
    + * A test source plugin without a source_module.

3. All of these functions should use {@inheritdoc}. In fact, they should not be undefined in the parent class, but that is out of scope. (We have discussed this before, haven’t we?) Should we return the correct type of empty ('' or []) instead of NULL?

    +  /**
    +   * Gets the source base path for the concrete test.
    +   *
    +   * @return string
    +   *   The source base path.
    +   */
    +  protected function getSourceBasePath() {
    +  }

4. It is annoying that we have to specify all these fields explicitly. Maybe (not as part of this issue!) we should refactor Drupal\migrate\Plugin\migrate\id_map\Sql::ensureTables(). For now, maybe add an @see comment.

    +  /**
    +   * Creates d7_menu and d7_menu_test map and message tables for testing.
    +   */
    +  protected function createTables() {

This patch address the points in #63 and makes changes to some comments.

Still to do is all of #62. My one attempt to get an error from going to /admin/reports/upgrade-messages/d7_node_complete%3Aforum has failed. That is next.

This patch is cleanup, it removes unused method and variables from MessageForm.

I've been dabbling in a version of this that does not require a source database connection and should be able to post a patch tomorrow.

Here is a version that does not require a source db connection to display the tables. During manual testing I found that \Drupal\system\Plugin\migrate\source\Menu::fields was failing due to a PDOException. That was when the database connection existed but the database name was incorrect.

This should also fix #62.2.

Still to do is do work for the other source plugins, listed in the IS, that query the database.

Without migrations the displayed values are different. Here is the overview page.

@quietone, here is a review based on the changes since #61. I will do a more comprehensive review once you have finished all the to-dos.

1. Nit: $id is unused.

    +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
    @@ -23,4 +53,308 @@ public function showLog() {
    ...
    +    foreach ($tables as $id => $table) {
    +      $migration_id = str_replace('migrate_message_', NULL, $table);
    +
    +      try {
    +        $migration = $this->migrationPluginManager->createInstance($migration_id);
    +      }
    +      catch (\PDOException $e) {
    +        // The source database connection may not exist.
    +      }

2. Same snippet: the second argument of str_replace() is declared as string|array, so let’s use '' instead of NULL.

3. I have not tested yet, but is this a reliable way to get the migration ID? For derived migrations and long migration IDs, I think we do some munging/truncating when creating table names.

4. Break this line to keep it under 80 characters:

    +      $message_count = $this->database->select($table)->countQuery()->execute()->fetchField();

I think I will stop my review here, since (3) is so important. If we cannot reliably get the migration IDs (and then the migrations) then we should go back to the earlier approach. Also, the MigrateController::details() method is getting unwieldy. I assume you are still experimenting, and it is not really ready for review. Sooner or later, we have to resolve the discussion of "thin controllers" started in #42.6.

For 68
1. Fixed
2. Fixed. Should have remembered this one, just fixed it somewhere else based on your review.
3. Agree, this is not ideal and needs more thought. It was more POC.
4. Fixed

Your assumption is correct and I agree with last paragraph. I've added those points to the Remaining Tasks in the IS.

I have searched the patch and there are no more todos. So, this is as tidy as will get until work is done on a) the reliability of getting the migration id from the table name and b) making a thin controller.

Setting back to NW for the items in the last paragraph of #68.

68.3. Yes, I think this is a reliable way to get the table names. See https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/modules/migr.... But #2845340: migrate mapping & messages table names are truncated, can lead to incorrect mapping lookups introduces a md5 hash on the machine_name. So, when that gets committed the only way to get the table name requires a source database connection.

This removes the ability to view the messages without the source database, adds mores assertions to the test, and adds try/catch to the fields method for each source plugin listed in the IS.

Ignore that, bad patch.

I like this approach much better, getting the migration plugins from the plugin manager rather than trying to get them from the database table names.

I did some testing, and I reviewed the controller class. I need to get some sleep, so the form class and the tests will have to wait.

1. I suggested some text for this message in #59.2. Remember to update the tests.

    +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
    @@ -23,4 +53,265 @@ public function showLog() {
    ...
    +    if ($add_explanation) {
    +      $build['explanation'] = [
    +        '#type' => 'item',
    +        '#markup' => $this->t("Sample explanation of 'Not available'"),
    +      ];
    +    }
    +    return $build;

2. Nit: can you restore the blank line before return here and in the other source plugins?

    +++ b/core/modules/user/src/Plugin/migrate/source/d6/ProfileFieldValues.php
    @@ -67,15 +68,20 @@ public function fields() {
    ...
    +    catch (RequirementsException $e) {
    +    }
    +    catch (\PDOException $e) {
         }
    -
         return $fields;

3. (Same snippet.) Checking the PHP docs for catch, we could use a single catch block: catch (RequirementsException|\PDOException $e). (In PHP 8, it will be legal to omit the $e.)

4. It seems odd to include this helpful comment in just one of the source plugins. Ideally, we would have it in one central place, but I cannot think where it would be. Would it be wrong to copy/paste the comment into the other source plugins?

    +++ b/core/modules/system/src/Plugin/migrate/source/Menu.php
    @@ -31,11 +32,21 @@ public function fields() {
    ...
    +    // The database connection may not exist, for example, when building
    +    // the Migrate Message form.
    +    try {
    +      if ($this->getDatabase()
    +        ->schema()
    +        ->fieldExists('menu_custom', 'language')) {

5. Follow-up to #62: After enabling migrate_drupal_ui, but before running the migrations, I visited /admin/reports/upgrade-messages/d7_menu and /admin/reports/upgrade-messages/d7_node_complete%3Apage. I get the same behavior for both: a 404 response. If I clear caches before visiting either page, then I also get the error message

   Failed to connect to your database server. … No database connection configured for source plugin variable.

   It is odd to get the error message and the 404 response, but I think it is OK since there is no way to get to these pages other than entering the URL directly.

6. Nit: use single quotes for consistency:

    +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
    @@ -23,4 +53,265 @@ public function showLog() {
    ...
    +      $row['messages'] = [
    +        'data' => [
    +          '#type' => 'link',
    +          '#title' => $message_count,
    +          '#url' => Url::fromRoute("migrate_drupal_ui.messages.detail", $route_parameters),

7. Nit: use "ID" (all caps) consistently:

    +  /**
    +   * Displays a listing of migration messages for the given migration ID.
    +   *
    +   * @param string $migration_id
    +   *   A migration id or the possible migration ID derived from the message
    +   *   table name.

8. (Same snippet.) Are we still trying to get the migration ID from the table name?

9. I guess this explains the error message with the 404 response. Can we clear the messages before sending the response?

    +  public function details($migration_id) {
    +    /** @var \Drupal\migrate\Plugin\MigrationInterface $migration */
    +    $migration = $this->migrationPluginManager->createInstance($migration_id);
    +
    +    if (!$migration) {
    +      throw new NotFoundHttpException();
    +    }

10. Can you think of a way that just one of the tables is missing?

     +    // If both the map and message table do not exist do not continue.
     +    if (!$this->database->schema()->tableExists($message_table)
     +      || !$this->database->schema()->tableExists($map_table)) {
     +      throw new NotFoundHttpException();
     +    }

    In fact, it would be bad if we had a map table but no message table, since then there will be a link in the overview form but it will go to a 404 response.

    I think we should return a 404 if the map table is not there. Other than my testing (where I re-install Drupal and reload the page in my browser) I do not think it will ever come up. But if the map table is there and the message table is not, then we should give some sort of message.

11. Nit: use "ID" consistently in comments.

     +    // Create the column header names.
     +    $header = [];
     +    $source_plugin = $migration->getSourcePlugin();
     +    // 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.

12. We already exited early if either table was not there:

     +    if ($this->database->schema()->tableExists($message_table) && $this->database->schema()->tableExists($map_table)) {

13. What could go wrong here?

     +      $new_row['level'] = $levels[$message_row->level];
     +      $new_row['message'] = $message_row->message;
     +      $rows[] = $new_row;
     +    }
     +
     +    // Build the complete form.
     +    $build['message_filter_form'] = $this->formBuilder->getForm('Drupal\migrate_drupal_ui\Form\MessageForm');
     +    $build['message_table'] = [
     +      '#type' => 'table',
     +      '#header' => $header,
     +      '#rows' => $rows,
     +      '#attributes' => ['id' => $message_table, 'class' => [$message_table]],
     +      '#empty' => $this->t('No messages for this migration.'),
     +    ];

    If $message_row->level has some unexpected value, we will get a PHP Notice. I think that is appropriate.

    I think we have to sanitize $new_row['message'].

    Do we need an id attribute? If so, then I think we need to apply Drupal\Component\Utility\Html::getUniqueId(). If we only need a CSS class, then we should use Drupal\Component\Utility\Html::getClass().

14. Nit: make the one-line description more specific. After all, the controller class has two page callbacks. Maybe "Gets the title of the message details page." Or "… message listing page."

     +  /**
     +   * Gets the title of the page.
     +   *
     +   * @param string $migration_id
     +   *   A migration id.
     +   *
     +   * @return \Drupal\Core\StringTranslation\TranslatableMarkup
     +   *   The translated title.
     +   */
     +  public function title($migration_id) {

Clearing out the simple fixes.
1-4 Fixed
5. Good to know.
6,7 Fixed
9. Good idea, how to clear them?
11. Fixed
14. Fixed

To do

8, 10, 12, 13

Re #73.13: I was wrong about sanitizing the message. I guess Twig does that for us. I tried

    $new_row['message'] = 'foo</td></tr><tr><td>Source ID</td><td>Severity</td><td>Message';
    $rows[] = $new_row;

and I got a bunch of escaped markup, not an extra row.

Re #73.9: First, inject the messenger service. Then (untested)

      $errors = $this->messenger
        ->deleteByType(MessengerInterface::TYPE_ERROR);
      array_pop($errors);
      foreach ($errors as $error) {
        $this->messenger->addError($error, TRUE);
      }

We cannot simply clear all errors because there might be some batch process that logs messages and redirects to our page.

We should add a test that this works as expected.

This is pretty cumbersome. Maybe we should add an issue for a new method on the Messenger class.

Maybe we should make it

      $last_error = array_pop($errors);
      if (strpos($last_error, 'Failed to connect to your database server.') !== 0) {
        $errors[] = $last_error;
      }

Or maybe we should handle this in a follow-up issue. As I said in #73.5, it is not a serious problem. This example suggests that migrate_drupal_migration_plugins_alter() should not be adding that message in the first place.

I agree looking at that should be in a followup.

Add the remaining points in #73.
8. No. The table names are obtained from the idMap for each migration.
10. The only way I can think of is if someone deletes a table manually. Made changes for the suggestions. When the map table does not exist
the message is "The message table is missing for this migration."
12. Yes, no longer needed.

I think that covers everything, but I didn't sleep well, so maybe I missed something.

It wasn't everything - didn't run commit-code-check.

@quietone, thanks for all the updates! This is getting close. I reviewed the form class and the test, and none of the points below seem too complicated.

1. Follow-up to #73.2, #73.3: can you make these changes consistently for all the source plugins touched by this patch? I see that you added spaces around | in the catch lines: did you find that usage already in core?

2. Follow-up to #73.5, #73.9: I added #3198339: Improve error reporting from migrate_drupal_migration_plugins_alter().

3. Follow-up to #73.13: Sorry, I combined a few things in that item. We do not need to do anything about the others, but I think we should use Html::getUniqueId() and/or Html::getClass(). I still question whether we need an id attribute.

4. Follow-up to #73.14:

    +  /**
    +   * Gets the title of the message listings page or the details page.
    +   *
    +   * @param string $migration_id
    +   *   A migration id.
    +   *
    +   * @return \Drupal\Core\StringTranslation\TranslatableMarkup
    +   *   The translated title.
    +   */
    +  public function title($migration_id) {

   I suggested either "message listings page" or "message details page". I think having both suggests that this title callback is used for two different pages.

5. Nit (same snippet): "migration ID".

6. Does this title show up anywhere?

    +++ b/core/modules/migrate_drupal_ui/src/Form/MessageForm.php
    @@ -0,0 +1,116 @@
    +  public function buildForm(array $form, FormStateInterface $form_state) {
    +    $form['#title'] = $this->t('Upgrade messages');

7. What do you think of splitting this up into two test methods? Since it tests both overview and detail pages, the name testOverview() is a little misleading. How about testNoSourceDatabase() and testWithSourceDatabase()?

    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerMessageTest.php
    @@ -0,0 +1,392 @@
    ...
    +  /**
    +   * Tests the overview and detail pages for migrate messages.
    +   *
    +   * Tests the overview page with the following scenarios;
    +   * - No source database connection or message tables.
    +   * - No source database connection with message tables.
    +   * - A source database connection with message tables.
    +   *
    +   * Tests the detail page with the following scenarios;
    +   * - No source database connection or message tables with a valid and an
    +   *   invalid migration.
    +   * - A source database connection with message tables with a valid and an
    +   *   invalid migration.
    +   * - A source database connection with message tables and a source plugin
    +   *   that does not have a description for a source ID in the values returned
    +   *   from fields().
    +   */
    +  public function testOverview() {
    ...
    +    // Create a source database connection.
    +    $this->createMigrationConnection();
    +    $this->sourceDatabase = Database::getConnection('default', 'migrate_drupal_ui');
    +    $this->loadFixture(drupal_get_path('module', 'migrate_drupal') . '/tests/fixtures/drupal7.php');
    +    // Get the current major version.
    +    [$this->destinationSiteVersion] = explode('.', \Drupal::VERSION, 2);
    +    // Use the upgrade form to setup the source database.
    +    $this->drupalGet('/upgrade');
    +    $this->submitForm([], 'Continue');
    +    // Get valid credentials.
    +    $edits = $this->translatePostValues($this->getCredentials());
    +    $this->submitForm($edits, 'Review upgrade');
    +
    +    // Now, test with a source database connect and with message tables.

   If we do split it into two methods, then I think we should change $migration_ids into a class variable. The down side is that we will run createTables() a second time.

8. Nit: I think "Create message and map tables." is a little clearer.

    +    // Create data tables.
    +    $this->createTables($migration_ids);

9. Thanks for adding the @see comment (#63.4). I added #3198352: Make it easier to create migration tables for a single migration as a follow-up issue.

    +  /**
    +   * Creates d7_menu and d7_menu_test map and message tables for testing.
    +   *
    +   * @see \Drupal\migrate\Plugin\migrate\id_map\Sql::ensureTables
    +   */
    +  protected function createTables($migration_ids) {

@benjifisher, thanks. we are getting there....

1. Fixed. Sorry about that.
2. Good.
3. I am not sure what you want done for this. Does this need #attributes? Does it need both id and class, or just one of them?
4. Oops, fixed.
5. Fixed
6. No, not used. Removed setting of #title.
7. Sure, it is now two tests. It was easier and faster to develop the test with them together. I certainly prefer the faster part. :-) And the test had to be modified due to #3198339: Improve error reporting from migrate_drupal_migration_plugins_alter(). I added an @todo comment for that.
8. Fixed. Although I prefer 'map and message' to 'message and map
9. Your welcome. Although I think the followup is very low priority.

#78.3 On slack benjifisher suggested removing #attributes line from MigrateController. This patch removes that line.

@quietone: There are just a few changes left:

1. When I asked for consistent formatting in the source plugins, I was hoping for the blank line before the return statement as well as the single catch block.

2. Now that this test only handles the overview page, the "Overview page" comment is redundant.

    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateControllerMessageTest.php
    @@ -0,0 +1,424 @@
    ...
    +  public function testOverview() {
    +    $session = $this->assertSession();
    +
    +    // First, test with no source database or message tables.
    +    // Overview page.

3. Do you think this @todo comment is helpful?

    +    // Test overview with no source database connection and with message tables.
    +    // @todo Remove reference to Variable source plugin in
    +    // https://www.drupal.org/node/3198339
    +    $this->drupalGet('/admin/reports/upgrade-messages');

   Unless we add a comment to #3198339: Improve error reporting from migrate_drupal_migration_plugins_alter(), my guess is that whoever works on that issue will notice the failing test before the code comment.

As for #3198352: Make it easier to create migration tables for a single migration, I see at least one other test where it would be useful. To be honest, I think my eyes glazed over the first few times I looked at the createTables() method. When I added that issue, I finally realized that the table schema was a small part of that helper function.

@benjifisher

1. Ah, I was trying to limit the changes to the files. The new lines have been added, I hope.
2. Comments in the Overview test changed.
3. I wasn't sure either - it is removed.

@quietone, thank for your persistence. I think this is ready to go!

I am updating the issue summary, including the "Remaining tasks" section and moving the screenshots to the "User interface changes" section.

Starting in #42.6, we discussed whether the controller class should extend ControllerBase. Can we get some guidance from a core committer? Is this a problem? If so, should we deal with it now or defer it to a follow-up issue?

I think we are right to refer to "Upgrade messages" as in this issue. I will add a note to #2713327: Document ways to remove migration tables (ID map etc.) suggesting that it adopt similar UI text.

1. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
   @@ -67,13 +68,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 (RequirementsException | \PDOException $e) {
        }
   +
   

   Shouldn't this be caught there and not here? I.e. are we hiding an error? I guess you want the fields regardless - but this feels a bit dubious. Requiring this change to migration sources means that the form is likely to fail if contrib sources get involved.

2. +++ b/core/modules/migrate_drupal_ui/migrate_drupal_ui.routing.yml
   @@ -47,7 +47,27 @@ migrate_drupal_ui.upgrade_review:
   +  options:
   +    _admin_route: TRUE
   ...
   +  options:
   +    _admin_route: TRUE
   

   This is not necessary - it's only necessary when the route does not begin with /admin/ - like some of the routes here.

3. +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
   @@ -3,12 +3,42 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function create(ContainerInterface $container) {
   +    $controller = parent::create($container);
   +    $controller->database = $container->get('database');
   +    $controller->formBuilder = $container->get('form_builder');
   +    $controller->migrationPluginManager = $container->get('plugin.manager.migration');
   +    return $controller;
   +  }
   

   Let's add a constructor and do proper dependency injection. Controllers are internal and we can trigger deprecations for not passing these in. Are there extensions of this class in contrib?

4. +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
   @@ -23,4 +53,266 @@ public function showLog() {
   +      $message_count = $migration->getIdMap()->messageCount();
   +      // The message count is zero when there are no messages or when the
   +      // message table does not exist.
   +      if ($message_count == 0) {
   +        continue;
   +      }
   

   It's it worth it for the user to know that there are no messages for a particular migration?

5. +++ b/core/modules/migrate_drupal_ui/src/Form/MessageForm.php
   @@ -0,0 +1,115 @@
   +        $_SESSION['upgrade_messages_overview_filter'][$name] = [
   

   We shouldn't be using $_SESSION - we need to get the session from the request. The

6. +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
   @@ -23,4 +53,266 @@ public function showLog() {
   +    $build['message_filter_form'] = $this->formBuilder->getForm('Drupal\migrate_drupal_ui\Form\MessageForm');
   

   I think you might be able to avoid using session at all if you call buildForm here with your own instance of FormState. You should then be able to get the values from there... ah this is not going to work - the pager will break that. I guess that that is why DbLog is using session.

   FWIW it is easy to pass the session in from the request here. You can add \Symfony\Component\HttpFoundation\Request $requestto the method arguments and the argument resolver will provide it to you automatically.

   Hmmm... actually looking at the exposed filter code in views maybe we can avoid session completely and do:

       $form_state = (new FormState())
         ->setMethod('get')
         ->setAlwaysProcess()
         ->disableRedirect();
   

   I think this might make it work with the pager and retain any input.

Are we sure we want this debugging info for migrations to only be about running upgrades? What if I want to see logs from a non upgrade migration? For something besides a d2d migration? Shouldn't the functionality here go into migrate module instead? And then all this carefully worded "upgrade" stuff would need to be reworded to say migration. Because migrate.module provides an API and all things should be worded from that perspective. While migrate_drupal_ui.module provides a d2d upgrade implementation.

I was thinking about #87 when reviewing but forgot to ask - great question!

For #86.
1. That makes sense. There is now a try/catch around fields in the MigrateController. I do think the try/catch should still be in the source plugin so the fields can be listed with drush. Also, in testing I dropped the source database and then got a DatabaseNotFound exception. I added that to the list of expectation but I wonder if there are others that should be listed, or just use \Exception.
2. Fixed
3. Fixed. This is a new class, so no usages in contrib.
4. This was asked in #33. I think it is too much information, #34, and benjifisher agreed in #42 saying "I think you are right that showing all migrations is overwhelming and does not add anything.".
and
5. Nothing to do here because of 6?
6. Fixed. Paging seemed to work although not tested thoroughly.

#87. That makes a lot of sense. My concern is the user is doing an 'Upgrade' and then suddenly is looking at 'Migrate logs' and 'Migrate messages'. I think that adds confusion.

#86.1: @quietone first mentioned the problem in #40:

The menu source plugin is changed to add a check if the database is set or not to avoid a call on NULL.

If we leave the source plugins as is, then we will have to catch an error, not an exception. We can do that. (I only know because we discussed that on #3167267: MigrateExecutable should catch not only exceptions, but also fatal errors.) I think the current consensus is that we can catch errors but that we should not do so.

In #43.7 and replies, we agreed that we should either "fix" all the source plugins or none of them.

Yes, we are likely to get some problems from contrib modules.

#86.3: It was my idea to override create() and not define a constructor. I do not see what advantage there is to using a constructor.

#86.5, #86.6: If we can avoid using $_SESSION here, should we have a follow-up issue to make the same change in the dblog module? There may be another place in core that uses the same pattern.

Here it is: TranslateFilterForm in the locale module also uses $_SESSION to store form filter values.

Sorry, I did not mean to set the issue back to NW.

Coming back to finish my comment. I wanted to ask why the constructor is better but I see that benjifisher has made the same point.

1. Follow-up to #86.2:

    +++ b/core/modules/migrate_drupal_ui/migrate_drupal_ui.routing.yml
    @@ -47,8 +47,24 @@ migrate_drupal_ui.upgrade_review:
     migrate_drupal_ui.log:
       path: '/admin/reports/upgrade'
       defaults:
    -      _controller: '\Drupal\migrate_drupal_ui\Controller\MigrateController::showLog'
    +    _controller: '\Drupal\migrate_drupal_ui\Controller\MigrateController::showLog'
       requirements:
         _custom_access: '\Drupal\migrate_drupal_ui\MigrateAccessCheck::checkAccess'
       options:
         _admin_route: TRUE

   If fixing the indentation is in scope, then we can also remove the options. This is the only existing route in the file with a path that starts /admin/.

2. Re-reading @alexpott’s comment from #86.3:

   Let’s add a constructor and do proper dependency injection. Controllers are internal and we can trigger deprecations for not passing these in. Are there extensions of this class in contrib?

   As @quietone already pointed out (#89), this is a new class, so there are no extensions (yet). I am still trying to understand why we should use a constructor, and I am thinking of that second sentence. Maybe the point is that, in the future, this class might be extended in contrib, and then we might want to inject additional services. Since create() is declared in the interface, does that mean we cannot throw exceptions nor trigger (deprecation) errors in it? But we could from __construct()?

   These questions are only for my own understanding. The patch in #89 uses a constructor as requested in #86.

3. How did userStorage get in here?

    +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
    @@ -3,12 +3,65 @@
    ...
    +  public function __construct(Connection $database, FormBuilderInterface $form_builder, MigrationPluginManager $migration_plugin_manager) {
    +    $this->database = $database;
    +    $this->formBuilder = $form_builder;
    +    $this->migrationPluginManager = $migration_plugin_manager;
    +    $this->userStorage = $this->entityTypeManager()->getStorage('user');
    +  }

4. From the interdiff:

    @@ -234,7 +262,11 @@ public function details($migration_id) {
         }
   
         // Build the complete form.
    -    $build['message_filter_form'] = $this->formBuilder->getForm('Drupal\migrate_drupal_ui\Form\MessageForm');
    +    (new FormState())
    +      ->setMethod('get')
    +      ->setAlwaysProcess()
    +      ->disableRedirect();

   I am not sure how it is supposed to work, but I think you need both the form and the form state. When I test with the patch in #89, I do not see the form for filtering the messages. I think the point of #86.6 was to find a way to save the parameters (when paging, for example) without using $_SESSION, so I think that #86.5 is still something we want to fix.

   As far as I am concerned, it is OK to keep using $_SESSION for now, then have another issue to get rid of it here, in dblog, and in locale.

5. Re #87: Let’s see if we can come up with labels that make sense for both D-to-D upgrades and more generic migrations.

#94
1. Removed the indentation fix, that would have been the results of reformatting the whole file, not just the changes.
3. Removed the userStorage.

Judging by the interdiff, #96 does what the comment says. I am satisfied with reverting the indentation fix (#94.1).

Still NW for #86.5-6. See the follow-up in #94.4.

We discussed #87 in the weekly migration meeting today. I think we have some good ideas:

If we want to keep this UI after migrate_drupal is deprecated then using migrate language is the way to go. Maybe on the page have some explanation for non-devs that this is the part of the 'logs' from /upgrade?

I think that is a good idea. Even better if that explanation is added by migrate_drupal or migrate_drupal_ui, so that it only appears when that/those modules is/are enabled. I think I prefer migrate_drupal_ui.

We could have two links [in the admin menu] going to the same page. The migrate_drupal_ui module could add one, as it is now. Then the migrate module could add a link in a different part of the menu tree.

The migrate_tools module adds menu items under Structure in the admin menu. Migrations are grouped by migration group, and there are several pages (edit, delete, ...) for each migration. Adding a link to /admin/reports/upgrade-messages has some problems, but it could be where people expect to find it. I think it is better than having two different links to the same page under Reports.

Problems: it duplicates what migrate_tools provides; it breaks the usual relationship between paths and menus; we probably want to change upgrade-messages to migrate-messages or migration-messages. The last problem is easy to fix except for choosing which option to use.

@benjifisher, I don't know what to do for #86.5, .6 and 94.4. Do you?

This patch is the first go at moving the forms to the Migrate module. I haven't checked everything or done a decent self review but at least the forms are working. As discussed in a meeting and in #97, there are two menu items pointing to the same form and from migrate_drupal_ui some extra text is added to help the non-developer. That text definitely needs work!

A picture is worth a 1000 words so I have made some screenshots.

The reports page

The message overview form from "Upgrade messages"

The message overview form from "Migrate messages"

The failing test is a copy/paste error, the file core/modules/migrate/tests/src/Functional/MigrateMessageControllerTest.php needs to be deleted.

From #98:

@benjifisher, I don't know what to do for #86.5, .6 and 94.4. Do you?

I already suggested this in #94.4:

As far as I am concerned, it is OK to keep using $_SESSION for now, then have another issue to get rid of it here, in dblog, and in locale.

In other words, decide that #85.5 is out of scope for this issue. We are following a pattern (using $_SESSION) that is already used in two other places in core. Even if we find a better way to do it in this issue, the two existing uses will still need to be fixed. We might as well fix three cases of the pattern in a follow-up issue as two cases.

NW for the failing test.

Right, follow up made, #3216840: Remove use of $_SESSION for filter values

The test that was failing was plain wrong and not needed and has been removed. Tweaked the two Controller tests and changed the title of the message page when accessed from migrate_drupal_ui.

Too late to do a self review, maybe tomorrow.

silly me. Uploaded the patch from #98 and not the new on. Here is the patch for #103. The interdiff in #103 is correct.

Fix the assertion in the failing test.

Noting another issue that is adding a menu item for migrate, #2713327: Document ways to remove migration tables (ID map etc.).

@quietone, thanks for resuming work on this issue. It has been a while since I looked at it closely. Because of that and also since most of the new code is in the migrate module instead of migrate_drupal_ui, I am taking a fresh look.

1. I do not think we need a dedicated class for this access check:

    +++ b/core/modules/migrate/src/MigrateAccessCheck.php
    @@ -0,0 +1,35 @@
    +<?php
    +
    +namespace Drupal\migrate;
    +
    +use Drupal\Core\Access\AccessResultAllowed;
    +use Drupal\Core\Session\AccountInterface;
    +
    +/**
    + * Checks access for migrate routes.
    + *
    + * The Migrate Drupal UI can only be used by user 1. This is because any other
    + * user might have different permissions on the source and target site.
    + *
    + * This class is designed to be used with '_custom_access' route requirement.
    + *
    + * @see \Drupal\Core\Access\CustomAccessCheck
    + */
    +class MigrateAccessCheck {

   When I write a custom access check, I usually re-read Custom route access checking and then define checkAccess() in my controller class. That seems to be common in the core modules: I searched core/modules/*/*.routing.yml for _custom_access, and found that most uses (outside migrate_drupal_ui) referenced Controller or Form classes.

2. Same snippet: the comment about user 1 is not valid in this context. This is how a "simple" suggestion like #87 ("Shouldn’t the functionality here go into migrate module instead?") can lead to a ton of technical details!

   I am open to other suggestions, but I think the right thing to do is introduce a new permission for these message pages.

   For example, one site may be migrating users from another source. The messages might contain private information. Another site might be migrating blog posts from an external feed. Those messages might be much less sensitive. (Or they might expose trade secrets.) I think we have to let the administrator decide who gets the permission.

   If we make access permission-based, then we do not need the custom access checker at all.

   On the other hand, when migrate_drupal_ui is enabled, it might want to enforce its access checker on the message pages. We have to worry that a compromise of the user data could lead to access to the site. On the other other hand, that is already a concern, and migrate_drupal_ui might be enabled for less sensitive, content-only data import.

3. This comment is not valid in the new context, either.

    +  public function checkAccess(AccountInterface $account) {
    +    // The access result is uncacheable because it is just limiting access to
    +    // the migrate UI which is not worth caching.
    +    return AccessResultAllowed::allowedIf((int) $account->id() === 1)->mergeCacheMaxAge(0);
    +  }

4. Do we have to include system? I just tried removing it, and the test passed (2 tests, 11 assertions).

    +++ b/core/modules/migrate/tests/src/Functional/MigrateMessageControllerTest.php
    @@ -0,0 +1,315 @@
    ...
    +  protected static $modules = [
    +    'message_test',
    +    'migrate',
    +    'system',
    +  ];

That is all for now. I will try to continue the review tomorrow.

#109.
1-3. Removed the custom access check and instead this now uses a permission, 'view migrate messages'.
4. system removed. It must be installed by the profile.

Queuing up a test against 9.4 to see where this is before I go to the effort of testing it in a real environment.

Fixing spelling errors.

1. Is the "No message tables" screenshot in the issue summary still accurate? If so, then how hard would it be to avoid showing the table header?

2. The page title is "Migration messages", not "Migrate messages". Can we make the permission title, which shows up in the admin UI, consistent? I think it is OK to leave the machine name as is, or you can update it too if you want.

    +++ b/core/modules/migrate/migrate.permissions.yml
    @@ -0,0 +1,2 @@
    +view migrate messages:
    +  title: 'View migrate messages'

3. I think only the overview() method needs to override the parent class. The two properties, the constructor, and the create() method look identical.

    +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateMessageController.php
    ...
    +class MigrateMessageController extends BaseMessageController {
    +
    +  /**
    +   * The database connection.
    +   *
    +   * @var \Drupal\Core\Database\Connection
    +   */
    +  protected $database;
    ...

4. Can we come up with a more descriptive variable name?

    ...
    +  public function overview() {
    +    $build = parent::overview();
    +
    +    $tmp['help'] = [
    +      '#type' => 'item',
    +      '#markup' => 'The upgrade process may log messages about steps that require user action or errors. This page allows you to view these messages',
    +    ];
    +    $tmp['info'] = [
    +      '#type' => 'item',
    +      '#markup' => Link::fromTextAndUrl('Review the detailed upgrade log.', Url::fromRoute('migrate_drupal_ui.log'))
    +        ->toString(),
    +    ];
    +    return $tmp + $build;

5. In the same method, I think we should override the table headers.

6. We should update the issue summary to explain that we are creating similar pages at /admin/reports/migration-messages and /admin/reports/upgrade-messages. It is a little odd that both of these summary pages link to the same detail pages.

Added new screenshot to the IS.

#115.
1. The table of messages is built with,

    $build['message_table'] = [
      '#type' => 'table',
      '#header' => $header,
      '#rows' => $rows,
      '#empty' => $this->t('No messages for this migration.'),

which makes me think this is the expected behaviour. For example, when viewing /admin/content with no content, the headers still appear.
2. Changed to 'migration messages'.
3. Yes, that is correct and it is changed.
4. Variable name is now 'description'.
5. Override the headers with what?
6. This patch adds a route subscriber in migrate_drupal_ui to alter the _controller in the migrate.message route so the migrate_drupal_ui controller is used. There is now only one route and one link.

I also did some renaming of 'upgrade' to 'migration'.

I did a little manual testing. I enabled the Migrate module and visited /admin/reports/migration-messages: I see just the message

There are no migration message tables.

and no table header, as I requested in #115.1. Thanks! I am replacing the screenshot in the issue summary with a new one.

I tested a simple custom migration, using test_user_role from #2953111-133: Only migrate role permissions that exist on the destination. Since that issue has not been fixed yet, the test migration generates two error messages. I tested filtering on the message and/or the severity level, and it all works as expected. I sorted on the name and message columns. Since both messages have the same severity, sorting on that column was inconclusive.

I will have a look at the code now.

Following up on the points in #115:

1. I updated the screenshot in #117.
2. Fixed, thanks.
3. Fixed, thanks.
4. Fixed, thanks.
5. No change, but maybe that is just as well, given the next point.
6. Now migrate_drupal_ui overrides the page controller instead of adding a very similar page. Good, I think this has less potential for confusion.

This is looking good, but I see a few problems that are either new or I missed them in earlier reviews.

1. This text should be translatable. Just add $this->t().

    +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateMessageController.php
    @@ -0,0 +1,36 @@
    ...
    +    $description['help'] = [
    +      '#type' => 'item',
    +      '#markup' => 'The upgrade process may log messages about steps that require user action or errors. This page allows you to view these messages',
    +    ];
    +    $description['info'] = [
    +      '#type' => 'item',
    +      '#markup' => Link::fromTextAndUrl('Review the detailed upgrade log.', Url::fromRoute('migrate_drupal_ui.log'))
    +        ->toString(),
    +    ];

2. The UI text in the base class needs to be translatable, too.

    +++ b/core/modules/migrate/src/Controller/MigrateMessageController.php
    @@ -0,0 +1,325 @@
    ...
    +      $build['no_tables'] = [
    +        '#type' => 'item',
    +        '#markup' => "There are no migration message tables.",
    +      ];

3. Thanks to the way PHP objects work, you do not need to add the route back to the collection.

   +++ b/core/modules/migrate_drupal_ui/src/Routing/MigrateDrupalUiRouteSubscriber.php
   ...
   +    $route = $collection->get('migrate.messages');
   +    if ($route) {
   +      $route->setDefault('_controller', '\Drupal\migrate_drupal_ui\Controller\MigrateMessageController::overview');
   +      $collection->add('migrate.messages', $route);
   +    }
   

4. Looking at the interdiff, I noticed these extra blank lines:

    +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateMessageControllerTest.php
    @@ -0,0 +1,563 @@
    +<?php
    +
    +namespace Drupal\Tests\migrate_drupal_ui\Functional;
    +
    +use Drupal\Core\Database\Database;
    +use Drupal\migrate\Plugin\MigrateIdMapInterface;
    +
    +
    +use Drupal\Tests\BrowserTestBase;
   

@benjifisher,

Made changes as per comment #118.

Needs review.

@Ratan Priya, thanks for making the changes.

Looks like random failure. Retesting and adding a test run for 9.5.x.

I checked the patch in #120, and it makes the changes I requested in #118.

@Ratan Priya, it looks as though this is your first contribution on d.o. Thanks, and I hope there are more to come!

I did some manual testing in #117, and a complete review in #118. Now that those points have been fixed, I think this issue is ready. @quietone, thanks for all the work on this!

The test on 10.0.x passed.

The test on 9.5.x had a single failure: a deprecation notice from an unrelated test. I do not see that locally, although I do see some deprecation notices when I test on the (patched or unpatched) 10.0.x branch.

Manual test of the patch https://www.drupal.org/files/issues/2022-08-05/3063856-120.patch:

Using core install of 9.4.5, Confirmed no page found for admin/reports/migration-messages
Added patch with same results on local dev - got the same result.
Rebuilt local dev instance and reinstalled
Migration Messages listed in admin/reports reporting no results
Ran a sample migration with errors see MIgrationMessagesAfterMigraion.png
Viewed Filtered detail listing, see FIlteredPublicFilesErrors.png

@bsnodgrass:

Thanks for the test results. I would have installed a clean D6 or D7 site and added some content with Devel Generate. Using the database from a real, crufty site is a much better test!

In the Slack discussion that we started in the Migration meeting on 2022-08-04, you mentioned that you did not find any problems with sorting nor filtering. I take that as +1 for RTBC.

+++ 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) {
     }
+

+++ 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) {
+    }

The changes to MenuLink::fields() concern me and I'm not even sure if they are necessary given the code in MigrateMessageController. Do we really need to change the source plugins here?

To me some digging to remember why the try/catch is used.

The reason the try/catch is added to the field methods is because we need to be able to return the fields array. With the try/catch the source plugin can catch the exception thrown by \Drupal\migrate\Plugin\migrate\source\SqlBase::setUpDatabase and continue on and return the field array. If the exception is not caught by the source plugin the fields array is not available to the MigrateMessageController and there are no headings for the display.

Hope that helps!

I investigated the question raised in #128. I think the reply in #129 is correct: we want to get as many fields from the plugins as we can, so we need the try/catch in the source plugins. There may be source plugins from contrib modules that throw exceptions, so we need the try/catch in the page controller as well.

However, I think I found a more serious problem.

First, a minor point:

    +++ b/core/modules/system/src/Plugin/migrate/source/Menu.php
    @@ -36,12 +38,21 @@ public function fields() {
    ...
    -    if ($this->database->schema()->fieldExists('menu_custom', 'language')) {
    -      $fields += [
    -        'language' => $this->t('Menu language.'),
    -        'i8n_mode' => $this->t('Menu i18n mode.'),
    -      ];
    +    // 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) {
    +    }

Besides putting the database access inside a try/catch block, this hunk replaces $this->database->schema() with $this->getDatabase()->schema(). This might be considered an unrelated bug fix, since it prevents a WSOD with this error message when the database connection is not available:

Error: Call to a member function schema() on null in Drupal\system\Plugin\migrate\source\Menu->fields() (line 42 of core/modules/system/src/Plugin/migrate/source/Menu.php).

My main point is that there are bigger problems with this issue when we have migrated from Drupal 6/7 and the database connection is not available.

These were my testing steps:

1. Install Drupal 7 with the devel_generate module (part of the devel project, at least for D7).
2. Generate some sample content: vocabularies, taxonomy terms, users, menu links, and nodes.
3. Install Drupal 10, with the patch from #120, and the migrate_drupal_ui module.
4. Migrate from the D7 site, starting at /upgrade on the D10 site.
5. Review the migration messages using the pages added in this issue.
6. Shut down the database server for the D7 site.
7. Repeat Step 5.

I cleared caches frequently.

@quietone, shutting down the database server is the sort of situation we are trying to handle gracefully, isn’t it?

I still get a WSOD from an uncaught PDOException from these lines:

    +++ b/core/modules/migrate/src/Controller/MigrateMessageController.php
    @@ -0,0 +1,325 @@
    ...
    +  public function overview() {
    +    // Check if there are migrate_message tables.
    ...
    +    // There are migrate_message tables so build the overview form.
    +    $migrations = $this->migrationPluginManager->createInstances([]);
    ...
    +  }
    +
    ...
    +  public function details($migration_id) {
    +    /** @var \Drupal\migrate\Plugin\MigrationInterface $migration */
    +    $migration = $this->migrationPluginManager->createInstance($migration_id);

The error message is

PDOException: SQLSTATE[HY000] [2002] php_network_getaddresses: getaddrinfo for ddev-drupal7-db failed: Temporary failure in name resolution in Drupal\migrate\Plugin\migrate\source\SqlBase->setUpDatabase() (line 203 of core/modules/migrate/src/Plugin/migrate/source/SqlBase.php).

Is there an easy way out of this? The only things I can think of involve serious restructuring of the system.

Yes, that is an awful discovery.

I migrated the d7 fixture from a separate ddev site, then checked that there was data on the message page and the details page. I then stopped the d7 site. On reload on the message page I got the WSOD. That confirms what benjifisher discovered.

I then walked through the code and eventually realized that \Drupal\migrate\Plugin\migrate\source\SqlBase::setUpDatabase only catches ConnectionNotDefinedException. If that is changed to also catch PDOExceptions the WSOD stops.

Now, on the Message page there is the following db error message. The details page does not display an error.

Error message
Failed to connect to your database server. The server reports the following message: No database connection available for source plugin variable.

    Is the database server running?
    Does the database exist, and have you entered the correct database name?
    Have you entered the correct username and password?
    Have you entered the correct database hostname?

I then walked through the code ...

Sorry, I could have saved you some trouble by quoting a stack trace in #130.

I think the error message in #131 is a separate problem, out of scope for this issue. Did you clear caches before checking the details page? When I have time to review the patch in #131, I will check that.

No worries, walking through it was useful to me. I did clear caches, frequently. Also, I think there is another issue about changing SQLBase, in a similar way. I should have time today to search for it.

@quietone and I agreed to postpone this issue on #3312733: SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled.

Adding postponed item to the list of remaining tasks per the issue summary field documentation, remaining task.

I am working on #3312733: SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled, and it is helpful to use this issue for testing.

This issue needs a reroll after #3181778: [w/c September 17th] Replace t() with $this->t() in all plugins. Since the changes made in #131 will be handled in #3312733, I re-rolled the patch in #120. The attached patch applies cleanly to 10.0.x and 10.1.x.

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

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

P.S. It seems that changing the status to NR is enough to trigger a 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 #3312733: SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled.

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 #3312733: SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled

We can un-postpone this issue now that #3312733: SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled 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 #3312733: SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled.)

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?

Edit don't know what I was looking at please ignore.