Problem/Motivation

As argued in #2462233: Migrations should not use the configuration entity system migrations should not use the config system. Now I agree they should not be config but instead should be plugins. Note the original issue did not suggest plugins, this was an idea of phenaproxima which I now consider doable based on a new, more complete understanding of the plugin system I've acquired during the long and hard debugging of #2605684: Routing silently fails in kernel tests.

Proposed resolution

Make them plugins. The current YML files themselves become a plugin each via a custom discovery. UI and code provided migrations still go into the config active store and retrieved as plugin derivatives.

Benefits of approach

  • No more templates, custom to the Migrate API and hard for others to understand.
  • No more builders, this now uses derivates like other parts of core. E.g. menu blocks.
  • No more config objects but still a simplified YAML file to describe migrations.

Remaining tasks

  1. Add back a query system. This will be done in a followup. The config entity query is easy to extend for this -- only the loadRecords method needs to be overridden.
  2. Dependency/requirements handling is also a followup.

User interface changes

API changes

Migration entity is gone so Migration::create and Migration::load calls need to be converted. Migration::create($config) in tests becomes simply new Migration([], uniqid(), $config); and the use statement needs to change from Entity to Plugin. Migration::load($id) becomes \Drupal::service('plugin.manager.migration')->createInstance($id). Once you have a Migration object -- now a plugin -- the methods are the same. A followup will make it necessary to change the use statement for the MigrationInterface as it will also move from Entity into Plugin.

For code provided migrations here's an example from MigrateTaxonomyTermStubTest:
Before:

    $term_migration = Migration::create($config);
    $term_migration->save();

After:

    $term_migration = MigrationConfigDeriver::save('terms', $config);

There is no create because, well, there's nothing save-able to create.

Data model changes

Files: 
CommentFileSizeAuthor
#168 2625696-6-168.patch193.19 KBalexpott
#159 interdiff.txt1.92 KBbenjy
#159 2625696-159.patch193.5 KBbenjy
#157 interdiff.txt3.61 KBbenjy
#157 2625696-156.patch193.11 KBbenjy
#155 interdiff.txt8.23 KBbenjy
#155 2625696-155.patch192.42 KBbenjy
#148 2625696-5-148.patch191.47 KBalexpott
#146 interdiff.txt1.51 KBbenjy
#146 2625696-146.patch191.37 KBbenjy
#143 2625696-4-143.patch193.02 KBalexpott
#143 141-143-interdiff.txt3.17 KBalexpott
#141 interdiff.txt811 bytesbenjy
#141 2625696-141.patch190.94 KBbenjy
#139 interdiff.txt9.83 KBbenjy
#139 2625696-139.patch190.93 KBbenjy
#138 2625696-4-138.patch190.44 KBalexpott
#138 136-138-interdiff.txt17.83 KBalexpott
#136 2625696-4-136.patch188.63 KBalexpott
#136 135-135-interdiff.txt8.76 KBalexpott
#135 interdiff.txt7.78 KBbenjy
#135 2625696-135.patch186.49 KBbenjy
#132 interdiff.txt3.65 KBbenjy
#132 2625696-132.patch188.32 KBbenjy
#127 interdiff.txt3.17 KBbenjy
#127 2625696-127.patch185.59 KBbenjy
#126 migrate-upgrade-ui.png64.14 KBbenjy
#125 files-table.png190.48 KBbenjy
#119 2625696-119.patch184.58 KBbenjy
#119 interdiff.txt2.23 KBbenjy
#116 2625696-3-116.patch183.32 KBalexpott
#116 114-116-interdiff.txt719 bytesalexpott
#114 112-114-interdiff.txt2.27 KBalexpott
#114 2625696-3-114.patch182.4 KBalexpott
#112 2625696-3-110.patch181.39 KBalexpott
#112 110-112-interdiff.txt1.17 KBalexpott
#110 interdiff.txt16.01 KBbenjy
#110 2625696-108.patch181.18 KBbenjy
#107 interdiff.txt1.16 KBbenjy
#107 2625696-107.patch172.44 KBbenjy
#106 interdiff.txt20.92 KBbenjy
#105 2625696-106.patch171.28 KBbenjy
#103 2625696-103.patch153.07 KBcatch
#92 2625696-2-92.patch153.2 KBalexpott
#92 84-92-interdiff.txt8.81 KBalexpott
#90 84-90-interdiff.txt6.2 KBalexpott
#90 2625696-2-90.patch153.32 KBalexpott
#87 2625696-2-87.patch153.26 KBalexpott
#87 84-87-interdiff.txt6.48 KBalexpott
#84 2625696-84.patch152.7 KBalexpott
#84 83-84-interdiff.txt5.83 KBalexpott
#83 2625696_83.patch155.26 KBchx

Comments

chx created an issue. See original summary.

chx’s picture

Issue summary: View changes
dawehner’s picture

IMHO this is a 8.1.x change, because well, 8.0.x is not meant to be for big changes. We have 5 months which is not a long time anyway, but more important, I think we all just care about getting something done, not whether its available directly.

phenaproxima’s picture

A guarded +1 for this. I wrote some proof-of-concept code on GitHub to show that we can keep our existing YAML files as-is while quietly leveraging the plugin system and moving away from entities. I'm glad the builder system will be gone -- it was always a hack, and it solves the same problem that derivers do.

Overall I think this will allow us to shed a lot of existing code and make the migration system more understandable. @EclipseGc originally gave me the idea :)

catch’s picture

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

I also think it's 8.1.x.

The only caveat is it'd be good to ensure that migration source and destinations between modules don't need any changes between 8.0.x and 8.1. so that modules only have to port once.

For actual migrations I'm more concerned with not breaking early use of migrations on 8.0.x in patch releases than keeping 8.0.x and 8.1.x in sync.

chx’s picture

Yes, definitely 8.1.x filing under 8.0.x was a mistake. However, upon more reviewing I found that keeping BC is a bit harder than expected -- while the interfaces I looked at yesterday for source and destination doesn't use the migration (mostly), the base class constructors do.

Based on a discussion with alexpott, we will add a MigrationInterface to the Plugin namespace and have a @deprecated one in Entity extend it and implement the ConfigEntityInterface by throwing an exception on save and such.

Gábor Hojtsy’s picture

Wait, isn't one of the points of experimental modules that we can still make bigger changes however we want? We did that before the 8 release to migrate out of bounds of whatever process state for the rest of core. Should experimental modules now be in the same process bounds as the rest of core? (Bojhan just proposed last weekend that experimental modules be used to get fast feedback on UX changes / additions in core. If they are bound to a 6 month release / change cycle like this issue suggests, then that does not sound possible to me).

catch’s picture

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

Contrib modules will hopefully be starting to write migration sources and destinations, and we know that people are using migrate in the wild to upgrade from 6.x-8.x regardless of experimental status.

Those are two of the main things that are going to ensure that when migrate stops being experimental, that people will be able to successfully upgrade sites - so we should try not to break them in patch releases.

So if the changes here come with a backwards compatibility layer, then I'd not have a problem committing them to 8.0.x - means less divergence and means that modules don't have to port twice. Then because it's @internal/experimental, we could remove that backwards compatibility layer in 8.1.x if we want to.

If we absolutely had to break things in a patch release by adding a method to an interface because it's necessary to fix a bug, then that's a different discussion to have. I also think new interfaces/modules that we might add as experimental are a very different category - there's not going to be an API exposed to contrib modules or the urgency to use them in production un the same way.

So moving back to 8.0.x for now - if there are changes that will actually break contrib modules and migrations we can split them out.

benjy’s picture

I spoke with chx about this when he started but I just wanted to comment publicly that i'm +1 here. I think removing the migration_templates which is a home grown migrate concept is a big win.

It would also be nice to get rid of builders although that might be more tricky. We'll likely have to go back to run-time generation of the sub-migrations, similar to how the load plugins used to work.

xjm’s picture

I disagree with committing this to 8.0.x; it completely breaks semver. We should commit the change to 8.1.x as soon as it's ready, but not to 8.0.x. The experimental designation is protection for the 8.0 to 8.1 upgrade, not a license to break semver in patch releases.

xjm’s picture

Just to reiterate "commit it to 8.1.x" does not mean "later". It means right now. 8.1.x is our development branch and we are committed to improving migrate right now.

phenaproxima’s picture

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

I'm in favor of this being an 8.1 target. Moving off the entity system is a big enough change to Migrate (mostly under the hood, to be sure, but still a major change) that it doesn't seem appropriate to me to put it into 8.0.x.

chx’s picture

well then, this is the question: may I move the interface out of entity breaking every migrate plugin out there? Fixing them is a trivial change in the use statement.

benjy’s picture

I think we should yes, if this is going into 8.1 we can just write a change record here with the upgrade steps required. Obviously we're trying to keep it simple but updating some use statements falls within that for me.

mikeryan’s picture

So, I'm skeptical of making this change, but want to keep an open mind - @chx, please update when your sandbox is stable, and I'll try updating migrate_plus/migrate_tools/migrate_upgrade to use it and see how the DX is affected.

Specific notes:

We do lose the config entity query-ability. This does not seem to be such a hurdle as originally imagined; so far we did not need this ability for any UI, written or imagined.

We are using entityQuery() in migrate_tools, to filter on migration group (functionality provided by migrate_plus). And I for one am capable of imagining any number of query scenarios;P.

based on what core is doing I believe most everyone is shipping using templates

I hope that's not true! The template approach makes sense for the Drupal-to-Drupal case so we can, say, make multiple type-specific node migrations from one .yml file, but most custom migration scenarios should be putting migration configuration into config/install. See migrate_example on how this would normally work.

