Use a custom error message for PluginNotFoundException in the migratelookup service

Instead of

      $migrations = print_r($migration_ids, TRUE);
      throw new PluginNotFoundException($migration_ids, sprintf("Plugin ID '%s' was not found.", $migrations));

how about something like this?

      $migrations = implode(', ', (array) $migration_ids);
      throw new PluginNotFoundException($migration_ids, sprintf("Plugin IDs '%s' were not found.", $migrations));

Variant:

      $message = is_array($migration_ids)
        ? "Plugin IDs '" . implode(', ', $migration_ids) . "' were not found."
        : '';
      throw new PluginNotFoundException($migration_ids, $message);

We might even use "', '" as the first argument to implode(), leading to an error message like

Plugin IDs 'foo', 'bar' were not found.

Should we consider making that change in the PluginNotFoundException constructor? How common is this pattern of looking up one or several plugins by ID?

I searched the core migration modules for "PluginNotFoundException" trying to answer that question. It is too close to my bed time for me to create any new issues, but here are some notes:

1. We have kernel and unit tests for exceptions thrown by MigrateLookup::lookup() and MigrateStub::createStub() that are almost identical. Can we remove the kernel tests?
2. MigrateFieldPluginManager::getPluginIdFromFieldType() copies the doc block from its interface, with a few extra lines, instead of using {@inheritdoc} and regular comments.

I did not find any other examples of the one-or-several (string or string[]) pattern, so maybe changing the PluginNotFoundException constructor is not a great idea.

I created #3187794: Remove redundant kernel tests in the Migrate module and supplied a patch. I was going to add an issue for #5.2, but first I checked when the doc block was added. It was discussed in #3004456-25: Add a weighting option to MigrateField plugin's definition and lower weight of deprecated 'date' plugin so it isn't used and the following comment. I guess that implementation details should not be added to the interface, and moving some lines from the doc block to regular comments (including one @see comment) would mean that they no longer show up in the API docs.

Back to this issue!

The testbot approves, the code looks good, and the exception messages are a lot easier to read than in the original patch. RTBC

Needs a reroll.

Sorry, I added that line in #3187794: Remove redundant kernel tests in the Migrate module. The earlier patches on this issue already asserted the exception message supplied by the data provider. The reroll removes the assertion of a static exception message.

Back to RTBC.

+++ b/core/modules/migrate/tests/src/Unit/MigrateLookupTest.php
@@ -48,14 +48,36 @@ public function testLookup() {
+    return [
+      'string' => [
+        'bad_plugin',
+        "Plugin ID 'bad_plugin' was not found.",
+      ],
+      'array one item' => [
+        ['bad_plugin'],
+        "Plugin IDs 'bad_plugin' were not found.",
+      ],

Should we do:

 is_array($migration_ids) && count($migration_ids) > 1

when determining the message, so that the array of one item gets treated as singular?

This is still problematic. The constructor for PluginNotFoundException ignores the $plugin_id parameter if a non-empty message is given, so the patches here prevent an error. But the @param comment specifies that $plugin_id is a string. Sooner or later, we will enforce that with a type declaration. The patch in #12 replaces a one-element array with a string, but still passes an array when there are multiple plugin IDs.

Also, $migration_ids[0] will not work if $migration_ids = ['a' => 'my_migration']. It is safer to use reset().

I was going to say that we could throw one exception for each migration ID on the list, but that does not work.

We could create a chain of exceptions:

    if (!$migrations) {
      $exception = NULL;
      foreach ((array) $migration_ids as $migration_id) {
        $exception = new PluginNotFoundException($migration_id, '', 0, $exception);
      }
      throw $exception ?? new PluginException('No plugin IDs were provided.');
    }

I cannot think of a more specific exception to throw when $migration_ids == [].

The alternative is to come up with a single string to pass to the PluginNotFoundException constructor as $plugin_id
as well as a string to pass as $message.

On second thought, I think the best solution, respecting the semantics of PluginNotFoundException, is to throw the more generic PluginException whenever $migration_ids is an array and count($migration_ids) != 1. When we throw PluginException, we just have to come up with a message; we do not have to make up a single plugin ID.

This looks like it will break in the case of an empty array. We certainly want test coverage for that case.

Can we add test coverage for the case of an empty array?

Added tests for an empty array.

@quietone: I meant to mention that I like the way you implemented the logic in #14. It is very clear and efficient.

@anmolgoyal74: Thanks for adding the test!

I am sorry I did not notice this in my earlier review:

+++ b/core/modules/migrate/tests/src/Unit/MigrateLookupTest.php
@@ -47,15 +48,63 @@ public function testLookup() {
...
+  public function testExceptionOnMultipleMigrationsNotFound($migrations, $message) {
...
+  /**
+   * Provides data for testExceptionOnMigrationsNotFound.
+   */
+  public function providerExceptionOnMultipleMigrationsNotFound() {

The method name in the comment does not match the name of the test method. It is a small thing, but we may as well fix it.

While we are at it, we should also thank @catch for making us take another look at this issue.

The last nit-pick is resolved ... the last nit is picked? ... so back to RTBC.

+++ b/core/modules/migrate/src/MigrateLookup.php
@@ -35,6 +36,12 @@ public function lookup($migration_id, array $source_id_values) {
+          throw new PluginException("Plugin IDs '" . implode(', ', $migration_id) . "' were not found.");

+++ b/core/modules/migrate/tests/src/Unit/MigrateLookupTest.php
@@ -47,15 +48,63 @@ public function testLookup() {
+        "Plugin IDs 'foo, bar' were not found.",

I think this should
Plugin IDs 'foo', 'bar' were not found.

We can use "', '" as the glue.

The reason being that usual quotes are used to quote a single thing - not multiple things.

The patch in #21 makes the improvement requested in #20. Back to RTBC.

Committed and pushed c0904f72e1 to 9.2.x and dd85517943 to 9.1.x. Thanks!