Throw exception for destination plugins without a destination_module property

After spending some time creating patch by pulling code from #2908282-53: Throw exception for source plugins without a source_module property, I realized that it's not the only thing needed for this patch.
We need to rebuild the patch keeping the fact in mind that 'destination_module' property is only valid for migration into Drupal destinations. See #2908282-64: Throw exception for source plugins without a source_module property.

Adding links to related comments from #2908282: Throw exception for source plugins without a source_module property in Issue Summary.

@catch, @effulgentsia, @Cottser, @lauriii and I discussed this and decided that in this case it's probably actually major. This is an important issue for making Migrate more developer- and user-friendly, but we wouldn't block marking Migrate stable on it. (We allow throwing new exceptions in minor releases especially if the code would break later anyway.)

If its not critical, it cannot be Migrate critical either. I believe we decided to harmonize these in Vienna last autumn.

Let's get this kicked off. I'm sure there will be several failures.

I think we are going to have to override the destination plugin manager similar to what we did for the base migration plugin manger and provide our own createInstance special snowflake that calls getDestinationModule during the constructor. It is a little later than discovery and processDefinition(), but at least its still pretty early in the bootstrap.

Here's a quick test to see if moving this down into createInstance() even makes sense. No interdiff, as I'm still playing around.

I have just added the interdiff, If anyone wants to look in b/w

Here's a more real patch. Let's see how many errors we have.

We should now load all destination modules and test they have a destination module, similar to how we are doing that for source_module. I think, given the nature of how we are gathering things, all modules /will/ have a destination module. But we should test for that.

Tried adding a test, Please review and let me know, is this the right direction?

Take a look at #2937045: source plugin source_module testing seems incomplete and that's what we'll want to do, but for destinations. But since that one is already RTBC let's just postpone this on it.

Hit the wrong status on that last save. Resetting status.

Apologies #23 was totally wrong patch

Discussed this on migration Maintainers meeting, assigning myself writing the test

I have added the test according to the direction of #25

Looks like its going to be some code duplication. so made some changes according to that.

Do we need to add testdestinationProvider like testSourceProvider?

Thanks Adam :)

I just reviewed the comments and found 1 main issues, that is the comments state that the source or destination module property must be on the annotation but isn't it really a requirement of the plugin definition?

1. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -26,20 +26,36 @@ public function testSourceProvider() {
   +    // All source plugins must define a source_module annotation property.
   

   The source_module property can be defined in the migration yml as well.

2. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -26,20 +26,36 @@ public function testSourceProvider() {
   +    // All destinations must define a destination_module annotation property.
   

   Same here, it can be defined in the migration yml.

3. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -26,20 +26,36 @@ public function testSourceProvider() {
   +  ¶
   

   trailing whitespace

4. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -26,20 +26,36 @@ public function testSourceProvider() {
   +  ¶
   

   trailing whitespace

5. +++ b/core/modules/migrate_drupal/config/install/migrate_drupal.settings.yml
   @@ -3,3 +3,9 @@
   +# Migrations with any of these tags will raise an exception if their
   

   Is it raise or throw for an exception? Just using dreditor but the 80 column wrapping looks wrong.

6. +++ b/core/modules/migrate_drupal/src/MigrationPluginManager.php
   @@ -7,21 +7,19 @@
   + * their respective plugin annotations. This is done in order to support the
   

   Hmm, 'plugin annotations' again. Are the definitions in the migration yml considered annotations?

7. +++ b/core/modules/migrate_drupal/src/MigrationPluginManager.php
   @@ -87,21 +116,42 @@ public function processDefinition(&$definition, $plugin_id) {
   +      // Throw an exception if the destination plugin definition does not define
   +      // a destination_module.
   

   Here there is no mention of 'annotation'. It just referring to the plugin definition which is better and accurate.

Just a reroll but includes fix whitespace errors

Add destination_module to the color destination plugin.

Tests were added in #33

Add destination_module to MigrateEntityComplete.