UI-wise and for BC again, we will pick up migrations from the active store probably with a deriver and an override mechanism

I'm a little vague on how migrations get into the active store in the first place, if not installed from config/install or building templates...

We'll likely have to go back to run-time generation of the sub-migrations...

That could work for the straight upgrade-as-is Drupal-to-Drupal case... But what about custom migrations? The builder creates concrete distinct migrations per content type, so you can then tweak field mappings and such for each migration.

chx’s picture

> We are using entityQuery() in migrate_tools, to filter on migration group (functionality provided by migrate_plus). And I for one am capable of imagining any number of query scenarios

This is very valuable information. I will look into this; last I checked config query used remarkable little of the config system -- for example, no schema information is used so extending it won't be hard.

> I hope that's not true!

I had many hopes with Drupal 8, most of them are dead.

> I'm a little vague on how migrations get into the active store in the first place,

The UI needs to save things somewhere. What about the config store? Or is it going to be the K-V store? I am wondering. Not sure yet.

> The builder creates concrete distinct migrations per content type, so you can then tweak field mappings and such for each migration.

What you mean simply is that the deriver idea of mine won't work but we need an override like some of the menu link systems because we need an override for plugins created by a deriver. That's fine.

benjy’s picture

We are using entityQuery() in migrate_tools

Yeah, migrate_ui is using that as well although I don't see why we can't just do what we did for migrate templates in MigrateTemplateStorage, that should be fine once we add some caching #2626488: MigrateTemplateStorage should implement some caching

mikeryan’s picture

The UI needs to save things somewhere. What about the config store? Or is it going to be the K-V store? I am wondering. Not sure yet.

Sure, a UI will save things... wherever. But what's the pattern for custom-developed migration modules? Instantiate the migration (in whatever store) in hook_install()? That's something we get for free now with config/install...

chx’s picture

> But what's the pattern for custom-developed migration modules? Instantiate the migration (in whatever store) in hook_install()?

Put the YAML files in [moduleroot/]migrations. End. (The sandbox currently picks them up from migration_templates for BC and easier development of this system.)

mikeryan’s picture

Ah, so the YAML files in the migrations directory are automatically installed, I didn't get that. Thanks.

chx’s picture

> Ah, so the YAML files in the migrations directory are automatically installed,

No, there is no such thing as installation. They are simply plugin definitions.

heddn’s picture

I'm very curious about the DX we get with this change. Are there examples yet that will show the before/after picture of what converting to plugins will introduce? I'm already seeing a lot of traffic on migrate_source_csv and want to be ahead of the game if things are going to change significantly.

dawehner’s picture

As far as I understand its basically moving all existing migrate.migration.yml files to a new subfolder, that's it.
Instead of then using config_devel or drush cedit or such, you'd just clear the plugin cache and have new versions of your migrations.

chx’s picture

Not even that, we can have the discovery pick up from the old templates directory too. In fact, that's what the current sandbox code does.

The only big change I see on the road ahead is the nuking of builder plugins in favor of derivers.

chx’s picture

Status: Active » Needs review
FileSize
99.75 KB

If this passes I will be surprised. But! I saw one test passing so let's see.

Status: Needs review » Needs work

The last submitted patch, 25: 2625696_25.patch, failed testing.

chx’s picture

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

Meh I was developing against 8.0.x , I wanted a bot run against that. I will move this back to 8.1 once the testing finished.

chx’s picture

Status: Needs work » Needs review
FileSize
99.75 KB

Status: Needs review » Needs work

The last submitted patch, 28: 2625696_25.patch, failed testing.

chx’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
FileSize
114.96 KB

