Malformed migration_dependencies breaks all migrations

Ran into approximately this issue (actually a missing space between a dash and the name meant it was a string instead of an array, but close enough)... Adjusted to report an error like so (when run through drush):

drush8 migrate-status ifa_nimble_contact_primary
Drupal\migrate\MigrateException: Invalid migration dependencies configuration for migration ifa_nimble_supplier in                                           [error]
/mnt/c/taoti/ifa-d8/web/core/modules/migrate/src/Plugin/Migration.php:618
Stack trace:
#0 /mnt/c/taoti/ifa-d8/web/core/modules/migrate/src/Plugin/MigrationPluginManager.php(122): Drupal\migrate\Plugin\Migration->getMigrationDependencies()
#1 /mnt/c/taoti/ifa-d8/web/profiles/taoti/modules/contrib/migrate_tools/migrate_tools.drush.inc(518):
Drupal\migrate\Plugin\MigrationPluginManager->createInstances(Array)
#2 /mnt/c/taoti/ifa-d8/web/profiles/taoti/modules/contrib/migrate_tools/migrate_tools.drush.inc(154):
drush_migrate_tools_migration_list('ifa_nimble_cont...')
#3 /mnt/c/taoti/ifa-d8/vendor/drush/drush/includes/command.inc(422): drush_migrate_tools_migrate_status('ifa_nimble_cont...')
#4 /mnt/c/taoti/ifa-d8/vendor/drush/drush/includes/command.inc(231): _drush_invoke_hooks(Array, Array)
#5 /mnt/c/taoti/ifa-d8/vendor/drush/drush/includes/command.inc(199): drush_command('ifa_nimble_cont...')
#6 /mnt/c/taoti/ifa-d8/vendor/drush/drush/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#7 /mnt/c/taoti/ifa-d8/vendor/drush/drush/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#8 /mnt/c/taoti/ifa-d8/vendor/drush/drush/drush.php(12): drush_main()
#9 {main}

I suppose the question would be if it needs a test?

These changes seem reasonable. A couple small points and this should be ready. Tagging novice. We could also use tests. It is debatable if test part is novice, but the rest is.

1. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -22,7 +22,7 @@ class Migration extends PluginBase implements MigrationInterface, RequirementsIn
   +   * @var string620
   

   This seems random?

2. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -615,6 +615,9 @@ public function setTrackLastImported($track_last_imported) {
   +      throw new MigrateException("Invalid migration dependencies configuration for migration {$this->id()}");
   

   Let's throw an InvalidPluginDefinitionException instead?

Only for contribute to discussion of this enhancement.

+++ b/core/modules/migrate/src/Plugin/Migration.php
@@ -22,7 +22,7 @@ class Migration extends PluginBase implements MigrationInterface, RequirementsIn
+   * @var string620

Whats mean string620? In all code I don't found other part with this code.

@@ -615,6 +615,9 @@ public function setTrackLastImported($track_last_imported) {
+      throw new MigrateException("Invalid migration dependencies configuration for migration {$this->id()}");

IMO it is right because this is not a plugin exception and this exceptions is used for Migrate proccess.

Thanks for the patch @NickWilde! #4 works great for me.

I presume the string620 might have been an accident. I've just updated it to throw an InvalidPluginDefinitionException but am happy to change it back to a MigrateException? I also fixed the !is_array check to check required as well as optional.

I have given the test a go... I've never contributed a test before so I'm not sure if I've approached it correctly, I've tried to get inspiration from migrate and other core modules, so any feedback would be appreciated!

Oops.. Will fix deprecation tomorrow!

Here's an updated version with deprecation fix. I was testing on an older version of core stupidly! Also I'm getting:

1) Drupal\Tests\migrate\Kernel\process\DownloadTest::testOverwritingDownload
mkdir(): No such file or directory
(separate issue)

Locally, but this is with and without the attached patch...

Thanks! Setting status to NR - to trigger the test bot.

(and yes that 620 was just random characters that somehow got in there... someone (me) didn't properly review the actual patch file before posting apparently)

Best to include a patch only thing that fails... test-only copy of latest patch.
Real patch is just a re-upload of @littlepixiez' latest patch.

Debatable, but let's add a CR explaining that an exception is now thrown if the format of migration_dependencies is wrong.

And I think we need to also support the following scenarios without throwing exceptions:
non-existent migration_dependencies
empty array of migration_dependencies = []
and single array entries migration_dependencies = ['required' => 'd6_node']

Thanks for the feedback guys!

This line:
$this->migration_dependencies = ($this->migration_dependencies ?: []) + ['required' => [], 'optional' => []];
seems to ensure we won't get an exception thrown for any of those scenarios and it will still return the correct array even if it's non-existent, empty or provides only required/optional (but required/optional still need to be provided as an array).

I've added another test to cover these scenarios to ensure this isn't broken, if that's helpful.

@NickWilde presumably putting both up means you are making sure the test fails before, and succeeds after?

Thanks for your patience! :) Also not entirely sure about how to add a change record?

+++ b/core/modules/migrate/tests/src/Unit/MigrationTest.php
@@ -112,6 +113,74 @@ public function testRequirementsForMigrations() {
+    $this->expectExceptionMessage("Invalid migration dependencies configuration for migration {$migration->id()}");

$migration->id() returns NULL.

+++ b/core/modules/migrate/src/Plugin/Migration.php
@@ -615,6 +616,9 @@ public function setTrackLastImported($track_last_imported) {
+      throw new InvalidPluginDefinitionException($this->id(), "Invalid migration dependencies configuration for migration {$this->id()}");

$this->id() returns NULL

Perhaps get('id') is needed in both cases/

@littlepixiez, thanks for adding extra tests. Yes, the test only patch proves the failure.

There is a handbook page for how to make a change record,
Contributor task: Write up a change record for a Drupal core issue

Hi @quietone. Thanks for the feedback! Oh yes, you're totally right. Considering $this->id() seems to return $this->pluginId, which is set in the constructor originally and has no override method, I've popped a method on the TestMigration class to set the plugin ID, and then tested that the message matches the one that's been set in the test function. When a real migration has invalid configuration set, $this->id() returns the migration machine name as expected too.

Thanks for the handbook page reference, how exciting, I'll have a gander. :)

Code changes here are looking good. Just missing the CR at this point.

Hey @heddn. Great! I attempted creating a change record. I wasn't sure whether to explain that before this, it would throw a general error, and that after this it would throw an exception. I also wasn't sure whether to give an example of an incorrectly formatted migration_dependencies (as the most common mistake is forgetting to use optional or required as the key).

Thank you! https://www.drupal.org/node/3077671

Hope to do get to this on the weekend.

@littlepixiez, thanks for the CR, it does contain the necessary information. Providing examples of all the valid format and stating the required action is great and well covered, I thing. I do think that using an 'if' statement in the Description can be avoided by just stating the case convered here. I don't know maybe, 'Formatting errors in the migration_dependencies property of a migration ....'.

Oh, and should it state the previous behavior when it failed?

Hi @quietone, so sorry for my really late reply! I've updated the CR now to be a bit more concise. Hopefully I've worded it in a way that makes sense. Thanks so much for your feedback. :) The only thing I'm unsure of is that it wouldn't always necessarily throw a TypeError if they are incorrectly formatted depending on what you actually format incorrectly. So I'm not sure whether specifying the error is appropriate?

Looking good.

Uploading #16 again so it is last.

Committed 3cfe0f0 and pushed to 8.8.x. Thanks!

Fixing credit.

+++ b/core/modules/migrate/tests/src/Unit/MigrationTest.php
@@ -112,6 +113,75 @@ public function testRequirementsForMigrations() {
+   *
+   * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException

Is this accurate?

Missed that this had been committed to d8 - I think this is the correct status

The patch in #25 applies pretty cleanly against 8.7.x as-is.

While I appreciate that it now throws an appropriate error instead of just barfing all over my terminal, IMHO it would have been nicer if each item had allowed NULL values too, especially for beginners (or experienced folks who forget). I'll probably open a new issue for this request.

#25 rerolled for 8.7, because testbot.

8.7 is security only now

What is the reason for adding count($this->migration_dependencies) !== 2 to the condition statement?

ignore this comment, please.

Only removing the count($this->migration_dependencies) !== 2 in this patch.