Problem/Motivation

We moved all the source plugins into their relevant modules within core but now they all have a hidden dependency on migrate_drupal because the source classes extend DrupalSqlBase. This is exposed when you use the migrate source plugin manager to discover the plugins but then get a fatal error when instantiating a plugin because of the missing DrupalSqlBase class.

Proposed resolution

  1. Add an annotated class discovery variant which automatically discovers provider dependencies based on namespace (of its class and every class it extends). Which is different from core that just looks at the namespace of the class and not its parents.
  2. Use this in the new MigrationSourcePluginManager.
  3. Move the provider filter logic from findDefinitions into a decorator and while at it, expand it into multiple provider support.
  4. Add a migrate specific decorator which removes plugins which try to use non-existing source plugins. Since we automatically discover the migrate_drupal provider dependency of the source plugins as they extend the DrupalSqlBase class this fixes the original issue.. almost.
  5. Add the provider list to the few migrations which are not automatically covered because they use a source from migrate: empty and embedded_data. Currently, there are only three, all three using embedded_data. Now we are really done with the original issue.
  6. While it could be called a different issue, we are also fixing when a non-migrate_drupal source uses a migrate_drupal deriver by using the new ProviderFilterDecorator -- in fact, this is the reason it exists: DerivativeDiscoveryDecorator::getDeriverClass() will get cranky and throw an exception if you try to call a nonexisting deriver. Could be a follow up but since it's already done, why not. Process plugins are definitely a followup because they need the definition normalizer to happen on discovery instead of runtime. Destination plugins could be here but they are an easy followup as they are rarely a problem.

Remaining tasks

Add tests, doxygen.

Followup: #2786355: Move multiple provider plugin work from migrate into core's plugin system

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Files: 
CommentFileSizeAuthor
#224 2560795-3-224.patch30.78 KBalexpott
#224 220-224-interdiff.txt1.22 KBalexpott
#220 2560795-3-220.patch29.56 KBalexpott
#220 219-220-interdiff.txt1.3 KBalexpott
#219 2560795-3-219.patch29.56 KBalexpott
#219 201-219-interdiff.txt1.94 KBalexpott
#218 20140411-1b7689fc4711b8c1d11b166b4ab79e28b59095b426657021560b842-1024.png261.44 KBchx
#201 2560795-3-201.patch29.52 KBalexpott
#201 197-201-interdiff.txt1.75 KBalexpott
#197 2560795-3-197.patch29.53 KBalexpott
#197 194-197-interdiff.txt1.51 KBalexpott
#194 191-194-interdiff.txt5.42 KBalexpott
#194 2560795-3-194.patch29.47 KBalexpott
#191 2560795-3-191.patch27.25 KBalexpott
#180 interdiff-2560795-178-180.txt1.47 KBphenaproxima
#180 2560795-180.patch27.23 KBphenaproxima
#178 interdiff-2560795-174-178.txt9.46 KBphenaproxima
#178 2560795-178.patch27.33 KBphenaproxima
#174 2560795_174.patch26.25 KBchx
#101 source_plugins_have_a-2560795-101.patch63.87 KBmikeryan
#94 interdiff.txt1.22 KBpenyaskito
#94 source_plugins_have_a-2560795-94.patch63.91 KBpenyaskito
#82 source_plugins_have_a-2560795-82.patch63.79 KBmikeryan
#64 interdiff.txt1.3 KBmikeryan
#64 source_plugins_have_a-2560795-64.patch39.75 KBmikeryan

Comments

benjy created an issue. See original summary.

benjy’s picture

For purist sakes, we could argue to keep it in migrate_drupal but it’s not a very big class and it’s much simpler to move the class to migrate, I'm +1 for option 1 at this stage.

phenaproxima’s picture

I'm preferential, overall, to option #3, which is to introduce a new annotation type, provided by Migrate Drupal, used specifically for Drupal sources. It could be called MigrateDrupalSource, and it would provide the added benefit of giving us an official place to document hidden-but-crucial annotation properties like source_provider and minimum_schema_version, which is already its own issue: #2559345: Document source_provider and minimum_schema_version annotation properties

But if that doesn't pan out, I'm also OK with option #1.

webchick’s picture

Priority: Normal » Major
Issue tags: +Migrate critical

This sounds like something we need to figure out in order to declare the API complete. Marking as Migrate critical.

mikeryan’s picture

I prefer option #3. Moving DrupalSqlBase to migrate means that you won't get actual errors in discovery of source plugins, but you will be discovering source plugins that don't actually make sense without migrate_drupal enabled.

phenaproxima’s picture

Discussed on IRC with @benjy, and we devised what seems to both of us like a reasonable solution: standardize the source_provider annotation property and use it to filter out plugins which cannot be instantiated for dependency reasons.

Example: consider the d6_user source plugin. With this proposed solution, its annotation would look like this:

/**
 * @MigrateSource(
 *   id = "d6_user",
 *   source_provider = "drupal.6.user"
 * )
 */

With this kind of tagging, the plugin manager can know ahead of time that the plugin is built on top of Migrate Drupal (the drupal part of the source_provider value), that it's relevant to Drupal 6, and that it's part of the User module's migration path. The hierarchy helps with grouping plugins (i.e., in the Views-like UI that @benjy's working on), to any level of specificity.

We could apply this same logic to non-Drupal sources:

/**
 * @MigrateSource(
 *   id = "wp_posts",
 *   source_provider = "wordpress.4"
 * )
 */

There are a lot more details to be fleshed out here, but so far this strikes me as workable. Thoughts?

benjy’s picture

I think we should roll a path with something like #6.

mikeryan’s picture

If we move forward with #6, does that subsume #2559345: Document source_provider and minimum_schema_version annotation properties?

Also note #2569805: For Drupal migration, identify the source module - for the UI we want to be able to identify the source module for all Drupal migrations, including variables, and the source_provider annotation by itself doesn't get us there, can we work a solution for that in here? E.g., allow the source provider to be overridden in the configuration:

source:
  plugin: variable
  variables:
    - actions_max_stack
  provider: system

and have a getProvider() API?

phenaproxima’s picture

@benjy and I talked more about this on IRC. We still think this is workable, but the main issue is how to make the system aware that a source_provider value like drupal.6.node or drupal.7.aggregator is a dependency on the Migrate Drupal module.

My thought is we could add something like this to Migrate Drupal:

class MigrateDrupalSourceProvider implements ServiceModifierInterface {

    public function alter(ContainerBuilder $container) {
        $service = $container->get('plugin.manager.migrate.source');
        $service['tags'][] = array(
            'name' => 'migrate.source_provider',
            'pattern' => 'drupal.*',
        );
    }

}

This would explicitly mark the plugin.manager.migrate.source service as being responsible for plugins whose source_provider annotation matches the drupal.* pattern. By comparing source_provider annotations to service tags, @benjy's UI would easily be able to tell which plugins can be instantiated.

phenaproxima’s picture

Took a first pass at this:

  • Migrate Drupal implements a service modifier which tags the plugin.manager.migrate.source service with the migrate.source_provider tag. This tag contains a list of regular expressions, meaning that the plugin manager is responsible for any source plugin whose source_provider annotation matches any of those expressions.
  • DrupalSqlBase's checkRequirements() is totally rewritten. It parses the source_provider annotation, which now has the form of drupal.MODULE.SCHEMA_EXPRESSION. MODULE is obviously the name of the module, and SCHEMA_EXPRESSION is an optional regular expression which the installed schema version must match.
  • Added a test of the new checkRequirements() logic.
  • Changed all existing source_provider annotations to conform to the new format.

Status: Needs review » Needs work

The last submitted patch, 10: 2560795-10.patch, failed testing.

The last submitted patch, 10: 2560795-10.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
21.95 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,900 pass(es), 0 fail(s), and 1 exception(s). View

I done screwed up. Maybe this will be better...

The last submitted patch, 10: 2560795-10.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: 2560795-13.patch, failed testing.

The last submitted patch, 13: 2560795-13.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
21.95 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 116,807 pass(es), 428 fail(s), and 22 exception(s). View

Wanna play again?

Status: Needs review » Needs work

The last submitted patch, 17: 2560795-17.patch, failed testing.

Status: Needs work » Needs review

phenaproxima queued 17: 2560795-17.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 17: 2560795-17.patch, failed testing.

Status: Needs work » Needs review

phenaproxima queued 17: 2560795-17.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 17: 2560795-17.patch, failed testing.

Status: Needs work » Needs review

phenaproxima queued 17: 2560795-17.patch for re-testing.