Really annoying :( I rolled this patch with git diff origin/8.1.x > 2625696_30.patch surely it will apply.

Edit: bot fail due to https://dispatcher.drupalci.org/job/default/55864/ DCI_CoreBranch 8.0.x

Status: Needs review » Needs work

The last submitted patch, 30: 2625696_30.patch, failed testing.

chx’s picture

I wish there was a way to delete unnecessary comments.

chx’s picture

Status: Needs work » Needs review

The last submitted patch, 30: 2625696_30.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 32: 2625696_32.patch, failed testing.

chx’s picture

Issue summary: View changes
chx’s picture

Issue summary: View changes
chx’s picture

Issue summary: View changes
chx’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
122.83 KB
benjy’s picture

Nice work so far :)

  1. +++ b/core/modules/field/src/Tests/Migrate/d6/MigrateFieldTest.php
    @@ -98,7 +98,7 @@ public function testFields() {
    +    $migration = $this->container->get('plugin.manager.migration')->createInstance('d6_field');
    
    +++ b/core/modules/field/src/Tests/Migrate/d7/MigrateFieldTest.php
    @@ -116,7 +116,7 @@ public function testFields() {
    +    $migration = $this->container->get('plugin.manager.migration')->createInstance('d7_field');
    

    Seems this is a common action, maybe we should add a getMigration or similar helper to the test base class?

  2. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -81,7 +68,7 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
    -  protected $process = [];
    +  protected $process;
    

    bad merge.

  3. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -571,28 +602,13 @@ public function getMigrationDependencies() {
    +    // While normal plugins do not change their definitions on the fly, this
    +    // one does so accomodate for that.
    

    Interesting, I guess this is a side-effect of the migration properties changing. Certainly could be un-expected that a definition could change during the life of a plugin. Guess we don't have much choice?

    Also, s/accomodate/accommodate

  4. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -0,0 +1,92 @@
    +    $plugin_ids = preg_grep('/^' . $id . ':/', array_keys($this->getDefinitions()));
    

    Does the id need a preg_quote? Not sure if plugin ids can have hyphens in their names maybe?

  5. +++ b/core/modules/migrate/tests/src/Kernel/Entity/MigrationTest.php
    @@ -24,9 +24,11 @@ class MigrationTest extends KernelTestBase {
    -   * @covers ::calculateDependencies
    +   * @todo: this should be covers, fix when dependencies are fixed.
    +   * @-covers ::calculateDependencies
    ...
    +    return;
    

    debug code?

  6. +++ b/core/modules/migrate_drupal/migrate_drupal.module
    @@ -19,3 +22,41 @@ function migrate_drupal_help($route_name, RouteMatchInterface $route_match) {
    +function migrate_drupal_migration_plugins_alter(&$definitions) {
    ...
    +        $plugin_ids = ['d6_term_node:' . $source_vid, 'd6_term_node_revision:' . $source_vid];
    +        foreach ($plugin_ids as $plugin_id) {
    

    Ouch, hopefully this won't be a common pattern but maybe we could create a follow-up to discuss other solutions?

  7. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
    @@ -0,0 +1,115 @@
    +namespace Drupal\migrate_drupal\Plugin\migrate;
    +
    +
    

    Double space.

  8. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
    @@ -0,0 +1,115 @@
    +        catch (RequirementsException $e) {
    +          // Kill the rest of the method.
    +          $source_plugin = [];
    +        }
    

    Will this bubble up somewhere else?

  9. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/D6NodeDeriver.php
    @@ -0,0 +1,155 @@
    +    $source_plugin = static::getSourcePlugin('d6_field_instance');
    ...
    +      $source_plugin->checkRequirements();
    ...
    +      // Don't do anything; $fields will be empty.
    ...
    +      foreach (static::getSourcePlugin('d6_node_type') as $row) {
    

    We squash the requirement errors? Should we at least log these? If you had a schema mismatch, it would be hard to know?

    Also, we don't call checkRequirements on the d6_node_type source?

  10. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/D6NodeDeriver.php
    @@ -0,0 +1,155 @@
    +    catch (\Exception $e) {
    +
    +    }
    

    Can we comment what exceptions we're squashing here.

  11. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/D7NodeDeriver.php
    @@ -0,0 +1,111 @@
    +      foreach (D6NodeDeriver::getSourcePlugin('d7_node_type') as $row) {
    +        $node_type = $row->getSourceProperty('type');
    +        $values = $base_plugin_definition;
    +        $derivative_id = $node_type;
    ...
    +        if (isset($fields[$node_type])) {
    +          foreach ($fields[$node_type] as $field_name => $info) {
    

    All this looks very similar to the D6 version, shall we add a base class?

  12. +++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
    @@ -58,6 +58,7 @@ protected function loadFixture($path) {
    +    return;
    

    Debug?

  13. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6TestBase.php
    @@ -61,9 +59,10 @@ protected function migrateUsers($include_pictures = TRUE) {
    +        #$manager->createInstance($id)->delete();
    

    Not sure what this means to delete the plugin instance? But we should remove the comment or something?

  14. +++ b/core/modules/migrate_drupal/src/Tests/dependencies/MigrateDependenciesTest.php
    @@ -28,24 +27,26 @@ class MigrateDependenciesTest extends MigrateDrupal6TestBase {
    +    $migration_items = array('d6_comment', 'd6_filter_format', 'd6_node:page');
    +    // @TODO: fix when dependencies are working again.
    +    return;
    

    Debug code.

  15. +++ b/core/modules/migrate_drupal/src/Tests/dependencies/MigrateDependenciesTest.php
    @@ -67,8 +68,10 @@ public function testMigrateDependenciesOrder() {
    +    // @TODO: fix when dependencies are working again.
    +    return;
    

    and here.

  16. +++ b/core/modules/node/src/Tests/Migrate/d6/MigrateNodeTest.php
    @@ -52,6 +57,25 @@ public function testNode() {
    +    // Test that link fields are migrated.
    +    $this->assertIdentical('https://www.drupal.org/project/drupal', $node->field_test_link->uri);
    

    Thanks for bringing this test coverage back!

  17. +++ b/core/modules/user/src/Plugin/migrate/ProfileValues.php
    @@ -0,0 +1,49 @@
    +      catch (\Exception $e) {
    +      }
    

    Comments here would be good.

  18. +++ b/core/modules/user/src/Plugin/migrate/User.php
    @@ -0,0 +1,60 @@
    +      if (\Drupal::moduleHandler()->moduleExists('field')) {
    

    Can we inject this into the migrations now.

chx’s picture

A few quick notes as I have a flight to catch: most of those return; statements are not debug code and have a @todo attached, they are killed off until dependencies are added back. The one in MigrateDrupalTestBase::installMigrations is a tentative one before killing off the method (in a followup?): what does it mean to install a migration now? Nothing. That's not an operation we need to do. Migrations simply are.

Status: Needs review » Needs work

The last submitted patch, 39: 2625696_39.patch, failed testing.

chx’s picture

FileSize
122.71 KB
35.88 KB
chx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 43: 2625696_43.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
122.68 KB

Try #2.

Status: Needs review » Needs work

The last submitted patch, 46: 2625696_46.patch, failed testing.

mikeryan’s picture

I've added issues to POC this against my contrib modules: #2667366: Make migrate_plus work with Drupal 8.1.x, #2667368: Make migrate_tools work with Drupal 8.1.x, #2667370: POC of migrations-as-plugins. Hopefully, this coming weekend I'll have some time to see how it affects in particular migrate_plus (and the infamous beer example) and migrate_tools.

Could we have an explicit list of anticipated benefits in the issue summary?

Thanks.

chx’s picture

Status: Needs work » Needs review
FileSize
154.42 KB

Most tests should pass. Drupal\Tests\migrate\Unit\process\MigrationTest won't but then again I can't be bothered, I will write a new test once this is in.

Most bugs were due to preg_quoting the id revealing we still passed in :* when that wasnt supposed to happen and since :* unquoted merrily matched nothing it worked prior.

As for benefits, removing migration templates is the biggest win. Perhaps the only but that's enough. And of course we no longer pollute the config system. There's a MigrationConfigDeriver for UI based migrations saved into config.

chx’s picture

FileSize
154.86 KB

A few more :* fixed.

The last submitted patch, 49: 2625696_49.patch, failed testing.

chx’s picture

FileSize
154.35 KB

Hrm, that getMigration refactor didn't work out as planned. Here, this is better.

Status: Needs review » Needs work

The last submitted patch, 52: 2625696_52.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
154.29 KB
3.95 KB

Status: Needs review » Needs work

The last submitted patch, 54: 2625696_54.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
154.27 KB

This concludes the getMigration() refactor.

benjy’s picture

This is looking great, few doc fixes and problems noted below. Tried to ignore any existing problems in code that was just moved, looking forward to the follow-ups where we can get rid of many uses of \Drupal, seems we have a lot from the config entity approach.

  1. +++ b/core/modules/migrate/src/Entity/MigrationInterface.php
    @@ -99,6 +97,8 @@
    +  public function id();
    

    Missing docs.

  2. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -186,7 +186,7 @@ public function import() {
    -      $this->migration->checkRequirements();
    +      #$this->migration->checkRequirements();
    

    Should this be un-commented? Or, add a TODO?

  3. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -0,0 +1,614 @@
    + *
    

    Extra new line.

  4. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -0,0 +1,614 @@
    +   * Gets the plugin_id of the plugin instance.
    +   *
    +   * @return string
    +   *   The plugin_id of the plugin instance.
    +   */
    +  public function id() {
    

    We can just move this to the interface and inheritdoc as mentioned in 1

  5. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
    @@ -150,7 +149,7 @@ protected function prepareMigrations(array $id_mappings) {
    +      $this->migration = $this->container->get('plugin.manager.migration')->createInstance($migration);
    
    @@ -215,7 +218,7 @@ public function stopCollectingMessages() {
    +      $migration = $this->container->get('plugin.manager.migration')->createInstance($migration);
    

    We may as well use getMigration() where we can.

  6. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
    @@ -0,0 +1,114 @@
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigratePluginManager $cck_manager, PluginManagerInterface $migration_plugin_manager) {
    

    $migratio_plugin_manager is type-hinted with the wrong interface. We have a migrate specific one.

  7. +++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
    @@ -58,6 +58,7 @@ protected function loadFixture($path) {
    +    return;
    

    Add a todo here.

  8. +++ b/core/modules/taxonomy/src/Plugin/migrate/D6TermNodeDeriver.php
    @@ -0,0 +1,58 @@
    +    catch (\Exception $e) {
    +
    +    }
    

    Need a comment here.

  9. +++ b/core/modules/user/src/Plugin/migrate/User.php
    @@ -0,0 +1,60 @@
    +      catch (RequirementsException $e) {
    +      }
    

    Comment here as well please. Why do we hide the requirement failures.

chx’s picture

Issue summary: View changes
FileSize
126.66 KB
5.96 KB
chx’s picture

FileSize
126.58 KB

There was a $yaml_discovery->addTranslatableProperty('label', 'title_context'); left in the MigrationPluginManager -- the manager was strongly influenced (ahem) by MenuLinkManager and such but as we do not actually provide a context for title we do not need this.

benjy’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Spoke through a few more points on IRC, hard to review anymore with the excitement of getting rid of templates. My main concern was removing the dependency handling but chx plans on working on those right after this issue and the current patch is already big enough. RTBC from me.

benjy’s picture

Issue summary: View changes
andypost’s picture

chx’s picture

Do we need a change record? Unless you are writing a migrate UI which benjy and mikeryan do you don't need to do anything. Your migration YML can be the same, your source, process, destination plugins will still work. The builder plugins were never documented on https://www.drupal.org/node/2127611 . So ... why needs change record?

dawehner’s picture

There has been some discussion around using change records as some sort of replacement for changelog.txt. Given that maybe this issue is a big enough change to be mentioned?

alexpott’s picture

It'd be great to hear from @mikeryan again after the comment #48:

Hopefully, this coming weekend I'll have some time to see how it affects in particular migrate_plus (and the infamous beer example) and migrate_tools.

Especially as @mikeryan has been a supporter of keeping migrate in config.

BTW as maintainer of a lot of the config code - I'm super happy by this approach - I've not yet reviewed the code in detail.

chx’s picture

Issue tags: -Needs change record
FileSize
958 bytes
126.53 KB

I wrote a change record. I found something copied from MenuLinkManager which is not necessary so there's an tiny change attached but while at it I believe I found a doxygen bug in setCacheBackend, will discuss with the plugin people and file a documentation bug.

chx’s picture

FileSize
958 bytes

Let's try the interdiff again. Varnish went bonkers over my the previous attempt.

andypost’s picture

At least https://www.drupal.org/node/2527568 should be updated cos templates are useless now

andypost’s picture

Overall looks great, mostly nits but some require to be added to CR

  1. +++ b/core/modules/field/migration_templates/d6_field.yml
    @@ -2,9 +2,8 @@ id: d6_field
    -builder:
    -  plugin: d6_cck_migration
    -  cck_plugin_method: processField
    +class: Drupal\migrate_drupal\Plugin\migrate\CckMigration
    +cck_plugin_method: processField
    

    this should be added to change record as before-after

  2. +++ b/core/modules/field/src/Tests/Migrate/d6/MigrateFieldTest.php
    @@ -98,7 +98,7 @@ public function testFields() {
    -    $migration = Migration::load('d6_field');
    +    $migration = $this->getMigration('d6_field');
    
    +++ b/core/modules/field/src/Tests/Migrate/d7/MigrateFieldTest.php
    @@ -116,8 +115,9 @@ public function testFields() {
    -    $migration = Migration::load('d7_field');
    +    $migration = $this->getMigration('d7_field');
    

    is this changes in scope?

  3. +++ b/core/modules/file/src/Tests/Migrate/d7/MigrateFileTest.php
    @@ -34,8 +34,8 @@ protected function setUp() {
    -    /** @var \Drupal\migrate\Entity\MigrationInterface $migration */
    -    $migration = entity_load('migration', 'd7_file');
    +    /** @var \Drupal\migrate\Plugin\Migration $migration */
    +    $migration = $this->getMigration('d7_file');
    

    once there's no more entity_load() then change record should point a way to retrieve migrations

  4. +++ b/core/modules/migrate/migrate.services.yml
    @@ -26,3 +26,6 @@ services:
    +  plugin.manager.migration:
    +    class: Drupal\migrate\Plugin\MigrationPluginManager
    

    probably this one should be used to load them.
    Is there any helpers for?

  5. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -186,7 +186,8 @@ public function import() {
    -      $this->migration->checkRequirements();
    +      // @TODO https://www.drupal.org/node/2666640
    +      #$this->migration->checkRequirements();
    

    Why that code commented with hash?
    the related issue does not describe what to do

  6. +++ b/core/modules/migrate/src/Plugin/MigrationConfigDeriver.php
    @@ -0,0 +1,94 @@
    +  public static function save($migration_id, $definition, PluginManagerInterface $manager = NULL, ConfigFactoryInterface $config_factory = NULL) {
    +    if (!$manager) {
    +      $manager = \Drupal::service('plugin.manager.migration');
    +    }
    +    if (!$config_factory) {
    +      $config_factory = \Drupal::configFactory();
    
    +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -0,0 +1,91 @@
    +class MigrationPluginManager extends DefaultPluginManager implements MigrationPluginManagerInterface {
    

    strange way to pass deps, but arguments whould be properly type-hinted at least for MigrationPluginManagerInterface
    Why they optional?

  7. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
    @@ -168,8 +167,12 @@ protected function executeMigration($migration) {
    -    $migrations = Migration::loadMultiple($ids);
    -    array_walk($migrations, [$this, 'executeMigration']);
    +    $manager = $this->container->get('plugin.manager.migration');
    +    array_walk($ids, function ($id) use ($manager) {
    +      // This is possibly a base plugin id and we want to run all derivatives.
    +      $instances = $manager->createInstances($id);
    

    This should be in change record too, new way to get migrations!

chx’s picture

Thanks for the review, I edited the CR.

>Why that code commented with hash?
> the related issue does not describe what to do

To make it stand out more. None of the child issues describe what to do, they are placeholders. We will get there.

> strange way to pass deps, but arguments whould be properly type-hinted at least for MigrationPluginManagerInterface . Why they optional?

If there's a reroll I will do it if not I will do it in the dependency followup.. they are optional so that you can just do MigrationConfigDeriver::save() but they can be passed in for, I dunno, testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

In an ideal world this issue would be 100% complete on commit - however it does make sense to cleanups in separate issues as longs this issue contains all the API changes.

  1. I think we need to remove Drupal\migrate\Entity\MigrationInterface - as the issue summary hints at a good idea would be to do this is a followup. However I think we should put the correct interface in place in the issue and just make Drupal\migrate\Entity\MigrationInterface extend from it and add a @todo to remove it.
  2. grep -R "Drupal\\\migrate\\\Entity\\\Migration\W" ./core
    ./core/modules/comment/src/Tests/Migrate/d7/MigrateCommentTypeTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/field/src/Tests/Migrate/d6/MigrateFieldFormatterSettingsTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/field/src/Tests/Migrate/d6/MigrateFieldTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/file/src/Tests/Migrate/d6/MigrateUploadEntityDisplayTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/file/src/Tests/Migrate/d6/MigrateUploadEntityFormDisplayTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/file/src/Tests/Migrate/d6/MigrateUploadFieldTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/file/src/Tests/Migrate/d6/MigrateUploadInstanceTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/file/src/Tests/Migrate/d6/MigrateUploadTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/image/src/Tests/Migrate/d6/MigrateImageCacheTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/migrate/migrate.api.php: * \Drupal\migrate\Entity\Migration, with interface
    ./core/modules/migrate/src/MigrationBuilder.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/migrate/src/Plugin/migrate/builder/BuilderBase.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckMigration.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/node/src/Plugin/migrate/builder/d6/Node.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/node/src/Plugin/migrate/builder/d7/Node.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/node/src/Tests/Migrate/d6/MigrateNodeTypeTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/node/src/Tests/Migrate/d6/MigrateViewModesTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/node/src/Tests/Migrate/d7/NodeBuilderTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/system/src/Tests/Migrate/MigrateMenuTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/taxonomy/src/Plugin/migrate/builder/d6/TermNode.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/taxonomy/src/Tests/Migrate/d6/MigrateTaxonomyVocabularyTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/taxonomy/src/Tests/Migrate/d6/MigrateVocabularyEntityDisplayTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/taxonomy/src/Tests/Migrate/d6/MigrateVocabularyEntityFormDisplayTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/taxonomy/src/Tests/Migrate/d6/MigrateVocabularyFieldInstanceTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/taxonomy/src/Tests/Migrate/d6/MigrateVocabularyFieldTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/user/src/Plugin/migrate/builder/d6/ProfileValues.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/user/src/Plugin/migrate/builder/d7/User.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/user/src/Tests/Migrate/d6/MigrateUserPictureFileTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/user/src/Tests/Migrate/d6/MigrateUserRoleTest.php:use Drupal\migrate\Entity\Migration;
    ./core/modules/user/src/Tests/Migrate/d6/MigrateUserTest.php:use Drupal\migrate\Entity\Migration;
    

    This class no longer exists Drupal\migrate\Entity\Migration

  3. +++ b/core/modules/file/src/Tests/Migrate/d6/FileMigrationTestTrait.php
    @@ -0,0 +1,32 @@
    + * Contains
    

    Missing the rest of it... mind you these are all being removed.

  4. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -186,7 +186,8 @@ public function import() {
    +      #$this->migration->checkRequirements();
    

    I think commenting out this is a mistake. Looking at Drupal\migrate\Plugin\Migration::checkRequirements() - that's still doing an entity load multiple.

  5. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -252,6 +232,53 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
       /**
    +   * Construct a migration plugin.
    +   *
    +   * @param array $configuration
    +   *   A configuration array containing information about the plugin instance.
    +   * @param string $plugin_id
    +   *   The plugin_id for the plugin instance.
    +   * @param mixed $plugin_definition
    +   *   The plugin implementation definition.
    +   */
    +  public function __construct(array $configuration, $plugin_id, array $plugin_definition) {
    

    I don't think we can typehint $plugin_definition as there have been moves to make this an object. It is mixed in the docblock. Also isn't {@inheritdoc} appropriate here?

  6. +++ b/core/modules/migrate/src/Plugin/MigrationConfigDeriver.php
    @@ -0,0 +1,94 @@
    +/**
    + * Expose migrations in the active config store as derivatives.
    + */
    +class MigrationConfigDeriver extends DeriverBase implements ContainerDeriverInterface {
    

    Does this whole thing exist as a bc layer for customised migrations? What happens when existing sites have a config for an existing plugin? And does this class have any test coverage?

  7. +++ b/core/modules/migrate/src/Plugin/MigrationConfigDeriver.php
    @@ -0,0 +1,94 @@
    +      $prefix = 'migrate.migration.';
    

    So as the entity no longer exists with the prefix 'migrate.migration.' this is invalid configuration. What happens when the migrate UI reintroduces the config entity?

  8. +++ b/core/modules/migrate/src/Plugin/MigrationYamlDiscovery.php
    @@ -0,0 +1,92 @@
    +        $contents = Yaml::decode(file_get_contents($file)) ?: [];
    

    We need to catch exceptions here and re-throw with the information of what file has caused the problem - otherwise this can be hard to debug. (And yes we should do this in YamlDiscovery too - but not in this issue.)

  9. +++ b/core/modules/migrate/src/Plugin/MigrationYamlDiscovery.php
    @@ -0,0 +1,92 @@
    +      foreach (['migration_templates', 'migrations'] as $subdirectory) {
    

    Is the fact that both migration_templates and migrations are scanned tested anywhere? Also I think this could be clearly documented on the class doc.

  10. +++ b/core/modules/migrate/tests/src/Unit/process/MigrationTest.php
    @@ -27,6 +27,9 @@ class MigrationTest extends MigrateProcessTestCase {
    +    // @TODO https://www.drupal.org/node/2667620
    +    $this->assertSame(TRUE, TRUE);
    +    return;
    
    @@ -55,6 +58,9 @@ public function testTransformWithStubSkipping() {
    +    // @TODO https://www.drupal.org/node/2667620
    +    $this->assertSame(TRUE, TRUE);
    +    return;
    

    This does not look good - I think we should try to make sure this test coverage is fixed in this issue.

  11. +++ b/core/modules/migrate_drupal/migrate_drupal.module
    @@ -19,3 +22,41 @@ function migrate_drupal_help($route_name, RouteMatchInterface $route_match) {
    +  $vocabulary_migration_definition = [
    +    'source' => [
    +      'ignore_map' => TRUE,
    +      'plugin' => 'd6_taxonomy_vocabulary',
    +    ],
    +    'destination' => [
    +      'plugin' => 'null',
    +    ],
    +  ];
    +  $vocabulary_migration = new Migration([], uniqid(), $vocabulary_migration_definition);
    +  $executable = new MigrateExecutable($vocabulary_migration, new MigrateMessage());
    +  // This is why the deriver can't do this: the 'd6_taxonomy_vocabulary'
    +  // definition is not available to the deriver as it is running inside
    +  // getDefinitions().
    +  if (isset($definitions['d6_taxonomy_vocabulary'])) {
    

    Can't all the stuff before the if be inside the if

  12. +++ b/core/modules/migrate_drupal/migrate_drupal.module
    @@ -19,3 +22,41 @@ function migrate_drupal_help($route_name, RouteMatchInterface $route_match) {
    +    }
    +    catch (\Exception $e) {
    +
    +    }
    

    Why are exceptions being thrown and why are we catching the generic exception and saying nothing - looks dangerous.

  13. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/D6NodeDeriver.php
    @@ -0,0 +1,155 @@
    +    }
    +    catch (\Exception $e) {
    +      // @TODO https://www.drupal.org/node/2666640
    +    }
    

    Catching the generic exception and doing nothing looks super dangerous. Apparently this one is something about dependencies.

  14. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/D7NodeDeriver.php
    @@ -0,0 +1,111 @@
    +      foreach (D6NodeDeriver::getSourcePlugin('d7_field_instance') as $row) {
    ...
    +      foreach (D6NodeDeriver::getSourcePlugin('d7_node_type') as $row) {
    

    This looks like an abstract NodeDeriverBase could be useful. Or maybe even a more generic trait.

  15. +++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
    @@ -58,6 +58,8 @@ protected function loadFixture($path) {
       protected function installMigrations($version) {
    +    // @TODO https://www.drupal.org/node/2668436
    +    return;
    

    Does this need a separate issue? It looks like installMigrations can just be removed.

  16. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6TestBase.php
    @@ -61,9 +59,11 @@ protected function migrateUsers($include_pictures = TRUE) {
    -      Migration::load('d6_user_picture_file')->delete();
    -      Migration::load('user_picture_entity_display')->delete();
    -      Migration::load('user_picture_entity_form_display')->delete();
    +      $manager = $this->container->get('plugin.manager.migration');
    +      foreach (['d6_user_picture_file', 'user_picture_entity_display', 'user_picture_entity_form_display'] as $id) {
    +        // @TODO https://www.drupal.org/node/2666648
    +        #$manager->createInstance($id)->delete();
    +      }
    

    I think we can just delete this code and move on.

  17. +++ b/core/modules/migrate_drupal/src/Tests/dependencies/MigrateDependenciesTest.php
    @@ -28,24 +27,26 @@ class MigrateDependenciesTest extends MigrateDrupal6TestBase {
    +    $migration_items = array('d6_comment', 'd6_filter_format', 'd6_node:page');
    +    // @TODO https://www.drupal.org/node/2666640
    +    return;
    

    I'm really wary of leaving the dependency / requirements work till later.

  18. +++ b/core/modules/taxonomy/src/Plugin/migrate/D6TermNodeDeriver.php
    @@ -0,0 +1,58 @@
    +    catch (\Exception $e) {
    +      // @TODO https://www.drupal.org/node/2666640
    +    }
    

    This catch looks dangerous too.

mikeryan’s picture

I did start working on #2667366: Make migrate_plus work with Drupal 8.1.x (migrate_plus) and #2667368: Make migrate_tools work with Drupal 8.1.x (migrate_tools) with this patch yesterday, didn't have time to write up what my experience was so far until tonight...

The migrate_example module was simple enough - just moved the migration .yml files from config/install to the migrations directory (something that definitely needs to be in the change record). Well, I also had to hack the connection key into each migration, until I figure out how to implement the merge-the-migration-group's-config-into-each-migration with hook_migration_plugins(). But, pleased that at least a simple migration scenario works with no changes of substance.

First pass at implementing drush migrate-status, replacing the EntityQuery with plugin discovery, died a horrible death:

~/Sites/d8/modules/contrib/migrate_plus$ drush ms
exception 'Drupal\Component\Plugin\Exception\InvalidDeriverException' with message 'Plugin (d6_node) deriver "Drupal\migrate_drupal\Plugin\migrate\D6NodeDeriver" does not exist.' in         [error]
/Users/mryan/Sites/d8/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php:213

The problem here being that the d6_node migration plugin in the node module is referencing the D6NodeDeriver class in migrate_drupal, and I did not have migrate_drupal enabled. chx feels that all the Drupal migration plugins should be in migrate_drupal, although I still feel that the ideal model is that each module should be responsible for its own plugins. Anyway, either all the plugins should move to migrate_drupal, or D6NodeDeriver/D7NodeDeriver should move to the node module.

A more basic issue is, the drush migrate-status command is supposed to list any "available" (as in, "I can run them now") migrations. So, if I enable migrate_example and run drush ms, I expect to see the four migrations beer_term, beer_user, beer_node, and beer_comment listed. This is how it currently works - the migrations of interest to migrate-status are the active configuration entities, easily discovered with an entityQuery(). If I were doing a Drupal upgrade, the configuration process would create only those configuration entities that are relevant to the particular site being migrated (this is why we have templates), and migrate-status equally easily finds those.

So, I'm struggling to see how that works with migrations-as-plugins. Plugin-based discovery is going to discover all migration plugins - i.e., all the migrations defined in all enabled core modules. It's not clear to me how to filter out all the irrelevant plugins. This would apply in the upgrade use case as well - if I don't have the aggregator module enabled on my D6 site, I don't want to run the d6_aggregator_item (not to mention the d7_aggregator_item) migration when upgrading. Is this an issue that's going to be addressed in the dependency follow-up?

Apart from figuring out how that will work, though, the only change to the drush commands was to replace Migration::load($migration_id) calls with \Drupal::service('plugin.manager.migration')->createInstance($migration_id), so that's encouraging. The UI, based on ConfigEntityListBuilder and EntityForm, is going to be more work though.

The next step will be figuring out how this applies to the upgrade process (#2667370: POC of migrations-as-plugins). I won't have a good chunk of time to dive into that before this weekend (if even then), and I'm actually curious how chx and benjy see the plugin approach working in practice here (hint hint, if one of you has time to try a patch in the next few days...).

Also when I have a chance, I need to understand derivers better... I'm not quite clear on how one will, say, customize field mappings on a per-node-type basis as builders support now.

alexpott’s picture

There is other contrib that might want the YAML discovery of one plugin per file. I think it is worth splitting that out into another patch so we can add tests and generalise.

benjy’s picture

The problem here being that the d6_node migration plugin in the node module is referencing the D6NodeDeriver class in migrate_drupal, and I did not have migrate_drupal enabled. chx feels that all the Drupal migration plugins should be in migrate_drupal, although I still feel that the ideal model is that each module should be responsible for its own plugins. Anyway, either all the plugins should move to migrate_drupal, or D6NodeDeriver/D7NodeDeriver should move to the node module.

This is already a problem in HEAD, see #2560795: Source plugins have a hidden dependency on migrate_drupal and #2600926: Allow annotations to inherit across namespaces

chx’s picture

> chx feels that all the Drupal migration plugins should be in migrate_drupal

But I already said I won't move them back and anyways my plans to just get this in quick has been completely killed by alexpott's review where he doesn't want checkRequirements and perhaps even the dependency checks to be redone here. I was strongly inclined to won't fix it reading his review yesterday but yesterday I was very tired so I thought I will sleep on it. Well, now I did, and I still do not feel like continuing this at all. Heaping everything in one patch is not going to work, but of course, I can do it if I must.

alexpott’s picture

I've created #2671034: Create plugin per file YAML discovery to handle the generic plugin discovery.

Also I've discussed this issue with @catch. Whilst I'm disappointed that d.o does not yet support feature branches and that committing this will regress existing functionality, I concede that managing this in a mega-patch will be difficult. Therefore let's punt the dependency and checkRequirements stuff to a followup patch that should be a major and tagged with "migrate critical".

However I still think we need to address everything else from #71 and subsequent reviews.

I think we need to remove the entire config integration as providing this should be the remit of the module that provides the config entity type that configures migrate plugins. For me, leaving this in core is dangerous (because what about existing migration config in a live site) and confusing.

benjy’s picture

I think we need to remove the entire config integration as providing this should be the remit of the module that provides the config entity type that configures migrate plugins. For me, leaving this in core is dangerous (because what about existing migration config in a live site) and confusing.

I've been thinking about this and I think that does make sense. The config isn't needed to run the migrations, it is only really needed to support the tooling outside of core so that could quite easily be done from contrib. I could put the ConfigEntity definition into http://drupal.org/project/migrate_api which is a module specifically designed to be a dependency for migrate-contrib tooling.

chx’s picture

Status: Needs work » Needs review
FileSize
27.84 KB
133.37 KB

I removed the config deriver, fixed MigrateTaxonomyTermStubTest accordingly. The relevant concerns were addressed, the rest already has followups.

chx’s picture

FileSize
598 bytes
133.37 KB

Fixed a braino in the new doxygen. Note I didn't move the interface as I am unable to get git to recognize the change and it pushes the patch to be +20k . No point.

benjy’s picture

Reviewing the interdiffs they look good to me, shouldn't we also remove all the schema to do with the config entity here?

alexpott’s picture

@benjy yep you are right - core shouldn't have any remnant of the migrations as config entities at all.

chx’s picture

Still objecting for patch size reasons. I haven't deleted derivers either.

chx’s picture

FileSize
155.26 KB

But, here it is.

alexpott’s picture

benjy’s picture

Status: Needs review » Reviewed & tested by the community

I believes all of alexpott's feedback from #71 has been done and the per plugin file YAML patch was merged so back to RTBC.

alexpott’s picture

The biggest worry about going forward here is the affect on the Drupal Upgrade module ie. #2667370: POC of migrations-as-plugins as that is the module we want to put into 8.1.0 as an experimental module.

alexpott’s picture

I don't think the derivers should be reaching into the D6NodeDeriver to get code they need - let's encapsulate that in a trait.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 87: 2625696-2-87.patch, failed testing.

chx’s picture

The trait is a good idea but regarding the renames which caused the fail: http://webchick.net/please-stop-eating-baby-kittens :P

alexpott’s picture

Status: Needs work » Needs review
FileSize
153.32 KB
6.2 KB

@chx I was playing around with idea of move them but I didn;t mean to post a patch that did - sorry. I'm torn about the location of the derivers - but I can't work out what to do with the cck deriver and service. I guess we could work some of this out with the dependency stuff. But it feels off to move this stuff about again.

Interdiff with the last working patch - #84.

Status: Needs review » Needs work

The last submitted patch, 90: 2625696-2-90.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
8.81 KB
153.2 KB

Ok It is official I can't roll patches... so as the D*NodeDerivers obviously want me to move them and this time I've run the tests locally. They definitely should be part of node and not migrate Drupal - and then we only have the cck one to work what to do with.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

The latest patches just moved stuff about.

chx’s picture

Yes, I wanted to RTBC back too. Thanks.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I'm going to commit this now to give us the chance to get #2666640: Readd dependencies and such to migration done and then make the necessary changes to migrate_drupal and get that in core as an experimental module. There is a chance that this patch will be reverted if we decide that getting migrate_drupal in core is more important. However at least doing this now gives us a chance to do this.

Committed 694994f and pushed to 8.1.x. Thanks!

benjy’s picture

  • xjm committed 73f9170 on 8.1.x
    Revert "Issue #2625696 by chx, alexpott, benjy, mikeryan, andypost,...
xjm’s picture

Priority: Normal » Major
Status: Fixed » Needs work
Issue tags: +Needs release manager review, +Needs subsystem maintainer review, +Needs framework manager review

Sorry folks but we need to revert this, for two reasons:

  1. The beta for 8.1.x begins Wednesday, and as previously stated, Migrate UI is one of the top three release priorities for 8.1.0. The UI was very close to ready for a core commit prior to this patch. (I was told previously that this change would not affect existing migrations or the work being done on the UI, which apparently isn't correct.)
  2. As far as I understand from @mikeryan and the followups above, this introduces critical regressions in e.g. migration dependencies, which does not seem consistent with beta-stability code (see #2656994: Experimental modules should have their own version numbers). Migrate is still experimental, but it's something of a special case also. So based on Dries' "Always Be Shippable" blog post and the related DrupalCon Barcelona keynote, I think this important API work needs to be added to the main branch as a complete thing so that the train can leave the station appropriately for the minors.

Based on the followups above, it sounds like this might actually be a case for an official feature branch of Drupal 8 in the core repository, despite Migrate being experimental. We also should do what we can to make sure this work can go forward too and potentially land in 8.2.x soon.

Also, I think this issue will need subsystem, framework, and release manager signoff to go in, because it significantly impacts the subsystem, the architecture, and the release timeline and goals. @alexpott's commit is an implicit framework manager signoff, but we should probably discuss more given that I'm reverting this for release management reasons. It also sounds as though there was not consensus within the Migrate subsystem maintainers so that needs to be sorted too.

Finally, @alexpott previously suggested that with this being outstanding, we should consider marking Migrate itself alpha instead of beta. That's something we can address in #2656994: Experimental modules should have their own version numbers.

chx’s picture

Thanks for coordinating this with us. While I could say how close dependencies were and we were ready to give a hand to core UI if necessary I won't. I am done with migrate. I am unfollowing this and every other issue. I do not need people attacking me on Twitter for a BC break which gets reverted two days later with zero coordination.

benjy’s picture

Yeah this is really disappointing, especially given I spent most the weekend working on the follow-ups including the dependency patch which is pretty much ready.

I pinged alexpott offering to chip-in with any changes that would be required with the UI off the back of this issue, even though, I really didn't want anything to do with it.

It also sounds as though there was not consensus within the Migrate subsystem maintainers so that needs to be sorted too.

I don't know where those sounds are coming from, it would be nice if they spoke up directly to another migrate maintainer in #drupal-migrate

catch’s picture

Just a note that we’d like to not add major new functionality during the beta, like migrate UI, so not breaking that just before beta1 was important.

However since this is clean-up and migrate will be in alpha generally, it should be fine to be re-committed during the beta - we just don’t want to break migrate UI or other key functionality at the same time.

The last submitted patch, 92: 2625696-2-92.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
153.07 KB

Re-rolled, hopefully tests will run at least.

Status: Needs review » Needs work

The last submitted patch, 103: 2625696-103.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
171.28 KB

I've re-rolled again, the last re-roll left the migration class in the entity namespace which of course broke every test.

I had to bring in a a few changes from the dependency patch so we can load multiple migrations at once, I also added a way to load migrations based on a tag to the plugin manager as that was a feature of the entity storage. Most of the changes were against the migrate_drupal_ui, which needs some attention in general.

I had a failure on the UI test locally regarding not being able to set the "driver", i'm wondering if that's because I only have mysql available.

I think we can probably remove some of the "Needs xyz review" tags from this issue now? I'll let someone else do that.

benjy’s picture

FileSize
20.92 KB
benjy’s picture

OK, this is all I have time for tonight. It fixes all the obvious issues, but the UI tests are still failing. Didn't look into why and the UI just swallows the errors so there is nothing in the simpletest output.

The last submitted patch, 105: 2625696-106.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 107: 2625696-107.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
181.18 KB
16.01 KB

OK, this brings back the dependencies stuff which I wrote in #2666640: Readd dependencies and such to migration

Status: Needs review » Needs work

The last submitted patch, 110: 2625696-108.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
181.39 KB

Fixing some of the fails...

Status: Needs review » Needs work

The last submitted patch, 112: 2625696-3-110.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
182.4 KB
2.27 KB

So we have a chicken and egg situation - we need the source credential whilst instantiating the plugins but in the previous patch they were only set on the plugin after...

Status: Needs review » Needs work

The last submitted patch, 114: 2625696-3-114.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
719 bytes
183.32 KB

Fixing one of the test fails

Status: Needs review » Needs work

The last submitted patch, 116: 2625696-3-116.patch, failed testing.

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

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

benjy’s picture

Status: Needs work » Needs review
FileSize
2.23 KB
184.58 KB

I'm not sure what is going on in the file source but I removed that for now, it doesn't make any sense to me, its using the destination config to do something in the source but then I can't see anything been done with that data until the destination anyway? Alas, it will need some manual testing and inspection before commit.

The MigrationCreationTrait was manually calling checkRequirements() on the source and the destination rather than calling checkRequirements() on the migration itself?

These migrations are still failing:

Migration user_profile_field did not meet the requirements. Missing source provider profile source_provider: profile.
Migration user_profile_field_instance did not meet the requirements. Missing source provider profile source_provider: profile.
Migration user_profile_entity_display did not meet the requirements. Missing source provider profile source_provider: profile.

A backtrace from DrupalSqlBase::checkRequirements() would probably help figure these out.

Status: Needs review » Needs work

The last submitted patch, 119: 2625696-119.patch, failed testing.

alexpott’s picture

The 10 files in the d6 migration is interesting... the files table in D6 looks like this...

+-----+-----+--------------------+----------------------------------------------+------------+----------+--------+------------+
| fid | uid | filename           | filepath                                     | filemime   | filesize | status | timestamp  |
+-----+-----+--------------------+----------------------------------------------+------------+----------+--------+------------+
|   1 |   1 | Image1.png         | core/modules/simpletest/files/image-1.png    | image/png  |    39325 |      1 | 1388880660 |
|   2 |   1 | Image2.jpg         | core/modules/simpletest/files/image-2.jpg    | image/jpeg |     1831 |      1 | 1388880664 |
|   3 |   1 | Image-test.gif     | core/modules/simpletest/files/image-test.gif | image/jpeg |      183 |      1 | 1388880668 |
|   5 |   1 | html-1.txt         | core/modules/simpletest/files/html-1.txt     | text/plain |       24 |      1 | 1420858106 |
|   6 |   1 | some-temp-file.jpg | /tmp/some-temp-file.jpg                      | image/jpeg |       24 |      0 | 1420858106 |
+-----+-----+--------------------+----------------------------------------------+------------+----------+--------+------------+

So 4 is the correct number as temp files are not migrated.

alexpott’s picture

  public function getDerivativeDefinitions($base_plugin_definition) {
    // Read all CCK field instance definitions in the source database.
    $fields = array();
    $source_plugin = static::getSourcePlugin('d6_field_instance');
    try {
      $source_plugin->checkRequirements();

      foreach ($source_plugin as $row) {
        $fields[$row->getSourceProperty('type_name')][$row->getSourceProperty('field_name')] = $row->getSource();
      }
    }
    catch (RequirementsException $e) {
      // Don't do anything; $fields will be empty.
    }

This code is problematic because if we didn't add the database fallback thing that the source stuff needs to be somehow set on the source plugin created here.

xjm’s picture

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

The committers discussed this earlier in the week and as catch suggests in #101 we'd like to still try to get this into 8.1.x, especially if it can go in without regressions before a second beta. So moving back to the 8.1.x queue for visibility.

benjy’s picture

So 4 is the correct number as temp files are not migrated.

What makes you say temporary files are not migrated?

benjy’s picture

FileSize
190.48 KB

Yeah that files issue is a good one, heres what it looks like:

files table

The same issue has been mentioned by others in IRC and on the support forums: https://www.drupal.org/node/2681475 so its a pre-existing in HEAD. You can see the issue in the image, it shows files belonging to other users, and the weird ones are the picture files for users who had a empty picture in the source data.

I can't explain yet how this ever passed unless for some reason, the d6_user_picture_file migration was not running previously which would imply an issue with dependencies.

benjy’s picture

I've opened #2681505: Arbitrary files are created when migrating users with an empty picture although the UI tests fail there as well with Found 0 file entities, expected 4. because the UI was never really working for files and it just so happened that the UI got four bogus profile files rather than the 4 actual files it was expecting. See screenshot after running the Upgrade UI test.

upgrade test

benjy’s picture

Status: Needs work » Needs review
FileSize
185.59 KB
3.17 KB

OK, this fixes MigrateUpgrade6Test, it includes #2681505: Arbitrary files are created when migrating users with an empty picture as well and you can see from the image in #125, that 6 is the correct amount of files to be asserting for.

Status: Needs review » Needs work

The last submitted patch, 127: 2625696-127.patch, failed testing.

alexpott’s picture

@benjy the problem we have is that we're still just relying on the state fallback for the database - which in itself feels wrong. We're at an interesting place there is a tiny amount of configuration that is necessary for migrations... the source database and the source base path... and actually the list of migrations to perform.

benjy’s picture

@alexpott, yes I understand that, but that doesn't take away from the files issue i fixed in #127? Can we commit this with the fallback in place here and then remove it in a follow-up or do you want to do it all here?

alexpott’s picture

Well the fallback will work for the database but for the source base path?

benjy’s picture

Status: Needs work » Needs review
FileSize
188.32 KB
3.65 KB

The only quick workaround I can think of is hard-coding the logic currently in getMigrations() to do with the source base path into the batch runner when the migrations are created :/

This patch should be green, fixed an issue in migration creation trait. Also reverted back to just manually checking the requirements on the source/destination which we should create a follow-up to fix.

alexpott’s picture

+++ b/core/modules/file/src/Plugin/migrate/source/d7/File.php
@@ -91,7 +91,9 @@ public function prepareRow(Row $row) {
-    $path = str_replace($this->migration->get('destination')['source_base_path'], NULL, $path);
+
+    // @TODO, figure out why this is done here and not the destination.
+    // $path = str_replace($this->migration->get('destination')['source_base_path'], NULL, $path);

We need to remove this hack though...

benjy’s picture

Yes, this line of code is confusing and obviously doesn't have any test coverage. The destination simply sticks the two values back together before doing anything with it: $source = $this->configuration['source_base_path'] . $file;

I actually think we should just delete that line in the source and then add a new test to ensure that source_base_path gets into the destination and finally hardcode that logic for entity:file I mentioned above, into the batch runner when it does a createInstances(). That will get us finished here and we can fix properly in a follow-up?

benjy’s picture

OK, i've reverted the change in the D7 file source, we can fix that in #2577871: d7_file plugin should receive source_base_path configuration option

I've created an issue for the checkRequirements() calls I had to revert in the last patch: #2681867: Drupal Upgrade UI should validate the entire migration, not the source and destination individually

I've created #2681869: Provide clean way to merge configuration into migration plugins to remove the manual setting of the destination config in the batch runner and generically solve how we're going to allow sources and destinations to get their requirements.

I had to fix the source base path in the D6 test, that was why no files were been migrated for the D6 tests, as i reported in #2681539: MigrateUpgrade6Test does not migrate files

There is certainly plenty of ugly happening with the latest fixes required to get the UI green but this patch is nearly 200kb already. If we commit as is to 8.2.x then work on all the follow-ups and once it's all ready, back port to 8.1 that would be the best way forward IMO.

alexpott’s picture

Re-rolled as #2681505: Arbitrary files are created when migrating users with an empty picture has been committed. @benjy nice job on creating the followups. I've closed #2666640: Readd dependencies and such to migration as that is now redundant. The patch added does a bit of clean up - but there are still two references to #2666640: Readd dependencies and such to migration that need removing.

I also discussed the issue with @benjy on IRC. I think the state fallback for source databases and the way source base path is set are both not ideal but also HEAD is not ideal. Given that we have no regressions here I think fixing this so any migration can declare and get its requirements in #2681869: Provide clean way to merge configuration into migration plugins makes sense.

Furthermore whilst reviewing MigrateCreationTrait I've realised two things:

  1. The name is not correct with this patch - at most it is configuring migrations not creating them (it was prior to this patch) - address in #2682585: Rename MigrationCreationTrait as it no longer creates migrations - it configures them
  2. The methods getConnection() and getSourceDatabaseConnection() are confusing - they both return the source connection and they both do more than just getting a connection.

The latter is addressed by the patch added because getSourceDatabaseConnection() was added by it.

I think once the incorrect @todo's are addressed this patch is ready for serious review and consideration for 8.2.x. I think that in order to get this in 8.1.x-beta we need to get a certain amount of the followups done so we can cherry-pick the lot in one go. This way the patch does not put 8.1.0 at risk and can go there if we get the followups done in time BUT also the patch does not become a 500k unreviewable monster. (@benjy mentioned that doing something like is @catch's idea).

The great thing is that we've got to the point were they are no regressions over what is in HEAD for the purposes of migrate UI. benjy++

quietone’s picture

Just starting using code sniffer in the new IDE and currently get way too much info. But spotted this in all that.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
@@ -0,0 +1,114 @@
+  /**
+   * Constructs a CckMigration.
+   *
+   * @param array $configuration
+   *   Plugin configuration.
+   * @param string $plugin_id
+   *   The plugin ID.
+   * @param mixed $plugin_definition
+   *   The plugin definition.
+   * @param \Drupal\migrate\Plugin\MigratePluginManager $cck_manager
+   *   The cckfield plugin manager.
+   */

Missing @param for MigrationPluginManagerInterface $migration_plugin_manager.
And needs a use statement for the MigratePluginManager.

alexpott’s picture

Patch addresses #137 and also fixes a lot of other coding standards things noticed by phpcs that are in scope.

benjy’s picture

I refined it so we're only catching a database exception now instead of all exceptions and documented why that is so. I also added a test and fixed an issue todo with submitting the UI form with invalid database credentials.

Status: Needs review » Needs work

The last submitted patch, 139: 2625696-139.patch, failed testing.

benjy’s picture

Bad assumption about the driver.

Status: Needs review » Needs work

The last submitted patch, 141: 2625696-141.patch, failed testing.

alexpott’s picture

sqlite doesn't use passwords... let's just change the database and assert on migrate ui's message. Also we need to add proper requirements checking to the FieldInstance source plugins.

benjy’s picture

Thanks for fixing the sqlite issue.

I'm not sure if the tableExists checks are correct here or not, we usually rely on the source_provider for the requirement check. If the source provider exists, then so should the tables?

alexpott’s picture

@benjy but these are being used in the derivers it is bit of a different situation.

Also I think we need to get #2683579: Improve migrate UI tests in first because it properly tests the source_base_path setting through the UI and we're changing how this is configured on the migrations in this patch.

benjy’s picture

I added the source_provider rather than the table exist checks. interdiff is against #141.

Status: Needs review » Needs work

The last submitted patch, 146: 2625696-146.patch, failed testing.

alexpott’s picture

Rerolled...

M	core/modules/taxonomy/src/Tests/Migrate/d6/MigrateVocabularyFieldInstanceTest.php
M	core/modules/taxonomy/src/Tests/Migrate/d6/MigrateVocabularyFieldTest.php
Falling back to patching base and 3-way merge...
chx’s picture

Status: Needs review » Reviewed & tested by the community

So I diffed to the state when it was committed last and did a through review and most of this looks straightforward requirements and dependency resurrection (although obviously a huge effort: thanks benjy and alexpott) the only weird thing is the file change in the dump from 0 to 6 but that's just a reverse of #2681505: Arbitrary files are created when migrating users with an empty picture since files now work properly.

Also, the MigrationCreateTrait todo saying "The trait no longer creates migrations" is the kind of funny one usually finds only on Raymond Chen's The Old New Thing or TheDailyWTF.

Ps. I reviewed only because benjy asked. This doesn't change anything involvement level wise.

alexpott’s picture

I've merged this and #2683579: Improve migrate UI tests and the UI tests are still green so we've not broken the source_base_path UI here. Depending on which issue gets in first one will need a reroll.

alexpott’s picture

Wwhat I don't understand is how this patch fixes the files migrate in the UI test - ah I see...

+++ b/core/modules/migrate_drupal_ui/src/Tests/d6/MigrateUpgrade6Test.php
@@ -30,7 +30,7 @@ protected function setUp() {
-    return __DIR__ . '/files';
+    return './';

But this is the bit is not quite right... however this is fixed in #2683579: Improve migrate UI tests.

I've merged the patches locally and can confirm that the UI tests still pass.

[Edit: for clarity]

alexpott’s picture

I've deleted the comment #151 because it is a red herring and distracting. So I think it is okay if #148 goes in and we fix the UI test to use a custom source base path in #2683579: Improve migrate UI tests.

Given the effort that has go into this patch I think it deserves first bite.

catch’s picture

Started reviewing this, didn't finish a full pass. I already reviewed the original patch here and was mostly happy with it, so not much new here mostly minor.

  1. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -571,28 +603,13 @@ public function getMigrationDependencies() {
         }
    

    Sentence is a bit odd, and it could do with an explanation why.

  2. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -0,0 +1,230 @@
    +   *
    +   * We need to expand any derivative migrations. Derivative migrations are
    +   * calculated by migration derivers such as D6NodeDeriver. This allows
    +   * migrations to depend on the base id and then have a dependency on all
    +   * derived migrations. For example, d6_comment depends on d6_node but after
    +   * we've expanded the dependencies it will depend on d6_node:page,
    +   * d6_node:story and so on, for derived migration.
    

    Very minor but 'derivative migrations' and 'derived migrations' are used almost interchangeably here.

  3. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -0,0 +1,230 @@
    +      $plugin_ids += preg_grep('/^' . preg_quote($id, '/') . PluginBase::DERIVATIVE_SEPARATOR . '/', array_keys($this->getDefinitions()));
    

    preg_grep()++

  4. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -0,0 +1,230 @@
    +    // Migration dependencies defined in the migration storage can be
    +    // optional or required. If an optional dependency does not run, the current
    +    // migration is still OK to go. Both optional and required dependencies
    +    // (if run at all) must run before the current migration.
    +    $dependency_graph = array();
    +    $requirement_graph = array();
    

    It might help to explicitly point out that $dependency_graph will hold all dependencies, and $requirement_graph will hold only the required ones. Would almost call $requirement_graph $required_dependency_graph.

  5. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -0,0 +1,230 @@
    +      if (isset($migration_dependencies['optional'])) {
    +        foreach ($migration_dependencies['optional'] as $dependency) {
    +          $different = TRUE;
    +          $this->addDependency($dependency_graph, $id, $dependency, $dynamic_ids);
    +        }
    +      }
    +    }
    +    $graph_object = new Graph($dependency_graph);
    +    $dependency_graph = $graph_object->searchAndSort();
    +    if ($different) {
    +      $graph_object = new Graph($requirement_graph);
    +      $requirement_graph = $graph_object->searchAndSort();
    +    }
    +    else {
    +      $requirement_graph = $dependency_graph;
    +    }
    

    I can't see why $different = TRUE has to be tracked, this looks the same as if (!empty($migration_dependencies['optional'])) - which would also be more self-explanatory.

    Also $graph_object being set twice I had to read a couple of times. I think we could do $dependency_graph = (new Graph($dependency_graph)->searchAndSort()?

  6. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
    @@ -0,0 +1,230 @@
    +      // Populate a weights array to use with array_multisort later.
    

    Needs the parentheses.

  7. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManagerInterface.php
    @@ -0,0 +1,36 @@
    +   *   instantiated. Also accepts an array of plugin ids and an empty array to
    

    IDs

  8. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManagerInterface.php
    @@ -0,0 +1,36 @@
    +   *   plugin id.
    

    ID

  9. +++ b/core/modules/migrate/src/Plugin/MigrationPluginManagerInterface.php
    @@ -0,0 +1,36 @@
    +   *   If the instance cannot be created, such as if the ID is invalid.
    

    Since it's multiple instances should this be 'If an instance cannot be created' instead of 'If the...'

  10. +++ b/core/modules/migrate/src/Tests/MigrateTestBase.php
    @@ -168,8 +167,12 @@ protected function executeMigration($migration) {
    +      // This is possibly a base plugin id and we want to run all derivatives.
    

    ID again.

  11. +++ b/core/modules/migrate/tests/src/Unit/process/MigrationTest.php
    @@ -27,6 +27,9 @@ class MigrationTest extends MigrateProcessTestCase {
    +    // @TODO https://www.drupal.org/node/2667620
    

    What about nuking it and putting it back again? Assuming we fix the @todo soonish don't really mind though.

  12. +++ b/core/modules/migrate_drupal/migrate_drupal.module
    @@ -19,3 +22,41 @@ function migrate_drupal_help($route_name, RouteMatchInterface $route_match) {
    +    try {
    +      foreach ($vocabulary_migration->getSourcePlugin() as $row) {
    +        $executable->processRow($row, $process);
    +        $source_vid = $row->getSourceProperty('vid');
    +        $plugin_ids = ['d6_term_node:' . $source_vid, 'd6_term_node_revision:' . $source_vid];
    +        foreach ($plugin_ids as $plugin_id) {
    +          if (isset($definitions[$plugin_id])) {
    +            $definitions[$plugin_id]['process'][$row->getDestinationProperty('vid')] = 'tid';
    +          }
    +        }
    +      }
    +    }
    +    catch (\Exception $e) {
    +
    +    }
    

    Shouldn't we trigger_error() or something when there's an exception?

    Also shouldn't the try/catch be for each individual row? It's not clear at all from here what the implications are of the exception being tried/caught - but if we can eat the exception, I'd expect later rows to not be blocked on it. If we can't eat it, we should at least log an error.

  13. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -64,73 +64,49 @@ protected function getSystemData(array $database) {
    +   *   The drupal version.
    

    Drupal Drupal Drupal Drupal, Drupal.

  14. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/CckMigration.php
    @@ -0,0 +1,116 @@
    +        }
    +        catch (RequirementsException $e) {
    +          // Kill the rest of the method.
    +          $source_plugin = [];
    +        }
    

    Again no need to log anything here?

  15. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -949,30 +947,32 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +        // Store the retrieved migration ids in form storage.
    

    IDs

  16. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -949,30 +947,32 @@ public function validateCredentialForm(array &$form, FormStateInterface $form_st
    +        // Store the retrived system data in from storage.
    

    retrieved data in form storage.

  17. +++ b/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php
    @@ -0,0 +1,143 @@
    +    catch (RequirementsException $e) {
    +      // If checkRequirements() failed then the content module did not exist and
    +      // we do not have any CCK fields. Therefore, $fields will be empty and
    +      // below we'll create a migration just for the node properties.
    +    }
    

    Hmm so this isn't really exceptional, just means CCK was not installed?

  18. +++ b/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php
    @@ -0,0 +1,143 @@
    +    catch (DatabaseExceptionWrapper $e) {
    +      // Once we begin iterating the source plugin it is possible that the
    +      // source tables will not exist. This can happen when the
    +      // MigrationPluginManager gathers up the migration definitions but we do
    +      // not actually have a Drupal 6 source database.
    +    }
    +
    

    Shouldn't we still show an error in this case even if not fatal?

benjy’s picture

  1. Fixed
  2. Fixed
  3. n/a
  4. Renamed variable, think that makes it clearer.
  5. Updated both lines of code so we no longer store the graph.
  6. Fixed
  7. Fixed
  8. Fixed
  9. Fixed
  10. Fixed
  11. Left in place, I already have the patch in #2667620: Rewrite Drupal\Tests\migrate\Unit\process\MigrationTest written, just needs a re-roll.
  12. Not sure about this exception, will need looking into.
  13. Fixed
  14. When we're catching requirements exceptions, there is nothing to log. It usually means that the source site didn't have the tables/modules enabled for the source to be iterated.
  15. Fixed
  16. Fixed
  17. Correct, see source_provider on the sources and DrupalSqlBase::checkRequirements().
  18. The problem here is the D6 derivers get loaded even if you're migrating from D7, we have a follow-up to solve this better. #2560795: Source plugins have a hidden dependency on migrate_drupal

I missed the first part of 5 regarding $different, will do that next. 12 needs more looking into as I'm not sure what is being caught there. Rest is done.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 155: 2625696-155.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
193.11 KB
3.61 KB

Remaining feedback resolved. The exception only needed to be the requirements exception for the same reason as the derivers.

Fixed the tests after a conflict with #2225477: Add migrate sources and destinations for D6 i18n variables

Status: Needs review » Needs work

The last submitted patch, 157: 2625696-156.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
193.5 KB
1.92 KB

Fixes the two failing tests that were introduced in the last patch. I had to catch the database exception for the exact same reason as the derivers.

vasi’s picture

I may be misunderstanding things, but I tried this to get a list of migrations:

$migration_manager = \Drupal::service('plugin.manager.migration');
$migrations = $migration_manager->createInstances([]);

This currently seems to fail for me in most situations. If I enable just the migrate module, it fails because D6NodeDeriver tries to get the service "plugin.manager.migrate.cckfield", that lives in migrate_drupal. If I enable migrate_drupal, it fails because the migrate_drupal database is not configured. I suppose if I configured a DB it would likely work, but we have to assume some people will use migrations that have nothing to do with upgrades.

* Is my code snippet the right way to do this?
* How do we distinguish between migrations that exist, and migrations that are ready to run?
** Eg: Should we make the D6/D7 derivers simply yield nothing when the migrate database is not configured?
** Should dependencies factor into this?

benjy’s picture

@vasi, where are you doing that? Seems like something that would be done in a migrate contrib project. The dependency issue you describe already exists in core, see #2560795: Source plugins have a hidden dependency on migrate_drupal - we did discuss moving D6Deriver back to migrate_drupal earlier in this issue which would solve the problem.

Theres a follow-up to work out how sources/destinations get their configuration injected, as you mention, it currently throws an error connecting if you don't have a source database configured. We can discuss whether migrations that don't have their config should be silently skipped in #2681869: Provide clean way to merge configuration into migration plugins

alexpott’s picture

@vasi - good question - see Drupal\migrate_drupal\MigrationCreationTrait::getMigrations() - although I'm inclined to agree that $migrations = $migration_manager->createInstances([]); should work and only create migrations that can be run - however I think we'll need to fix #2681869: Provide clean way to merge configuration into migration plugins to make this right.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

@benjy's changes in response to @catch's review look great. Given that I haven;t worked on this since the last rtbc - I'm happy to re-rtbc it.

vasi’s picture

@benjy, I'm just doing that in a script now, to play with and understand the new system. I assume eventually migrate_tools will need to do something similar.

I should add that I really like the idea of using plugins instead of config entities in the abstract. I think it might come in really useful for i18n migrations, for example, having a TranslationDeriver that takes care of some or all of the dirty work. I just want to make sure that this patch ends up being the best it can be, not trying to make trouble.

mikeryan’s picture

@vasi's issue was previously raised at https://www.drupal.org/node/2625696#comment-10863860 - yes, this is a concern for migrate_tools. Good to hear #2681869: Provide clean way to merge configuration into migration plugins should handle this.

xjm’s picture

Issue tags: +beta target

Hooray, glad to see this RTBC again!

Tagging as a beta target; this issue is a priority for the 8.1.x beta phase.

xjm’s picture

Issue tags: +8.1.0 release notes

Also.

alexpott’s picture

Rerolled - some clashes with the unused use statements...

catch’s picture

OK. Committed/pushed to 8.2.x.

I have not yet cherry-picked to 8.1.x, after discussing with @alexpott:

- we can cherry-pick to 8.1.x whenever before tagging the second beta, but having this in 8.2.x helps to un-stack some patches
- there's at least one follow-up like the early return in test code it'd be good to have a green patch for and/or commit both to 8.2.x then cherry pick both to 8.1.x, couldn't decide how strongly I felt about that, so leaving at 8.2.x for now lets us get things going regardless.

Leaving RTBC against 8.1.x since we still might cherry-pick as-is.

Gábor Hojtsy’s picture

Should we publish the change record once the 8.1 merge happens? I assume that would be best for publicity.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Yep makes sense

Cherry-picked to 8.1.x now - several follow-ups already in and don't want the branches to diverge too much yet.

  • catch committed 732300b on 8.2.x
    Issue #2625696 by chx, benjy, alexpott, catch, mikeryan, andypost,...

  • catch committed 3839195 on 8.1.x
    Issue #2625696 by chx, benjy, alexpott, catch, mikeryan, andypost,...
matslats’s picture

I spent the last day trying to migrate my content entity type from d7 using 8.1.0-beta1
I'm now stuck because my entityTypeId changed between versions and the d7_field migration cannot know that.
I will now try to resave the d7_field migration to add a process plugin to map the entityTypeId;

Also some dx feedback for you.
It was tough workflow editing my yml file because drupal stores it as config. I had to delete the config and reimport the yml file whenever I made a change.
Also once the migration had failed it didn't run again. I had to delete rows in the db or change the source_row_status column.

hussainweb’s picture

Possible regression due to this change: #2691189: Rerunning a migration results in an error.

Status: Fixed » Closed (fixed)

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