quietone’s picture

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -101,13 +101,25 @@ public static function create(ContainerInterface $container, array $configuratio
+                throw new RequirementsException('Incompatible schema version ' . $installed_version);

How will the user know which module has an incompatible schema version?

Status: Needs review » Needs work

The last submitted patch, 17: 2560795-17.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review

OK, screw it. PIFR is randomly failing the patch, repeatedly, for no very good or obvious reason. DrupalCI, thy results are canonical.

phenaproxima queued 17: 2560795-17.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 17: 2560795-17.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review

Eurgh!!! I give up.

benjy’s picture

Only did a super quick review, couple of notes below. I'm still in favour of the major.minor.patch format for the source provider but i'm unsure about altering the service tags for source modules that want to specific a new pattern. Let me think on it some more.

  1. +++ b/core/modules/image/src/Plugin/migrate/source/d7/ImageStyles.php
    @@ -15,7 +15,7 @@
    + *   source_provider = "drupal.image.7[0-9]{3}"
    

    I'm not sure about the regex, do we really need that? Maybe we could just specify a minimum?

  2. +++ b/core/modules/migrate_drupal/src/SourcePluginServiceModifier.php
    @@ -0,0 +1,28 @@
    +    // Tag the source plugin manager as being responsible for any source whose
    +    // source_provider annotation matches the drupal.* pattern.
    

    I think the thing is, the plugin manager is already responsible for those plugins, that's exactly the problem we have now. Is there a part to this patch missing that would have filtered out plugins that didn't match a registered pattern in the plugin manager?

chx’s picture

Last I heard, annotation properties were not in short supply. You do not even need to define them on the annotation class if you do not want. So: if you want to depend on a module, say so! If you want a version regex, say so! Magic strings mixing these into one mess of a property: nope.

chx’s picture

Of course the best would be to roll the migration templates back into migrate_drupal and also to only use templates when necessary and stick with config entities when not but I guess those ships have sailed. (These are the two why I try not to participate much.)

chx’s picture

I filed an issue for putting them where they belong because it is impossible to find migrations these days. #2589805: Add documentation on how to find the menu migrations

phenaproxima’s picture

Status: Needs review » Needs work

I spoke with @EclipseGc and @jhodgdon about this at BADCamp and I believe I've found a much more elegant solution.

We can define an entirely new annotation type -- MigrateDrupalSource -- which extends MigrateSource and lives in Migrate Drupal. We can then apply that annotation to all plugins which depend on DrupalSqlBase. This will have the effect that, if Migrate Drupal is disabled, any MigrateDrupalSource-annotated plugins will be completely invisible to the plugin system. Boo-yah!

Not only does this solve our dependency issue, it's also easy to implement and it gives us the flexibility to add Migrate Drupal-specific annotation properties that are documented in a single, sensible place.

I will try to create the patch for this ASAP. It's such a beautiful solution that I could almost weep.

phenaproxima’s picture

chx’s picture

Status: Needs review » Needs work

Great solution! More comments on the new annotation to explain why it's necessary would be welcome though.

The last submitted patch, 35: 2560795-35.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Postponed
Issue tags: -Migrate critical

This will not work until #2600926: Allow annotations to inherit across namespaces is in core. Postponing.

Also removing the Migrate Critical tag since this is only revealed by @benjy's migration UI, which is not open to the public yet :) Under 98% of circumstances, users will not run into this problem.

benjy’s picture

mikeryan’s picture

Version: 8.0.x-dev » 8.1.x-dev

Also removing the Migrate Critical tag since this is only revealed by @benjy's migration UI, which is not open to the public yet :) Under 98% of circumstances, users will not run into this problem.

Are we still using the "Migrate critical" tag? At any rate, with the plugin patch in, it is also revealed by migrate_tools, and will affect anything trying to do straight-forward discovery on migration plugins. Under 98% of non-Drupal migration circumstances, users *will* run into this problem.

  $manager = \Drupal::service('plugin.manager.migration');
  $plugin_definitions = $manager->getDefinitions();

Without migrate_drupal enabled:

exception 'Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException' with message 'You have requested a non-existent service "plugin.manager.migrate.cckfield". Did you mean one of these:
"plugin.manager.migration", "plugin.manager.image.effect", "plugin.manager.migrate.source",
"plugin.manager.migrate.process", "plugin.manager.migrate.destination", "plugin.manager.migrate.id_map",
"plugin.manager.views.field"?' in
/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Component/DependencyInjection/Container.php:162

With migrate_drupal enabled, but no "migrate" connection to a Drupal database configured:

exception 'Drupal\Core\Database\ConnectionNotDefinedException' with message 'The specified database connection is not defined: migrate' in
/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Core/Database/Database.php:366
mikeryan’s picture

Applied both patches (with a trivial reroll of this one, not finding the d7_field_instance place to patch). I still get the attempt at the missing cckfield service. I believe the node deriver is triggering that, continuing to dig...

mikeryan’s picture

Capturing where I am with this, quitting for the night:

  1. The node derivers crap out because they're trying to get the cckfield service, which lives in migrate_drupal. I can get around this by having them catch the exception and return NULL when it's not there. This lets getDefinitions() succeed.
  2. I can then catch and ignore PluginNotFoundException when creating instances, which works for filtering out most of the irrelevant migrations, but...
  3. I get PluginException (instance class "Drupal\migrate_drupal\Plugin\migrate\CckMigration" does not exist) on the d6_field* migrations, which specify that as their migration class. I could silently catch PluginException to get by this, but then might miss exceptions that matter in "real" migrations.
  4. Catching and ignoring PluginException gets me close to what I want - I do get my four beer migrations in migrate_example (unsorted, which is another issue to address), with only two "extraneous" migrations, block_content_type and block_content_body_field. The problem here is that these migrations have no inherent dependencies on any Drupal source (they simply configure a default block type with a body field) - there is no red flag raised that a general tool can use to know that these are really only relevant in the context of a Drupal-to-Drupal migration.
mikeryan’s picture

OK, here's a version of the patch that I've got working with migrate_tools (don't exactly know what's up with actions in the interdiff...). Basically, just having the node derivers sanity check the cckfield service let's me get to the point where the tools can handle the rest.

willwh’s picture

I've rerolled this patch, because it's no longer applying against the current 8.1.x-dev branch. I've never made an interdiff before, so this was fun :)

mikeryan’s picture

The last patch dropped the MigrateDrupalSource annotation, which is kinda important. Restoring here - this (along with the annotations patch) works with migrate_plus/migrate_tools in contrib.

agoradesign’s picture

Hi Mike,
it seems that your patch is not complete (anymore?). I've setup a site with the current dev of core 8.1.x, migrate_tools 8.2.x and migrate_plus 8.2.x, and applied this and other patch, that is also needed for migrate_tools to work. But when I run drush migrate-status, I get this error:

Error: Class 'Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase' not found in
/var/www/drupalvm/web/core/modules/language/src/Plugin/migrate/source/Language.php, line 18

-> this class still has the "MigrateSource" annotation, changing it to "MigrateDrupalSource" helps.

mikeryan’s picture

FileSize
358 bytes

@agoradesign: Thanks for picking that up, I've added it in.

mikeryan’s picture

mikeryan’s picture

Status: Postponed » Needs review

#2600926: Allow annotations to inherit across namespaces has been committed, this is now unblocked - let's give the bots a shot at it...

The last submitted patch, 43: source_plugins_have_a-2560795-43.patch, failed testing.

The last submitted patch, 45: source_plugins_have_a-2560795-45.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 48: source_plugins_have_a-2560795-47.patch, failed testing.

The last submitted patch, 44: source_plugins_have_a-2560795-44.patch, failed testing.

heddn’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review

Let's see what happens when we use 8.2.x? I'm not expecting much better luck.

Status: Needs review » Needs work

The last submitted patch, 48: source_plugins_have_a-2560795-47.patch, failed testing.

mikeryan’s picture

The testbot is not reporting anything useful, but locally running the action tests:

core$ ../vendor/bin/phpunit --group=action
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

....E

Time: 8.94 seconds, Memory: 240.25Mb

There was 1 error:

1) Drupal\Tests\action\Kernel\Migrate\d7\MigrateActionsTest::testActions
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "d6_field_instance" plugin does not exist.

/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:57
/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryCachedTrait.php:30
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/src/Plugin/MigratePluginManager.php:61
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/src/Plugin/Migration.php:359
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/src/Plugin/MigrationDeriverTrait.php:34
/Users/mikeryan/Sites/8.1.x/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php:90
/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php:105
/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php:91
/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:279
/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:179
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/src/Plugin/MigrationPluginManager.php:146
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/src/Plugin/MigrationPluginManager.php:99
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/src/Plugin/MigrationPluginManager.php:85
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:239
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:152
/Users/mikeryan/Sites/8.1.x/core/modules/action/tests/src/Kernel/Migrate/d7/MigrateActionsTest.php:27

FAILURES!
Tests: 5, Assertions: 28, Errors: 1.

Note that it's the D7 MigrateActionsTest causing D6NodeDeriver to be invoked (and ultimately try to use d6_field_instance). I have no idea what's changed in the current patch that would cause this.

Once upon a time, on a former MacBook, I had figured out how to get PhpUnit tests to stop at breakpoints in PhpStorm, having trouble now (I've got a debug configuration that will run the tests within PhpStorm, but my breakpoints are ignored)...

mikeryan’s picture

Wait, WTF, the action tests actually pass in PhpStorm but not at the command line...

mikeryan’s picture

OK, debugging in migrate_tools:

  1. We do getDefinitions() to get all migration plugins, expecting that we'll be able to catch the non-useful ones at createInstance()/checkRequirements() time.
  2. However, derivers are applied at getDefinitions() time.
  3. D6NodeDeriver does getSourcePlugin('d6_field_instance')
  4. d6_field_instance has a source_provider of 'content' (i.e., it expects to have a connection to a D6 database which contains an enabled 'content' module).
  5. We don't even have a database connection, let alone one to a D6 database, let alone one with the content module enabled.
  6. You might think we'd see a complaint about no connection, but DrupalSqlBase::getSystemData() swallows all exceptions so all that ends up propagating up is the fact that we can't create the source plugin.

There's not much we can do about it from migrate_tools - the exception entirely aborts getDefinitions() so we get no migration plugins to work with, not even the ones that would work if we could only discover them...

Perhaps the derivers can catch PluginNotFoundException on the getSourcePlugin() calls, but it feels like the problem is deeper than that. I may throw that into the patch though just to see if it makes the testbot any happier.

agoradesign’s picture

The same error happened to me on my 8.1.x test install last weekend, as soon as I enabled migrate_drupal. Without migrate_drupal, I had no problems

mikeryan’s picture

In IRC, @dawehner was asking why we didn't take the approach of adding migrate_drupal as a module dependency to the migration .yml files - didn't have an answer off the top of my head.

As for the failure of this annotation-based patch to work, looking at the tests for #2600926: Allow annotations to inherit across namespaces I realize that somewhere we need to inject that additional namespace for MigrateDrupalSource to be picked up. Looking at that now...

mikeryan’s picture

Status: Needs work » Needs review
FileSize
38.33 KB
2.86 KB

OK, got the additional annotation namespace in (also fixed up one more source plugin which wasn't using MigrateDrupalSource). In the migrate_tools context we're now getting ConnectionNotDefinedException, but let's see how the testbot does with this.

Status: Needs review » Needs work

The last submitted patch, 61: source_plugins_have_a-2560795-61.patch, failed testing.

mikeryan’s picture

That's better - now just MigrationTest is failing, with "The "d6_field" plugin does not exist.". It's actually bogosity in the test, which is using a Drupal source plugin without enabling migrate_drupal. It got away with this before - all it's doing is getting the source plugin and fetching its ID, so the lack of migrate_drupal or any source connection didn't bit it - but now that the source plugin uses the MigrateDrupalSource annotation, the hidden dependency of the d6_field plugin on migrate_drupal is revealed. Let's see if we can find a source plugin not dependent on migrate_drupal...

mikeryan’s picture

OK, I expect this one to pass.

The last submitted patch, 61: source_plugins_have_a-2560795-61.patch, failed testing.

mikeryan’s picture

In the meantime - this patch is necessary but not sufficient for the tool use case (in general, being able to do getDefinitions() on the migration plugin manager without crashing and burning). It addresses the case where migrate_drupal is not enabled yet source plugins implicitly dependent on it exist in enabled modules. We've still got the problem of having migrate_drupal enabled and trying to get all plugins, where exceptions will go flying - either because there's no Drupal source database connection configured, or there is one configured, but plugins targeting a Drupal version other than the one that database contains will be... confused. I'll open a separate issue for that.

mikeryan’s picture

OK, both branches have passed - ready for manual review.

willwh’s picture

Status: Needs review » Reviewed & tested by the community

So the secret sauce here was Annotations on the Source Plugin Manager. This is working for me :)

agoradesign’s picture

Is there a chance to get this in for today's 8.1-rc1???

agoradesign’s picture

Version: 8.2.x-dev » 8.1.x-dev

Changing this back to version 8.1.x, as it was changed to 8.2.x in comment #54 only for the test bot

benjy’s picture

Status: Reviewed & tested by the community » Needs work

In IRC, @dawehner was asking why we didn't take the approach of adding migrate_drupal as a module dependency to the migration .yml files - didn't have an answer off the top of my head.

Funny, we'd have gotten this for free with config entities, however its actually a pretty good idea and solves the try/catch in the static constructors mentioned below.

  1. +++ b/core/modules/migrate/tests/src/Kernel/MigrationTest.php
    --- /dev/null
    +++ b/core/modules/migrate_drupal/src/MigrateDrupalServiceProvider.php
    
    +++ b/core/modules/migrate_drupal/src/MigrateDrupalServiceProvider.php
    @@ -0,0 +1,27 @@
    +class MigrateDrupalServiceProvider extends ServiceProviderBase {
    ...
    +  public function alter(ContainerBuilder $container) {
    ...
    +    $definition->setClass('Drupal\migrate_drupal\Plugin\MigrateDrupalPluginManager');
    

    I don't think we want to replace the existing service manager, we should just decorate it. See https://www.previousnext.com.au/blog/decorated-services-drupal-8

  2. +++ b/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php
    @@ -59,10 +60,16 @@ public function __construct($base_plugin_id, PluginManagerInterface $cck_manager
    +    try {
    +      $cckfield_plugin_manager = $container->get('plugin.manager.migrate.cckfield');
    +      return new static(
    +        $base_plugin_id,
    +        $cckfield_plugin_manager
    +      );
    +    }
    +    catch (ServiceNotFoundException $e) {
    +      return NULL;
    +    }
    

    A try/catch in the static constructor doesn't feel right, i've said before a temporary fix would be to move this to migrate_drupal. But as dawehner mentioned above, adding a module dependency to the plugin definition and filtering these out in the plugin manager would solve this.

JonasRMuc’s picture

mikeryan’s picture

mikeryan’s picture

Version: 8.1.x-dev » 8.2.x-dev
ayalon’s picture

Status: Needs work » Reviewed & tested by the community

I'm using this patch for a while now with the version 2.x-dev source plugins and migrate tools. Should be part of the next Drupal minor.

catch’s picture

Status: Reviewed & tested by the community » Needs work

#72 still needs addressing.

mikeryan’s picture

@ayalon: The tools no longer require this fix (nevertheless, it still should be fixed).

dsnopek’s picture

I've been trying to use 'migrate' to import the demo data in Panopoly (we did this in D7 too) per #2732417: Port Panopoly Demo to Drupal 8!, and without this patch, I need to enable 'drupal_migrate' even though we're importing data using the 'embedded_data' source plugin and not importing anything from an older Drupal version. The error I get without the patch is:

Uncaught PHP Exception Symfony\\Component\\DependencyInjection\\Exception\\ServiceNotFoundException: "You have requested a non-existent service "plugin.manager.migrate.cckfield". Did you mean one of these: "plugin.manager.migration", "plugin.manager.image.effect", "plugin.manager.migrate.source", "plugin.manager.migrate.process", "plugin.manager.migrate.destination", "plugin.manager.migrate.id_map", "plugin.manager.views.field"?" at /var/www/drupal/core/lib/Drupal/Component/DependencyInjection/Container.php line 157
mikeryan’s picture

Issue tags: +Migrate critical

Restoring the "Migrate critical" tag, it's really a big problem for any migration scenario outside of the core upgrade (e.g., working on the D8 version of wordpress_migrate, it has to include a logically-unnecessary dependency on migrate_drupal for plugin discovery to work).

ckaotik’s picture

I'm using the patch from #64, but still cannot get migrate_tools' drush ms working without enabling migrate_drupal.

They use the \Drupal::service('plugin.manager.config_entity_migration')->createInstances([]) to fetch a list of migrations and die on d6_field_formatter_settings (core/modules/field/migration_templates/d6_field_formatter_settings.yml), because it explicitly declares the use of class: Drupal\migrate_drupal\Plugin\migrate\CckMigration. Seems we missed this one.

exception 'Drupal\Component\Plugin\Exception\PluginException' with message 'Plugin (d6_field_formatter_settings) instance class "Drupal\migrate_drupal\Plugin\migrate\CckMigration" does not exist.' in /srv/www/htdocs/PROJECT/html/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php:97
Stack trace:
#0 /srv/www/htdocs/PROJECT/html/core/lib/Drupal/Core/Plugin/Factory/ContainerFactory.php(17): Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass('d6_field_format...', Array, 'Drupal\migrate\...')
#1 /srv/www/htdocs/PROJECT/html/core/modules/migrate/src/Plugin/MigrationPluginManager.php(99): Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('d6_field_format...', Array)
#2 /srv/www/htdocs/PROJECT/html/modules/contrib/migrate_tools/migrate_tools.drush.inc(438): Drupal\migrate\Plugin\MigrationPluginManager->createInstances(Array)
#3 /srv/www/htdocs/PROJECT/html/modules/contrib/migrate_tools/migrate_tools.drush.inc(140): drush_migrate_tools_migration_list(NULL, '')
#4 [internal function]: drush_migrate_tools_migrate_status()
#5 /usr/share/drush/drush-8.0.5/includes/command.inc(366): call_user_func_array('drush_migrate_t...', Array)
#6 /usr/share/drush/drush-8.0.5/includes/command.inc(217): _drush_invoke_hooks(Array, Array)
#7 [internal function]: drush_command()
#8 /usr/share/drush/drush-8.0.5/includes/command.inc(185): call_user_func_array('drush_command', Array)
#9 /usr/share/drush/drush-8.0.5/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#10 /usr/share/drush/drush-8.0.5/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#11 /usr/share/drush/drush-8.0.5/drush.php(12): drush_main()
#12 {main}

I just wish we had a way to overwrite/decide ourselves which migrations we want to use, instead of just always loading anything we can find... But that's another issue for another day ;)

mikeryan’s picture

Status: Needs work » Needs review
FileSize
63.79 KB

Plugins do have a means of filtering on a module dependency - the provider key. So, I added "provider: migrate_drupal" to all the Drupal migrations. That's not quite enough, though, because derivers are invoked before the provider is checked. So, I extended ContainerDerivativeDiscoveryDecorator to pre-check the provider.

I borrowed the base (migrate_drupal not installed) test from #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled - this still leaves the other angle (migrate_drupal is installed but no database is configured) broken, but we can pick up on that one in the other issue.

benjy’s picture

This seems like a pretty good solution to me. +1

benjy’s picture

chx suggested that we could also fix this in core? We could test this patch against the test in #82.

diff --git a/docroot/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php b/docroot/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
index 2acd41b..89ac166 100644
--- a/docroot/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
+++ b/docroot/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -284,7 +284,10 @@ protected function findDefinitions() {
       if (is_object($plugin_definition) && !($plugin_definition = (array) $plugin_definition)) {
         continue;
       }
-      if (isset($plugin_definition['provider']) && !in_array($plugin_definition['provider'], array('core', 'component')) && !$this->providerExists($plugin_definition['provider'])) {
+      $existing_provider = function ($provider) {
+        return in_array($provider, ['core', 'component']) || $this->providerExists($provider);
+      };
+      if (isset($plugin_definition['provider']) && !array_filter((array) $plugin_definition['provider'], $existing_provider)) {
         unset($definitions[$plugin_id]);
       }
     }
mikeryan’s picture

@benjy: It's not clear to me what that patch changes in behavior? The provider check is still too late to prevent us from invoking an invalid deriver.

If we do want to make this a broader fix, we could simply put the provider check in ContainerDerivativeDiscoveryDecorator... like so.

mikeryan’s picture

Status: Needs review » Needs work

Oh, duh, that's going to break, because anything else using ContainerDerivativeDiscoveryDecorator would need to pass in a module handler...

The last submitted patch, 85: source_plugins_have_a-2560795-85.patch, failed testing.

benjy’s picture

@benjy: It's not clear to me what that patch changes in behavior? The provider check is still too late to prevent us from invoking an invalid deriver.

Looking at the code now, you're right. I'm happy with the solution in #82 but would be good to get a review from alexpott.

mikeryan’s picture

Status: Needs work » Needs review

OK - patch in #82 needs review, we'll pretend #85 didn't happen...

mikeryan’s picture

Issue tags: -Needs change record

Draft change record added.

penyaskito’s picture

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
@@ -0,0 +1,68 @@
+    // Test with all modules containing Drupal migrations.

Isn't there any way of getting this list from Drupal core? I'm guessing we will forget to edit this test if new migrations are added to modules that don't have any already.

vasi’s picture

I'm not sure if I like overriding 'provider' this way. By default the 'provider' of each plugin is the module that defines it. Presumably Drupal uses this information in some way? Are there any other situations where provider is overridden? Any documentation of what this means, semantically?

Also, we already have a 'Drupal 6' or 'Drupal 7' tag that indicates that migrations are for migrate_drupal. I don't like having to say this same thing in two places, it provides one more way for people to mess up.

mikeryan’s picture

Isn't there any way of getting this list from Drupal core? I'm guessing we will forget to edit this test if new migrations are added to modules that don't have any already.

We could use the MigrationPluginManager... Oops;)! Kind of a chicken-and-egg problem there... I suppose we *could* use YamlDiscovery to dynamically set $modules, but I'm not sure that's worth it - we would have to do that in the constructor, wouldn't we? Or could it be done in setUp()?

I'm not sure if I like overriding 'provider' this way. By default the 'provider' of each plugin is the module that defines it. Presumably Drupal uses this information in some way? Are there any other situations where provider is overridden? Any documentation of what this means, semantically?

Core does explicitly support providing a provider in configuration (it checks for a configured provider before setting the provider to the current module), although I can't find any example of it being used in core outside of tests. I think it does make sense, in that our precise problem here is that migrate_drupal is logically a provider of these migrations, but we've chosen to have them live in their relevant modules (for reasons detailed in #2533886: [meta] Move module-specific migration support into the particular modules supported).

As near as I can tell, the meaning of "provider" is not documented, but the one thing it is used for is to exclude plugins whose provider is not enabled (exactly what we need).

The behavior we're looking for is that no Drupal migration plugin should be instantiated by the plugin manager unless both the module it contains and migrate_drupal are enabled. The Yaml discovery ensures the former, and making migrate_drupal a provider ensures the latter. I don't see a cleaner way to handle this.

penyaskito’s picture

Rerolled #82 after #2691659: d7 filter weights not migrating to d8 landed. Two phpcs raised issues fixed, see interdiff.

penyaskito’s picture

Issue tags: +DevDaysMilan
animaci’s picture

When I use this patch I get error message: Migration migration_name does not exist.
Do you have maybe any idea? Thanks!

mikeryan’s picture

When I use this patch I get error message: Migration migration_name does not exist.
Do you have maybe any idea? Thanks!

@animaci: That's not much to go on. Exactly what context do you see that message? What operation are you performing?

mikeryan’s picture

Issue tags: +blocker

This situation continues to be a major PITA for non-upgrade migration scenarios, and blocks cleaner contrib integration: #2752335: Properly integrate configuration-entity-based migrations with the core plugin manager.

animaci’s picture

@mikeryan: Thanks, I think it was my fault, it was not caused by the patch. Sorry for confusion. Just forget my comment :) Thanks!

phenaproxima’s picture

Looks pretty decent to me. I have a few minor issues, but overall this seems like a good, clean solution.

  1. +++ b/core/modules/migrate/src/Plugin/Discovery/MigrateContainerDerivativeDiscoveryDecorator.php
    @@ -0,0 +1,56 @@
    +class MigrateContainerDerivativeDiscoveryDecorator extends ContainerDerivativeDiscoveryDecorator {
    

    Now THAT is a class name :) It's a mouthful but it doesn't really explain what this class does. Can a doc comment be added for that?

  2. +++ b/core/modules/migrate/src/Plugin/Discovery/MigrateContainerDerivativeDiscoveryDecorator.php
    @@ -0,0 +1,56 @@
    +   * Creates a new instance.
    

    Should be "Constructs a new MigrateContainerDerivativeDiscoveryDecator."

  3. +++ b/core/modules/migrate/src/Plugin/Discovery/MigrateContainerDerivativeDiscoveryDecorator.php
    @@ -0,0 +1,56 @@
    +   *   The parent object implementing DiscoveryInterface that is being
    

    It's pretty obvious from the parameter type that it's implementing DiscoveryInterface ;-)

  4. +++ b/core/modules/migrate/src/Plugin/Discovery/MigrateContainerDerivativeDiscoveryDecorator.php
    @@ -0,0 +1,56 @@
    +  protected function getDeriver($base_plugin_id, $base_definition) {
    

    $base_definition should be type hinted if possible.

  5. +++ b/core/modules/migrate/src/Plugin/Discovery/MigrateContainerDerivativeDiscoveryDecorator.php
    @@ -0,0 +1,56 @@
    +    if (isset($base_definition['provider']) && !in_array($base_definition['provider'], array('core', 'component')) && !$this->providerExists($base_definition['provider'])) {
    

    providerExists() is just a call to the module handler, so do we really need it in its own method?

  6. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
    @@ -0,0 +1,63 @@
    +    $migration_plugins = \Drupal::service('plugin.manager.migration')->getDefinitions();
    

    We need a comment here explaining the nature of this test. What would happen if the test broke? Also...for correctness' sake, kernel tests should use $this->container->get(), not \Drupal::service().

mikeryan’s picture

Addressed all the comments above, except

$base_definition should be type hinted if possible.

This function signature matches the parent.

mikeryan’s picture

Anyone out there willing to review the latest patch and either give it an RTBC or identify any other changes needed?

benjy’s picture

Status: Needs review » Reviewed & tested by the community

I think the patch is good, I did mention in #88 that a review from alexpott would be good. Maybe a RTBC is the best way to get that :)

mikeryan’s picture

Issue tags: -Migrate BC break

The current approach is not a BC breaker.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I think the approach is valid but I do share some of the concerns of @vasi in #92. However if migrate_drupal is installed then existing contrib and custom plugins that depend on it will continue to work - so this is not a migrate BC break per se. And therefore I agreed with @mikeryan that we should commit this to 8.1.x as well.

I discussed how to inform developers that they need to add the provider key for migrate plugins that depend on migrate_drupal - we considered triggering a user recoverable error but that doesn't really buy us anything because it valid for a migration plugin to not have a provider.

Given that this patch achieves what we want it to and we can continue to evaluate it as solution once committed I'm happy to see this go in.

Committed and pushed 89815ba33923ecd0018b62afb393d05d987a40fa to 8.2.x and e98b14a to 8.1.x. Thanks!

  • alexpott committed 89815ba on 8.2.x
    Issue #2560795 by mikeryan, phenaproxima, penyaskito, willwh, benjy:...
chx’s picture

Assigned: Unassigned » chx
Status: Fixed » Reviewed & tested by the community

What on earth have happened. This needs to be rolled back and properly fixed! Any solution that involves editing every migration is a hack. Not for the first time we are hacking migrate to fix a core bug. :(

A) roll back, that's why I set it to RTBC

B) I will post a patch that fixes this in the plugin subsystem and then the source plugins. The real problem is that the source plugins have a dependency on more than one provider. Currently the plugin system uses one provider which is the module its namespace is in but it's entirely possible as the issue shows that a plugin depends on more than one provider and so the plugin needs to be able to have a list of providers it depends on. That's a plugin system limitation I thought I have made this clear but since I really badly don't want to post any non migrate patches I didn't but apparently I need to. Also, all the decorator needs to do is inherit the provider part of the definition and then DefaultPluginManager::findDefinitions() will throw the definition out if the providers are not there. Already explicit provider definitions are honored (see AnnotatedClassDiscovery::prepareAnnotationDefinition) so the plugin system just needs to allow for an array as I mentioned previously and that's all.

chx’s picture

FileSize
3.72 KB

Actually, AFAIK DerivativeDiscoveryDecorator::mergeDerivativeDefinition already inherits everything including providers. Here's a tentative patch with one plugin edited for demo, non-testing because it first needs a rollback. I need to rename ProviderDiscovery to ProviderDecorate and perhaps add a wrapper method for the service call but it shows the way.

mikeryan’s picture

A couple of points:

  1. It's not just the source plugins - there are migrations such as block_content_body_field which make no sense outside of a Drupal-to-Drupal migration and are not using a migrate_drupal-dependent source plugin. We don't want the plugin manager returning those.
  2. findDefinitions() is too late to prevent the node derivers from blowing up in the absence of migrate_drupal.
chx’s picture

Assigned: chx » Unassigned
Status: Reviewed & tested by the community » Fixed

Edit: deleted.

chx’s picture

Status: Fixed » Reviewed & tested by the community

I can't let this go and just edit a comment in a fixed issue. Let me reiterate:

The fundamental flaw of this issue is people hacking migrate when the shortcoming is in core. This is not the first time this happens. Can we stop doing this?

It is entirely possible to come up with a plan that covers almost all of the migration to find their dependencies automatically and only require the source or migration plugins to use an explicit provider when the automated process is inadequate. Mike points out the three migrations using the embedded_data source need to do this. That's fine. That's three.

What core misses here

  1. Add support for multiple providers in both findDefinitions and the deriver. Don't try to derive if one if the providers are missing. I have absolutely no idea why this (at least for the singular case) is in the migrate deriver. If I put there, that was a mistake.
  2. Add an annotation property which allows automated multiple provider finding based on use statements. We have the pieces for this just need to stitch them together: the parser has a getUseStatements() call so if we were to pass the parser to prepareAnnotationDefinition then we could just call getProviderFromNamespace on all the use statements and add them to the provider list.

End. Note that most of this is already coded either above or in https://www.drupal.org/node/2488258#comment-11402375 -- that patch persists use statements but processing them instead is better.

Once this is done, we will need to use the code above to copy source plugin providers into the Migration plugin providers. And then just opt in MigrateSource into this new mechanism and mostly be done:

namespace Drupal\node\Plugin\migrate\source\d6;

use Drupal\migrate\Row;
use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;

so this plugin depends on node, migrate and migrate_drupal. Yes, a few plugins will need explicit provider lists (VocabularyPerType , NodeRevision twice, CommentVariablePerCommentType, TermNodeRevision, ViewMode , FieldInstancePerViewMode, UrlAlias twice) but most of them won't. And most definitely you won't need to edit every migration.

  • alexpott committed fca71fa on 8.2.x
    Revert "Issue #2560795 by mikeryan, phenaproxima, penyaskito, willwh,...
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Okay discussed with @mikeryan in IRC - we agreed to revert and go for a fix that does not require updates to migrations whilst also being simple.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.96 KB

So here's an approach built on @vasi's suggestion of re-using tags. It decorates discovery to remove any definitions with missing definitions. I think this is better than the previous approach because no migrations have to change and it affects all definitions not just derivatives. Improved the test as well. I think it would be great to a test module with a migration that does not depend on Drupal.

No interdiff because new approach.

alexpott’s picture

We have such migrations already in a test module...

benjy’s picture

Tags for me have always been optional, basically just a feature for the UI and if you don't provide them then no problem, your migrations still work. Now they have functional meaning which can lead to fatal errors. I think using the provider key still makes more sense as that is a defined value, not just free text like tags.

Just because core doesn't have to update its migrations here, everyone else is still going to have to update their custom migrations unless of course they copied from core and have the tags already, which could be the case.

chx’s picture

Assigned: Unassigned » chx

I am coding as fast as I can. Wait an hour or two, I am almost done. Teaser trailer:

namespace Drupal\Core\Plugin\Discovery;
use Doctrine\Common\Reflection\StaticReflectionParser;
use Drupal\Component\Annotation\AnnotationInterface;
class AnnotatedClassDiscoveryAutomatedProviders extends AnnotatedClassDiscovery {
  protected function prepareAnnotationDefinition(AnnotationInterface $annotation, $class, StaticReflectionParser $parser) {
    $provider_set = $annotation->getProvider();
    parent::prepareAnnotationDefinition($annotation, $class, $parser);
    if (!$provider_set) {
      $namespaces = array_map(['Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery', 'getProviderFromNamespace'], $parser->getUseStatements());
      $namespaces[] = $annotation->getProvider();
      $annotation->setProvider(array_filter(array_unique($namespaces)));
    }
  }
}
chx’s picture

Progress report.

Status: Needs review » Needs work

The last submitted patch, 118: 2560795_118.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
13.74 KB

Try #2. So this patch

  1. Adds an annotated class discovery variant which automatically discovers provider dependencies based on use statements.
  2. Uses this in the new MigrationSourcePluginManager
  3. Adds a migrate specific decorator and uses it in MigrationPluginManager to copy the source plugin providers into the migration plugin provider. How fortunate source now has rich provider information...
  4. Moves the provider logic from findDefinitions into a decorator and while at it expands it into multiple provider support.
  5. Uses this decorator in MigrationPluginManager before calling the deriver decorator.

Status: Needs review » Needs work

The last submitted patch, 120: 2560795_119.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
13.81 KB

Eh.

chx’s picture

FileSize
13.83 KB

It's a bit annoying I can't delete comments and at the same time I'd like to show where I am. It's minimal fixes :/

The last submitted patch, 122: 2560795_122.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 123: 2560795_123.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
4.99 KB
14.08 KB

Interdiff is against #120 just to show how inconsequential the change is: the problem was passing in the module list instead of the module handler. This is what I get for trying to simplify the code from findDefinitions.

Status: Needs review » Needs work

The last submitted patch, 126: 2560795_124.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
5.99 KB
15.46 KB

WIN! MigrationTest tries to use d6_field source plugin but the patch removes that because migrate_drupal is not enabled. That's fun.

The rest seems to be fixed by not even passing in the module handler but the providerExists method itself.

chx’s picture

Issue summary: View changes
chx’s picture

Issue summary: View changes
FileSize
26.11 KB
14.53 KB

After talking to alexpott the inherit decorator is not needed because the source plugin will be gone already so what's needed instead is a plugin which filters out migrations if their source plugin is missing.

AnnotatedClassDiscoveryAutomatedProviders now always merges.

Also, the ProviderFilterDecorator is only for a very rare case: where the migration explicitly specifies providers and uses a deriver. No such thing currently.

Added all the necessary provider notations to sources (9) and migrations (3) which makes the test written by alexpott pass. Still much, much better than changing 147 migration YAMLs (plus all the custom ones using core sources).

alexpott’s picture

So the problem we have is class UrlAlias extends UrlAliasBase { - where the use on migrate_drupal is in UrlAliasBase and not UrlAlias. I think this is unexpected. I guess the question is how tricky / expensive is it to determine the full class chain and it's use statements.

alexpott’s picture

And this is where doctrine's limited static reflection is not going to help :(

alexpott’s picture

+++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
@@ -0,0 +1,86 @@
+  /**
+   * Passes through all unknown calls onto the decorated object.
+   */
+  public function __call($method, $args) {
+    return call_user_func_array(array($this->decorated, $method), $args);
+  }
+
+

I don't think this code is necessary.

alexpott’s picture

  /**
   * Gets the name of the provider of the annotated class.
   *
   * @return string
   */
  public function getProvider();

  /**
   * Sets the name of the provider of the annotated class.
   *
   * @param string $provider
   */
  public function setProvider($provider);

The patch in #130 is changing this... (core/lib/Drupal/Component/Annotation/AnnotationInterface.php)

Status: Needs review » Needs work

The last submitted patch, 130: 2560795_130.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

> So the problem we have is class UrlAlias extends UrlAliasBase { - where the use on migrate_drupal is in UrlAliasBase and not UrlAlias. I think this is unexpected. I guess the question is how tricky / expensive is it to determine the full class chain and it's use statements.

I would love to leave that to a followup. It's not exactly tricky, the scaffolding is in place: currently we use the MockFileFinder but Psr0FindFile exists and StaticReflectionParser::parse has code for finding the parent already, the optimization would need to be changed to include any extends or simply we would need to switch off the optimization. But I can do it in this issue if necessary.

> I don't think this code is necessary.

Please open an issue to remove it from InfoHookDecorator and from here. I copy-pasted. This I do not know. Edit: based on our discussion, the decorated object might implement additional interfaces and then the decorator shouldn't remove that functionality. Nevermind the decorated object will fail any type hints at least the method calls work...

I will amend the doxygen tomorrow to reflect it can be a string or an array.

chx’s picture

FileSize
3.64 KB
22.88 KB

Here we go. interdiff is only the relevant parts, I didn't interdiff reverting every source plugin. Doxygen later.

Edit: base on discussions with alexpott the next version will do away with reflection and use a class rather. I will post when I have written doxygen.

Status: Needs review » Needs work

The last submitted patch, 137: 2560795_137.patch, failed testing.

jibran’s picture

I think title and the component of this issue should be changed now after the new direction.

chx’s picture

Title: Source plugins have a hidden dependency on migrate_drupal » Make the plugin system even more awesome and get migrate to ride that unicorn
FileSize
23.18 KB
6.13 KB

@jibran, how's the new title?

Patch removes some cruft from #137 and tightens AnnotatedClassDiscoveryAutomatedProviders , adds doxygen where it was missing.

chx’s picture

Status: Needs work » Needs review
chx’s picture

FileSize
9.25 KB
22.71 KB

Reverted providerExists for even better BC. Interdiff against #137. This is beginning to look like done, there's very little change in existing classes outside of migrate and a few new classes. BC is now complete. The next step will be #2767483: Refactor the process definition normalizer into a decorator so process plugins are in a uniform format while discovering and then another followup which uses this fact to change NoSourcePluginDecorator into NoPluginDecorator .

chx’s picture

FileSize
1.61 KB
22.61 KB

While #142 is ready, this version reads slightly better in my opinion. And it's a bit shorter, too.

Status: Needs review » Needs work

The last submitted patch, 143: 2560795_143.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
22.61 KB

One semicolon. No change.

jibran’s picture

Some minor review points. Totally loving the new title.

  1. +++ b/core/lib/Drupal/Component/Annotation/AnnotationInterface.php
    @@ -13,16 +13,16 @@
    +   * @return string|array
    ...
    +   * @param string|array $provider
    

    Can we add a desc why it can be string and array as well?

  2. +++ b/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php
    @@ -19,6 +19,11 @@ class AnnotatedClassDiscovery implements DiscoveryInterface {
    +  const CLASSANNOTATIONOPTIMIZE = TRUE;
    
    +++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
    @@ -0,0 +1,31 @@
    +  const CLASSANNOTATIONOPTIMIZE = FALSE;
    

    Can we make it SCREAMING_SNAKE_CASE?

  3. +++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
    @@ -0,0 +1,31 @@
    +    $providers = array_flip((array) $annotation->getProvider());
    ...
    +      $providers += array_flip(array_filter(array_map([$this, 'getProviderFromNamespace'], $parser->getUseStatements())));
    ...
    +    $annotation->setProvider(array_keys($providers));
    

    Is there a reason for using array_flip first then array_keys?

  4. +++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
    @@ -0,0 +1,31 @@
    +    do {
    ...
    +    } while ($parser = StaticReflectionParser::getParentParser($parser, $finder));
    

    Now I know who has been adding do while to the core :P. I personally don't like it as coder but meh!

  5. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
    @@ -0,0 +1,81 @@
    +   * @var callable
    

    Desc missing.

  6. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
    @@ -0,0 +1,81 @@
    +   * Remove plugin definitions with non-existing providers.
    

    Removes.

  7. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
    @@ -0,0 +1,81 @@
    +  public static function filterDefinitions(array $definitions, callable $provider_exists) {
    +    // Besides what the caller accepts, we also accept core or component.
    +    $provider_exists = function ($provider) use ($provider_exists) {
    +      return in_array($provider, ['core', 'component']) || $provider_exists($provider);
    +    };
    +    return array_filter($definitions, function ($definition) use ($provider_exists) {
    +      $definition = (array) $definition + ['provider' => []];
    +      $providers = (array) $definition['provider'];
    +      return count($providers) == count(array_filter($providers, $provider_exists));
    +    });
    

    A lot of magic and I love it.

  8. +++ b/core/lib/Drupal/Core/Plugin/Discovery/StaticReflectionParser.php
    @@ -0,0 +1,25 @@
    +  public static function getParentParser(BaseStaticReflectionParser $parser, $finder) {
    

    Desc block missing.

benjy’s picture

+++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
@@ -0,0 +1,81 @@
+    $provider_exists = function ($provider) use ($provider_exists) {

+++ b/core/modules/migrate/migrate.services.yml
--- /dev/null
+++ b/core/modules/migrate/src/Plugin/MigrateSourcePluginManager.php

+++ b/core/modules/migrate/src/Plugin/MigrateSourcePluginManager.php
+++ b/core/modules/migrate/src/Plugin/MigrateSourcePluginManager.php
@@ -0,0 +1,32 @@

@@ -0,0 +1,32 @@
+class MigrateSourcePluginManager extends MigratePluginManager {
...
+  protected function getDiscovery() {

Is this still needed?

chx’s picture

FileSize
22.94 KB
5.02 KB

> A lot of magic and I love it.

Where?? I have contemplated quite a few versions and this seemed like the most readable.

I removed the array_keys / array_flip trickery. Let's not get tricky.

Edit: crossposted with benjy's review but yes the MigrateSourcePluginManager uses the new discovery so it's totally necessary, yes.

Edit2: jibran and benjy both confirmed the closures seemed like a bit magic but they are OK just need to get used to it. Good I haven't used the double array_filter I was planning...

Status: Needs review » Needs work

The last submitted patch, 148: 2560795_147.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

Random bot failure.

Status: Needs review » Needs work

The last submitted patch, 148: 2560795_147.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

git.drupal.org failed while the bot was checking out...

Status: Needs review » Needs work

The last submitted patch, 148: 2560795_147.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 148: 2560795_147.patch, failed testing.

The last submitted patch, 148: 2560795_147.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

Finally!

neclimdul’s picture

With my submission for the most superficial review of 2016:

+++ b/core/lib/Drupal/Component/Annotation/AnnotationInterface.php
@@ -13,16 +13,18 @@
+   *   Can be str4ing or array because of backwards compatibility reasons.
...
+   *   Can be str4ing or array because of backwards compatibility reasons.

s/str4ing/string/

chx’s picture

Issue summary: View changes

  • alexpott committed 89815ba on 8.3.x
    Issue #2560795 by mikeryan, phenaproxima, penyaskito, willwh, benjy:...
  • alexpott committed fca71fa on 8.3.x
    Revert "Issue #2560795 by mikeryan, phenaproxima, penyaskito, willwh,...

  • alexpott committed 89815ba on 8.3.x
    Issue #2560795 by mikeryan, phenaproxima, penyaskito, willwh, benjy:...
  • alexpott committed fca71fa on 8.3.x
    Revert "Issue #2560795 by mikeryan, phenaproxima, penyaskito, willwh,...
mikeryan’s picture

I see that this is committed to 8.3.x - what about 8.2.x (at least)?

alexpott’s picture

@mikeryan those status messages are an unfortunate consequence of creating the new 8.3.x branch - nothing (other than that has occurred) it has both the original commit and then revert. So it's still not committed anywhere.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Component/Annotation/AnnotationInterface.php
    @@ -13,16 +13,18 @@
        *
    -   * @return string
    +   * @return string|array
    +   *   Can be str4ing or array because of backwards compatibility reasons.
        */
       public function getProvider();
    ...
    -   * @param string $provider
    +   * @param string|array $provider
    +   *   Can be str4ing or array because of backwards compatibility reasons.
        */
       public function setProvider($provider);
    

    This sounds like a BC break for me. As a caller you can no longer be sure what you can, and there could be code breaking it. Can we do something about it?

  2. +++ b/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php
    @@ -19,6 +19,11 @@ class AnnotatedClassDiscovery implements DiscoveryInterface {
       /**
    +   * Whether the parser should return only wants class annotations.
    +   */
    

    Is this actual english?

  3. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
    @@ -277,19 +278,7 @@ protected function findDefinitions() {
    +    return ProviderFilterDecorator::filterDefinitions($definitions, function ($provider) { return $this->providerExists($provider); });
    

    Nitpick: Afaik we don't do this inline

  4. +++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
    @@ -0,0 +1,32 @@
    +    $finder = \Drupal::service('class_loader');
    

    Why can't we inject the class loader here?

  5. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
    @@ -0,0 +1,82 @@
    +      $definition = (array) $definition + ['provider' => []];
    +      $providers = (array) $definition['provider'];
    

    Can we document why we need this casting to array here? This is probably because plugin definitions can be arrays as well

  6. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
    @@ -0,0 +1,82 @@
    +      return count($providers) == count(array_filter($providers, $provider_exists));
    

    IMHO using array_diff would be more semantic here. An alternative could be to provide a $provider_not_exists and check whether the result is not empty.

  7. +++ b/core/lib/Drupal/Core/Plugin/Discovery/StaticReflectionParser.php
    @@ -0,0 +1,27 @@
    +
    +namespace Drupal\Core\Plugin\Discovery;
    ...
    + * Allows getting the reflection parser for the parent class.
    + */
    +class StaticReflectionParser extends BaseStaticReflectionParser {
    

    This could be moved to the plugin component, couldn't it?

mikeryan’s picture

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
@@ -0,0 +1,108 @@
+    // Enable migrate_drupal to test that the plugins can now be discovered.
+    $this->enableModules(['migrate_drupal']);
+    // Set up a migrate database connection so that plugin disocovery works.
...
+    Database::addConnectionInfo('migrate', 'default', $connection_info['default']);

s/disocovery/discovery/

Let's also test with migrate_drupal enabled and no database connection (yet) configured.

alexpott’s picture

I agree with @dawehner wrt to #164.1. Imo we should add new methods on a new interface something like getProviders() and setProviders(). It should be documented that getProvider() returns the "main" provider. Main is something that a plugin manager with allows plugins to have multiple providers can decide.

generalredneck’s picture

This is a reroll against 8.1.x-dev as some updates made the patch quit applying over there as of 8.1.8.

patching file core/lib/Drupal/Component/Annotation/AnnotationInterface.php
patching file core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php
Hunk #1 succeeded at 18 (offset -1 lines).
Hunk #2 FAILED at 142.
Hunk #3 succeeded at 149 (offset -29 lines).
1 out of 3 hunks FAILED -- saving rejects to file core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php.rej
patching file core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
Hunk #1 succeeded at 13 (offset -1 lines).
Hunk #2 succeeded at 277 (offset -1 lines).
patching file core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.php
patching file core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
patching file core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
patching file core/lib/Drupal/Core/Plugin/Discovery/StaticReflectionParser.php
patching file core/modules/block_content/migration_templates/block_content_body_field.yml
patching file core/modules/block_content/migration_templates/block_content_type.yml
patching file core/modules/migrate/migrate.services.yml
patching file core/modules/migrate/src/Plugin/MigrateSourcePluginManager.php
patching file core/modules/migrate/src/Plugin/MigrationPluginManager.php
patching file core/modules/migrate/src/Plugin/NoSourcePluginDecorator.php
patching file core/modules/migrate/tests/src/Kernel/MigrationTest.php
patching file core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
patching file core/modules/taxonomy/tests/modules/taxonomy_term_stub_test/migrations/taxonomy_term_stub_test.yml

Oh and corrected that fat fingered 4.
Please continue about your business in getting this in for 8.2... just need it for 8.1 at the moment. :)

chx’s picture

I am not a good judge of what needs to be done and what not. I have been long wary of handling this issue because the penultimate destination is adding a provider and a providers key to the definition and I so badly don't want that. So I would rather do nothing. I changed getProvider to forcibly return a single provider always and marked setProvider it accepts array or string. It doesn't matter much anyways because the plugin subsystem classes work on the definition directly anyways which nothing else should do anyways so while there is a minimal amount of BC break here it's in an internal array structure for which an API getter exists and that doesn't change. Let it just be.

> Let's also test with migrate_drupal enabled and no database connection (yet) configured.

That's a different issue.

> An alternative could be to provide a $provider_not_exists and check whether the result is not empty.

I had that in previous patches. It's unreadable. This is the most readable.

Status: Needs review » Needs work

The last submitted patch, 168: 2560795_168.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
26.37 KB
11.05 KB

Disregard. I tried to typehint the classloader. LOL.

chx’s picture

FileSize
26.34 KB

With less typos causing fatals.

The last submitted patch, 170: 2560795_170.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 171: 2560795_171.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
26.25 KB

One more type hint. I only removed it from the class doxygen and forgot the code itself. Another good reason why I hate, hate, hate to write doxygen like this

   * @param $class_loader
   *   The class loader.

That's just a burden.

Hope it's now all done. And now the issue is cluttered even more and noone will find my writeup above. Excellent. Love pointless doxygen.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Component/Annotation/Plugin.php
    @@ -78,7 +78,9 @@ public function get() {
    +      return is_array(isset($this->definition['provider'])) ? reset($this->definition['provider']) : $this->definition['provider'];
    

    is_array(isset())?! isset() returns boolean, so this will always evaluate to FALSE. Unless I'm missing something?

    Also, this should return FALSE if $this->definition['provider'] is not set, to maintain BC.

  2. +++ b/core/lib/Drupal/Component/Plugin/Discovery/StaticReflectionParser.php
    @@ -0,0 +1,27 @@
    +
    +  /**
    +   * If the current class extends another, get the parser for the latter.
    +   *
    +   * @param \Doctrine\Common\Reflection\StaticReflectionParser $parser
    +   *   The current static parser.
    +   *
    +   * @return static|null
    +   *   The static parser for the parent if there's a parent class or NULL.
    +   */
    +  public static function getParentParser(BaseStaticReflectionParser $parser, $finder) {
    

    Missing @param documentation for $finder.

  3. +++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
    @@ -0,0 +1,63 @@
    +  /**
    +   * Any class loader with a findFile() method.
    +   *
    +   * @var \Composer\Autoload\ClassLoader
    +   */
    +  protected $finder;
    

    Nitpick, but if this is a class loader, can it be called $classLoader instead of the more ambiguous $finder?

  4. +++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
    @@ -0,0 +1,63 @@
    +  public function __construct($subdir, \Traversable $root_namespaces, $plugin_definition_annotation_name, $annotation_namespaces, $class_loader) {
    

    $annotation_namespaces and $class_loader should be type hinted.

  5. +++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
    @@ -0,0 +1,63 @@
    +    $this->finder = $class_loader;
    

    If this is required to have a findFile() method, can we add a method_exists() sanity check here (and throw LogicException if the class loader doesn't implement findFile()) to prevent bugs from originating further up the call chain?

  6. +++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
    @@ -0,0 +1,63 @@
    +    do {
    +      $new_providers = array_map([$this, 'getProviderFromNamespace'], $parser->getUseStatements());
    +      $providers = array_merge($providers, $new_providers);
    +    } while ($parser = StaticReflectionParser::getParentParser($parser, $this->finder));
    

    This strikes me as a bit magical, can there be a comment explaining the while() condition at least?

  7. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
    @@ -0,0 +1,84 @@
    +  /**
    +   * @var callable
    +   */
    +  protected $providerExists;
    

    Missing description.

  8. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
    @@ -0,0 +1,84 @@
    +  public function __call($method, $args) {
    

    Nitpick, but I think $args should be type hinted for clarity's sake.

  9. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
    @@ -0,0 +1,84 @@
    +    return call_user_func_array(array($this->decorated, $method), $args);
    

    Please change array() syntax to [] for consistency in the same file.

  10. +++ b/core/modules/migrate/src/Plugin/MigrateSourcePluginManager.php
    @@ -0,0 +1,61 @@
    +   * @param $class_loader
    +   *   The class loader.
    

    This should have a type hint, and so should the parameter.

  11. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -68,7 +69,15 @@ protected function getDiscovery() {
    +      // This gets rid of migration which try to use a non-existing source
    

    Should be migrations, plural.

  12. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -68,7 +69,15 @@ protected function getDiscovery() {
    +      // non-existing provider like migrate_drupal.
    

    migrate_drupal *does* exist in core, so let's change the wording here; maybe something like "in case a plugin specifies a non-existent provider."

  13. +++ b/core/modules/migrate/src/Plugin/NoSourcePluginDecorator.php
    @@ -0,0 +1,50 @@
    +   * Constructs a InheritProviderDecorator object.
    

    Wrong class name.

  14. +++ b/core/modules/migrate/src/Plugin/NoSourcePluginDecorator.php
    @@ -0,0 +1,50 @@
    +    /** @var \Drupal\Component\Plugin\PluginManagerInterface $source_plugin_manager */
    +    $source_plugin_manager = \Drupal::service('plugin.manager.migrate.source');
    

    Why can't this be injected?

  15. +++ b/core/modules/migrate/src/Plugin/NoSourcePluginDecorator.php
    @@ -0,0 +1,50 @@
    +  public function __call($method, $args) {
    

    $args should be type hinted.

  16. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
    @@ -0,0 +1,108 @@
    + * Tests the migration manager plugin.
    

    Er...how about "Tests the default Migrate plugin manager"?

  17. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
    @@ -0,0 +1,108 @@
    +   * Tests MigratePluginManager::getDefinitions()
    

    Effectively repeats the @covers annotation, so might as well remove this line.

  18. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
    @@ -0,0 +1,108 @@
    +    // All the plugins provided by migrate_external_translated_test do depend
    +    // not on migrate_drupal.
    

    Should say "do not depend". Also, how about adding an assertCount()?

  19. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
    @@ -0,0 +1,108 @@
    +    // Set up a migrate database connection so that plugin disocovery works.
    

    Discocovery? :)

chx’s picture

Assigned: chx » Unassigned

Unfollowed.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
27.33 KB
9.46 KB

Fixed most of my nitpicks.

Status: Needs review » Needs work

The last submitted patch, 178: 2560795-178.patch, failed testing.

phenaproxima’s picture

Hopefully this will fix the failures. It turns out you shouldn't type hint a class loader, because there are many different class loaders that could be used (and none of them implement a single interface). For the record, my foot tastes great.

Status: Needs review » Needs work

The last submitted patch, 180: 2560795-180.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review

No it didn't. We just got whacked by inexplicable CI errors.

dpalmer’s picture

Subscribed

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php
    @@ -138,11 +143,11 @@ public function getDefinitions() {
    -              $parser = new StaticReflectionParser($class, $finder, TRUE);
    +              $parser = new StaticReflectionParser($class, $finder, static::CLASS_ANNOTATION_OPTIMIZE);
    

    Here are we creating a doctrine class or a core class... because now there's a class with this name in this namespace. This has cause us issues in the plugin system in the past.

  2. +++ b/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php
    @@ -138,11 +143,11 @@ public function getDefinitions() {
    -                $this->prepareAnnotationDefinition($annotation, $class);
    +                $this->prepareAnnotationDefinition($annotation, $class, $parser);
    
    @@ -173,8 +178,10 @@ public function getDefinitions() {
    -  protected function prepareAnnotationDefinition(AnnotationInterface $annotation, $class) {
    +  protected function prepareAnnotationDefinition(AnnotationInterface $annotation, $class, StaticReflectionParser $parser) {
    

    This is an API change. Component classes exist to be extended. Whilst it is neat to re-use all this code perhaps we should not. Of course not re-using the code introduces risks too.

  3. +++ b/core/lib/Drupal/Component/Plugin/Discovery/StaticReflectionParser.php
    @@ -0,0 +1,32 @@
    +   *   The class finder. Must implement
    +   *  \Doctrine\Common\Reflection\ClassFinderInterface, but can do so
    +   *  implicitly (i.e., implements the interface's methods but not the actual
    +   *  interface).
    

    Weird indentation.

  4. +++ b/core/lib/Drupal/Component/Plugin/Discovery/StaticReflectionParser.php
    @@ -0,0 +1,32 @@
    +      return new static($parser->parentClassName, $finder, $parser->classAnnotationOptimize);
    

    It's weird we have access to classAnnotationOptimize here - i guess that is because we're in a sub class... still odd.

  5. There seems to be only tests of the migrate changes - but not the core changes without the migrate changes.
phenaproxima’s picture

  1. I'm not sure I understand what you mean. Should the class be renamed?
  2. Would it be preferable to make $parser optional, then create it in the method if it's not passed?
  3. It was indented that way because if the class name is on the same line, the whole line exceeds 80 characters.
  4. Discussed with @benjy on IRC. I think you're right, the only reason we can get to it is because we're in a subclass -- so the code is taking advantage of a PHP quirk. Personally I'm OK with that, but I suppose it could be potentially dangerous in weird edge cases. Is it worth addressing now?
  5. @neclimdul raised this very question on IRC. Because Migrate really needs this issue to be fixed, is it acceptable to have a follow-up issue to add explicit tests -- @neclimdul has just such an issue open, and it's already a year and a half old, but I don't have the issue number offhand -- and commit this implicit test now?
chx’s picture

Hey, look a ClassFinderInterface! I didn't know that existed... oh.

> Would it be preferable to make $parser optional, then create it in the method if it's not passed?

You can make it optional but you can't recreate it in the method as you do not have the data necessary as far as I am aware. So if you make it optional, you need to throw http://php.net/manual/en/class.invalidargumentexception.php

> so the code is taking advantage of a PHP quirk.

http://www.phpwtf.org/php-protected-and-assault I am not even sure this is PHP specific.

> Personally I'm OK with that, but I suppose it could be potentially dangerous in weird edge cases. Is it worth addressing now?

Nope. This whole shebang should be addressed upstream and that's not "now".

> For the record, my foot tastes great.

It took me four tries to fix the same so you are much ahead and meanwhile I realized why they don't implement interfaces: what would load the interface that the class loader implements? :)

dawehner’s picture

@neclimdul raised this very question on IRC. Because Migrate really needs this issue to be fixed, is it acceptable to have a follow-up issue to add explicit tests -- @neclimdul has just such an issue open, and it's already a year and a half old, but I don't have the issue number offhand -- and commit this implicit test now?

How do even people develop code without proving along the way that things is working. I guess some people just use testing as an afterthought.
This issue is marked against 8.3.x, so I don't see why getting this patch is as soon as possible, is a requirement or so.

phenaproxima’s picture

It took me four tries to fix the same so you are much ahead and meanwhile I realized why they don't implement interfaces: what would load the interface that the class loader implements? :)

Touché. It'd probably need to be an interface built into PHP's core, based on a PSR standard. IMHO PHP should have such an interface, and while I'm dreaming I'd like a pet dragon.

mikeryan’s picture

Version: 8.3.x-dev » 8.2.x-dev

It was automatically bumped to 8.3.x, but we really want to get this fixed in 8.2.x and not wait yet another 6 months.

alexpott’s picture

Re #184.1 What I'm saying is that we should alias Doctrine's StaticReflectionParser in core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php because AnnotatedClassDiscovery is in Drupal\Component\Plugin\Discovery and we're adding Drupal\Component\Plugin\Discovery\StaticReflectionParser.

Re #184.3 I meant the rest of the comment.

I think if we want to get this in 8.2.x we need to make the changes to core's plugin system as minimal as possible. The changes to core/lib/Drupal/Component/Annotation/Plugin.php and core/lib/Drupal/Component/Annotation/AnnotationInterface.php are fine and easy to see there's no impact (still need a CR though). Whereas the other changes are tricker. I'd be in favour of trying to do this in migrate and then having a issue to move the functionality into core's plugin system for a minor (or major release).

alexpott’s picture

The patch attached just moves the customisation to the plugin system into the experimental migrate module and marks them internal because even if migrate gets to beta we might remove in 8.3.0 if we get around to #2786355: Move multiple provider plugin work from migrate into core's plugin system.

It'd be nice to add explicit unit tests of:

  • core/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
  • core/modules/migrate/src/Plugin/Discovery/ProviderFilterDecorator.php
  • core/modules/migrate/src/Plugin/Discovery/StaticReflectionParser.php

Since atm they only have implicit coverage.

No interdiff because it would be bigger then the patch - much of the code has be moved around.

alexpott’s picture

Title: Make the plugin system even more awesome and get migrate to ride that unicorn » Make migrate ride a sparkly unicorn showing the plugin system the future
mikeryan’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/Annotation/MigrateSource.php
    @@ -66,4 +66,20 @@ class MigrateSource extends Plugin {
    +   * Gets the name of the provider of the annotated class.
    

    I think it should be made clear that when there are multiple providers, this returns the first one.

  2. +++ b/core/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
    @@ -0,0 +1,138 @@
    +      throw new \LogicException('Class loader must implement findFile() method');
    

    Could we incorporate the class loader class name here?

  3. +++ b/core/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
    @@ -0,0 +1,138 @@
    +    $providers = (array) $annotation->getProvider();
    

    Doesn't getProvider() only return the first provider? We need a getProviders(), don't we?

alexpott’s picture

Status: Needs work » Needs review
FileSize
29.47 KB
5.42 KB

Addressing #193

phenaproxima’s picture

  1. +++ b/core/modules/migrate/src/Annotation/MigrateSource.php
    @@ -82,4 +76,21 @@ public function getProvider() {
    +    if (isset($this->definition['provider'])) {
    +      return is_array($this->definition['provider']) ? $this->definition['provider'] : [$this->definition['provider']];
    +    }
    +    return [];
    

    This can all be replaced with return (array) $this->definition['provider'].

    If the key is not set, it will cast to an empty array. If it's a string, it will be cast to [$provider].

  2. +++ b/core/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
    @@ -58,15 +59,21 @@ public function __construct($subdir, \Traversable $root_namespaces, $plugin_defi
    +      return $provider && $provider !== 'component';
    

    The return $provider bit essentially acts just like array_filter(). Let's either remove the call to array_filter() (my preference) or change this to return $provider !== 'component'.

chx’s picture

> If the key is not set, it will cast to an empty array.

and emit a warning. return is_array($this->definition['provider']) ? $this->definition['provider'] : [$this->definition['provider']]; this, however, can indeed be replaced.

alexpott’s picture

Addressing #195.1

I don;t get #195.2 that's a anonymous function being passed to array filter. array_filter() was originally added because that method called getProvider which returned FALSE if there was no provider yet - it does not anymore. But I left the truthiness check in because (a) there is utterly no harm and (b) I don't trust parsing code with regular expressions to never return an empty string... which is what StaticReflection parser does. Ah!!! @phenaproxima nice spot what we have here is unnecessary calls to array_unique and array_filter.

phenaproxima’s picture

This is great stuff, in my opinion. I would RTBC it if I could.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Yep, let's do this.

catch’s picture

This is going to need a couple of read throughs.

I found some nits on the first read through. I found myself wondering whether we really need to go to all this trouble, but the issue summary explains the problem well enough. It would be nice if the issue title explained the problem.

  1. +++ b/core/modules/migrate/src/Annotation/MultipleProviderAnnotationInterface.php
    @@ -0,0 +1,41 @@
    +   * Sets the name of the provider of the annotated class.
    

    Names of the providers?

  2. +++ b/core/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
    @@ -0,0 +1,145 @@
    +   *   The class loader already know where to find the classes so it is reused
    

    knows

  3. +++ b/core/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
    @@ -0,0 +1,145 @@
    +    if (!method_exists($class_loader, 'findFile')) {
    

    #2023325: Add interface for classloader so it can be cleanly swapped + adapters would be useful here, but not a blocker.

alexpott’s picture

Just addressing #200

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 201: 2560795-3-201.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Not so much...random CI failure.

catch’s picture

While the use statement parsing is clever, I don't like it very much. i.e. I wonder even with contrib whether the three lines of YAML it saves per plugin would add up to the amount of code added to support not doing that (which includes a double-nested foreach).

In irc alexpott mentioned it means contrib plugins don't need to be updated once the patch is in, so as a bc layer (kinda) that's nice, but it still doesn't sit right with me.

At a minimum the issue summary could use some more justification for this.

chx’s picture

I don't think the amount of code is large -- it's a lot of copy-paste ATM -- but in all honesty: whatever. This is my idea, take it or leave it. We can go on with manual it has been coded above. Whatever.

mikeryan’s picture

The originally committed patch at #101 passes tests for 8.1.x/8.2.x/8.3.x.

phenaproxima’s picture

For whatever my opinion is worth, I prefer @chx's solution.

@mikeryan's patch in #101 is the most expedient solution -- it is simple, it is small, it keeps the solution confined to Migrate, and it was already good enough to be committed once.

The reason I support @chx's way is because it is better long-term DX. If people have to declare a specific provider in their plugin definitions in order to get around cross-provider dependencies, and they forget to do it (or do it incorrectly), they are liable to encounter all manner of crazy-making, hard-to-debug devilry. @chx's patch utterly prevents that and makes plugin discovery smarter in general. Although it adds a substantial amount of extra code, in my opinion it is an overall win for the plugin system.

I would also like to see the words "sparkly unicorn" in core, so there's that.

mikeryan’s picture

Just for the record, I don't really care at this point which of the two patches goes in - I just care that one of them gets in soon (in time for 8.2.0-rc).

catch’s picture

To elaborate on #204. It seems possible that someone defines their classes using FQCN instead of use statements - that's against coding standards, but it's not illegal. If they do that, their code will not work. The only way to find out why would be to fix the code style (at which point it magically works), or somehow track it down to this code.

I realise that forgetting to add the YAML values is more likely, but at least then your actual code is wrong/incomplete vs. it not working just because it's a bit messy.

chx’s picture

Do note + $providers = $annotation->getProviders(); please. If you choose to list your providers manually, be my guest. That's still an open avenue and an easy out if things broke.

phenaproxima’s picture

I agree with @catch that handling FQCNs is important. However, I think that most Drupal programmers are going to be using use statements most of the time, so I think we should handle FQCNs in a follow-up issue filed against the plugin system.

mikeryan’s picture

To elaborate on #204. It seems possible that someone defines their classes using FQCN instead of use statements - that's against coding standards, but it's not illegal. If they do that, their code will not work.

Using FQCN is pretty edge-casey, and actually their code (in and of itself) will work, it's just that the provider would not be automatically recognized (so, no worse than forgetting an explicit provider if we took the approach of the first patch).

The last submitted patch, 101: source_plugins_have_a-2560795-101.patch, failed testing.

alexpott’s picture

Another thing in favour of the latest solution is if plugin A that is being extended by plugin B and plugin A is changed to have additional dependencies in the future then Plugin B will not have to be changed to include the additional (or changed) dependencies.

xjm’s picture

So @alexpott explained this issue to me at length. I think both HEAD and the handbook are missing a bit of documentation about how namespace and use statements are currently leveraged for discovering plugin dependencies, and I think this patch is also missing the documentation for how to use this more complete API relying on namespace and use statements within the Migrate system (both in the Migrate handbook and in the API docs).

Since Migrate is experimental this issue is not subject to the documentation gates; however, this documentation would be a required part of making Migrate stable so it's definitely in our interest to add it soon.

With this as the proposed resolution for the plugin discovery problem, how and where exactly developers place their use and namespace declarations becomes very important and impacts functional code within the plugin system.

xjm’s picture

Also it would be really great to give this issue a title that explains the issue.

xjm’s picture

Title: Make migrate ride a sparkly unicorn showing the plugin system the future » Source plugins have a hidden dependency on migrate_drupal
chx’s picture

alexpott’s picture

Looking at the patch and core's AnnotatedClassDiscovery I think we shouldn't use use statements. Core's AnnotatedClassDiscovery uses the namespace of the plugin class to determine the provider. What we should do here is use the namespaces of the parent classes and not look at the use statements. This will be way less prone to surprise than parsing use statements.

So this patch changes this:

Add an annotated class discovery variant which automatically discovers provider dependencies based on use statements (of its class and every class it extends).

to:

Add an annotated class discovery variant which automatically discovers provider dependencies based on namespace (of its class and every class it extends). Which is different from core that just looks at the namespace of the class and not its parents.

alexpott’s picture

Fixing some commentary.

chx’s picture

In your wisdom we are humbled. That is an amazing, sturdy idea.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

I am much more comfortable with that solution! It seems robust and will cover the fundamental problem.

I think we do still need some small bit of documentation here, at least in the Migrate topic docs (somewhere) and probably in the handbook. Also, we want a followup for adding this to core for 8.3.x, right?

NR for docs.

xjm’s picture

Issue summary: View changes
alexpott’s picture

Here's some docs.

phenaproxima’s picture

I also really like the idea of dealing with the parent class rather than all the use statements, but let us not forget to handle the interfaces implemented by the plugin class...not sure how StaticReflectionParser handles those, but they will also need to be part of the discovered providers.

alexpott’s picture

@phenaproxima core's AnnotatedClassDiscovery does not look at interfaces. If we need to do this then for me this is a separate issue. We don't need to do that to fix the problem here. And if the problem was widespread I think we'd be seeing issues around this for core's AnnotatedClassDiscovery provider determination.

phenaproxima’s picture

@alexpott: As we discussed on IRC, I am inclined to agree. In Migrate's case, interface handling is an edge case and we don't need to deal with it now. When this functionality moves into the general plugin system, I think we'll need to give it more thought, because I think pulling parent classes and interfaces from different providers is much less of a zany edge case in the wild. That said, I think you're right that we would be seeing complaints about it if it were a widespread problem...but what can I say? I believe in programming defensively, and knowing that there is this invisible line in the sand makes me nervous because we know that if two things exist, someone will eventually find a need to use them together. And when they do, it will break.

Having said my piece, I think we can proceed with what we've got. It's still a great solution to the immediate problem.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Docs have been written.

  • catch committed 4bd0cdc on 8.3.x
    Issue #2560795 by chx, alexpott, mikeryan, phenaproxima, penyaskito,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

xjm’s picture

Issue tags: +8.2.0 release notes
hass’s picture

Can someone write a understandable change record with examples, please? I do not understand when I need to set this in the migration, why I need to set it. A before / after example would also be helpful to understand it.

xjm’s picture

Title: Source plugins have a hidden dependency on migrate_drupal » [Needs CR] Source plugins have a hidden dependency on migrate_drupal
Status: Fixed » Needs work
Issue tags: +Needs change record

A change record is a good idea. While he point of this issue is actually that the developer shouldn't need to do anything in most cases (because plugins should always be dependent on the namespace that provides the parent), I didn't entirely get it until someone explained it to me (twice). So it can be a simple CR explaining that Migrate source plugins now automatically depend on the providers for their parents' namespaces.

I can't think of a scenario where a source plugin would need to manually implement the API, but if there is one, we could include an example of that as well.

Thanks @hass!

mikeryan’s picture

Pending an updated change record, I'd just like to point out to address @hass's concern - the existing draft change record was associated with a previous version of the patch, not the final solution (which automated provider detection for cases where the lack of provider would cause errors). You don't need to change anything in your migrations to prevent these plugin errors - it'd be nice to add an explicit provider if you have a Drupal migration which does not depend on migrate_drupal's DrupalSqlBase plugin (like, for example, block_content_body_field), but this patch will not break any existing migrations.

mikeryan’s picture

Title: [Needs CR] Source plugins have a hidden dependency on migrate_drupal » Source plugins have a hidden dependency on migrate_drupal
Status: Needs work » Needs review
Issue tags: -Needs change record

I've updated the draft change record.

mikeryan’s picture

So... Unfortunately, I reviewed the source code only, and did not test it in its actual context until tackling #2752335: Properly integrate configuration-entity-based migrations with the core plugin manager. This does not actually work in real life, although somehow the test does.

Test:

    $migration_plugins = $this->container->get('plugin.manager.migration')->getDefinitions();

Reality:

8.2.x$ drush ev '\Drupal::service("plugin.manager.migration")->getDefinitions()'
PHP Fatal error:  Class 'Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase' not found in /Users/mikeryan/Sites/8.2.x/core/modules/action/src/Plugin/migrate/source/Action.php on line 16
PHP Stack trace:
...
PHP  13. Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() phar:///usr/local/bin/drush/commands/core/core.drush.inc(1162) : eval()'d code:1
PHP  14. Drupal\migrate\Plugin\MigrationPluginManager->findDefinitions() /Users/mikeryan/Sites/8.2.x/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:175
PHP  15. Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions() /Users/mikeryan/Sites/8.2.x/core/modules/migrate/src/Plugin/MigrationPluginManager.php:251
PHP  16. Drupal\migrate\Plugin\Discovery\ProviderFilterDecorator->getDefinitions() /Users/mikeryan/Sites/8.2.x/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php:85
PHP  17. Drupal\migrate\Plugin\NoSourcePluginDecorator->getDefinitions() /Users/mikeryan/Sites/8.2.x/core/modules/migrate/src/Plugin/Discovery/ProviderFilterDecorator.php:81
PHP  18. array_filter(*uninitialized*, *uninitialized*) /Users/mikeryan/Sites/8.2.x/core/modules/migrate/src/Plugin/NoSourcePluginDecorator.php:40
PHP  19. Drupal\migrate\Plugin\NoSourcePluginDecorator->Drupal\migrate\Plugin\{closure}($definition = *uninitialized*) /Users/mikeryan/Sites/8.2.x/core/modules/migrate/src/Plugin/NoSourcePluginDecorator.php:40
PHP  20. Drupal\Core\Plugin\DefaultPluginManager->hasDefinition($plugin_id = *uninitialized*) /Users/mikeryan/Sites/8.2.x/core/modules/migrate/src/Plugin/NoSourcePluginDecorator.php:39
PHP  21. Drupal\Core\Plugin\DefaultPluginManager->getDefinition($plugin_id = *uninitialized*, $exception_on_invalid = *uninitialized*) /Users/mikeryan/Sites/8.2.x/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:59
PHP  22. Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() /Users/mikeryan/Sites/8.2.x/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryCachedTrait.php:22
PHP  23. Drupal\migrate\Plugin\MigrateSourcePluginManager->findDefinitions() /Users/mikeryan/Sites/8.2.x/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:175
PHP  24. Drupal\Core\Plugin\DefaultPluginManager->processDefinition($definition = *uninitialized*, $plugin_id = *uninitialized*) /Users/mikeryan/Sites/8.2.x/core/modules/migrate/src/Plugin/MigrateSourcePluginManager.php:75
PHP  25. is_subclass_of(*uninitialized*, *uninitialized*) /Users/mikeryan/Sites/8.2.x/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:253
PHP  26. spl_autoload_call(*uninitialized*) /Users/mikeryan/Sites/8.2.x/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:253
PHP  27. Symfony\Component\ClassLoader\ApcClassLoader->loadClass($class = *uninitialized*) /Users/mikeryan/Sites/8.2.x/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:253
PHP  28. require() /Users/mikeryan/Sites/8.2.x/vendor/symfony/class-loader/ApcClassLoader.php:110

We find the problem in DefaultPluginManager::processDefinition():

    if (!isset($definition['forms']['configure']) && isset($definition['class']) && is_subclass_of($definition['class'], PluginFormInterface::class)) {

We are looking here at the Action source plugin - $definition['class'] is 'Drupal\action\Plugin\migrate\source\Action', which of course inherits from DrupalSqlBase, which (without migrate_drupal installed) fails to autoload in the call to is_subclass_of().

So, is there an answer to this that can be a quickfix, rather than revert and try again (the existing patch doesn't seem to do any active harm)? Like, override processDefinition() to omit the offending form business? Well, probably not that, but open to suggestions...

mikeryan’s picture

Just a quick side note - the test only tests getDefinitions(), not createInstances(). I ran into this using createInstances() and spent a while trying to figure out why that would break when getDefinitions() doesn't, until discovering getDefinitions() is broken... Probably the fix will fix both, but it would be good to test both just to be safe.

Probably the first thing to do is figure out how the test passes, and "fix" it to fail...

phenaproxima’s picture

Let me begin by saying that this is not by any means a long-term solution. That said, here's what I propose as a quick fix.

DefaultPluginManager does this:

if (!isset($definition['forms']['configure']) && isset($definition['class']) && is_subclass_of($definition['class'], PluginFormInterface::class)) {

Let's take advantage of that first isset() to dodge the call to is_subclass_of(). We can have MigratePluginManager override processDefinition(), then in our overridden version, we can set $definition['forms']['configure'] to FALSE before calling the parent method.

I also suggest we open a follow-up issue for this. I think this issue has dragged on long enough and risks becoming a zombie.

chx’s picture

The trivial fix is to fix this "upstream" by adding a class_exists. If it doesn't exist then it can't be a subclass of so no BC break.

Berdir’s picture

class exists of what? the problem is that the parent class of something doesn't exist, we don't even know what that is before we load it.

This is also not really a problem of this issue, contrib has had troubles with that check since that was committed (pretty new in 8.2), see https://www.drupal.org/node/2796827, search_api had to create a new submodule and so on.

phenaproxima’s picture

@Berdir is correct -- it seems this is affecting more than just us. The main effort to fix it is going on in #2796953: [regression] Plugins extending from classes of uninstalled modules lead to fatal error during discovery. If we want to quick-fix this, well...I don't see any short-term problem with the approach in #238, since no Migrate plugin is user-configurable as yet.

mikeryan’s picture

Core issue is #2796953: [regression] Plugins extending from classes of uninstalled modules lead to fatal error during discovery, it's critical so hopefully should get quick action, it probably doesn't make sense to pursue a quickfix here. However, we should figure out why the tests fail to fail, I'll open a followup...

mikeryan’s picture

mikeryan’s picture

mpp’s picture

Status: Needs review » Needs work

The patch no longer applies:

  - Installing drupal/core (8.2.x-dev 42102ad)
    Cloning 42102ad7ae8365e2922f15ebe806252f4f37d2b9 from cache

  - Applying patches for drupal/core
    https://www.drupal.org/files/issues/2560795-3-224.patch (Source plugins have a hidden dependency on migrate_drupal)
   Could not apply patch! Skipping.

I'll add migrate_drupal as a dependency until this issue is resolved.

mikeryan’s picture

The patch has been committed to core (see #229 above), thus won't apply. It's still open for review of the change record.

xjm’s picture

Status: Needs work » Fixed

I published the CR after a quick skim. Does this need to be reverted for comments #236 on? If not, let's continue to discuss in followups. Thanks!

mikeryan’s picture

No need to revert - the actual bug (#2796953: [regression] Plugins extending from classes of uninstalled modules lead to fatal error during discovery) in the way of this working fully as expected is elsewhere, and it's still no worse than it was before.

Status: Fixed » Closed (fixed)

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