Problem/Motivation

Over in #2202511: Implement migration groups we are adding group support and there was talk of adding a way to store configuration for the group.

Storage of configuration shared among related migrations, most commonly source information - migrate_d2d stores database connection info in the group, wordpress_migrate stores the location of the source WXR file, etc.

Proposed resolution

Discuss our options.

Remaining tasks

Write the patch.

User interface changes

n/a

API changes

Possible additions.

CommentFileSizeAuthor
#43 interdiff.txt1.75 KBmikeryan
#43 support_shared-2302307-43.patch13.91 KBmikeryan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,179 pass(es). View
#35 2302307-35.patch12.05 KBbenjy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,964 pass(es), 12 fail(s), and 384 exception(s). View
#35 interdiff.txt2.65 KBbenjy
#26 interdiff.txt1.28 KBmikeryan
#26 support_shared-2302307-26.patch11.96 KBmikeryan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,824 pass(es). View
#24 interdiff.txt617 bytesbenjy
#24 2302307-24.patch11.94 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,730 pass(es). View
#22 interdiff.txt3.21 KBbenjy
#22 2302307-22.patch11.94 KBbenjy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,730 pass(es), 127 fail(s), and 92 exception(s). View
#18 interdiff.txt1.04 KBmikeryan
#18 support_shared-2302307-18.patch9.81 KBmikeryan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,415 pass(es). View
#15 support_shared-2302307-15.patch9.84 KBmikeryan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,027 pass(es). View
#10 interdiff.txt1.67 KBmikeryan
#10 support_shared-2302307-10.patch5.04 KBmikeryan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,978 pass(es). View
#9 interdiff.txt1.82 KBmikeryan
#9 support_shared-2302307-9.patch5.19 KBmikeryan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,961 pass(es). View
#4 support_shared-2302307-4.patch4.01 KBmikeryan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,166 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

mikeryan’s picture

Time to discuss our options. I have a patch for migrate_drupal at #2404401: Use dependency injection for the database connection which takes advantage of #2404397: Support injection of database configuration in SqlBase to store database credentials in the active configuration for each migration - it would of course be much cleaner to store them in one place, the group, not least because it would be much simpler to update the credentials (moving the source db to a different server, belatedly deciding to use a proper read-only SQL user instead of root, etc.).

So, I see migration_group as a new configuration entity. It would support id (machine name) and label, and apart from that could store any configuration relevant to the particular application (db credentials for any SQL-based migrations, location of files for a migration from CSV or XML, etc.). I think this should be relatively straightforward.

The main question in my mind is where to maintain the relationship between migrations and their groups. I believe we are agreed there is value is allowing a migration to appear in multiple groups. Do we extend the current migration_groups support (using machine names instead of labels)

id: d6_book
label: Drupal 6 books
migration_groups:
  - drupal_6
  - drupal_7
...

or list the migrations in the group (note the source information would be written to active configuration based on dynamically provided configuration, not provided in the original .yml file) - much like a manifest that's part of the active configuration?

id: drupal_6
label: Drupal 6
migrations:
  - d6_action_settings
  - d6_aggregator_feed
...
arguments:
  source:
    key: migrate6
    target: default
    database:
      database: my_d6_site
      username: readonly
      password: foobar
      prefix: ''
      port: '3306'
      host: 12.34.56.78
      namespace: Drupal\Core\Database\Driver\mysql
      driver: mysql

Edit: wrapped added configuration under 'arguments'

Thinking about some use cases - migrate_upgrade loading the migrations to run, a dashboard UI or drush migrate-status loading migrations to display - it seems clearly more straight-forward to me to have the group contain the list of migrations, rather than scanning all active migrations to find a matching group. The first approach makes it simpler to see what groups a migration belongs to, but that should be a relatively rare need, not to mention since there will be far fewer groups than migrations there's less to scan for the answer.

Unless anyone sees a problem with the general configuration entity approach, I'll start working on a patch.

mikeryan’s picture

I'm digging in, and have a hackish version of this sort of almost working with migrate_upgrade. One element I didn't account for above, however, is exactly when and how the group arguments get merged into the migration instance. If a migration is in more than one group, which one does it pull arguments from when loaded? My hack at the moment is to set migration_groups on each migration (in the active configuration) to the specific migration_group being used, then when loading the migration load the specified group and array_merge the group's arguments into the migration's configuration (not saving to active config, just using it locally). It's still not quite working right though.

I'm not seeing a clean way to deal with this at the moment (maybe just because it's Friday afternoon of a long week). Now I'm kind of feeling like addressing #2202475: Registering/instantiating migrations first - this part will make more sense if we're dealing with concrete instances. Guess I'll let it percolate over the weekend...

benjy’s picture

it seems clearly more straight-forward to me to have the group contain the list of migrations, rather than scanning all active migrations to find a matching group

Would this mean though that users writing migrations by hand then have to create a group as well as list the migrations in that group in a separate file?

It's a bit more complicated than simply adding a group name in the yaml file like we currently have.

mikeryan’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
4.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,166 pass(es). View

Finally got around to doing this. The attached patch adds MigrationGroupInterface/MigrationGroup. Group config contains a shared_configuration key, which provides default values to be applied to all migrations which reference the group through the active_groups key. When a Migration is constructed, if it contains an active_group key which is the machine name of a MigrationGroup instance, it will merge in any value in shared_configuration that is not already explicitly set. There are a couple of POC patches using this:

  1. migrate_upgrade: When submitting the upgrade form, a migration group is created containing the provided database credentials, and each specific migration references is through active_group. Once the migration is run, the migrate_plus drush commands can be used to review the results.
  2. migrate_plus: The drush migrate-status command will display migrations by group. Both migrate-status and migrate-import take --group arguments to filter their operations. The beer example in migrate_example creates a group and assigns the migrations to it.

Ideally, I'd like the key pointing a specific migration instance to its group to be "migration_group" rather than "active_group". Right now, we have the "migration_groups" key, but that is being used more as tagging rather than full-fledged groups. Actually, in the current migrate_drupal usage, I'd rather see a migrate_drupal-specific key "drupal_versions", since what it's really doing is listing what Drupal versions the given migration configuration applies to. I tried implementing this but got stuck trying to figure out how to define and apply the configuration schema properly, so figured I should first focus on the basic group functionality.

andypost’s picture

Why migration_group a config entity?
It's more like content that describes a migration process with migrations ER field pointing to migration config entities.

mikeryan’s picture

I don't see how the migration group would be content? Consider a custom migration project - you work on it in a development environment, tweaking the individual migrations' configuration (field mappings etc.) and the common configuration all migrations should have access to, and ultimately want to deploy that configuration to staging and ultimately production environments.

alexpott’s picture

A migration group should have a module field to store the providing module and this should be used for dependency calculation. If migrations are config, then migration groups feel like config to me - especially since they are going to be storing configurable data.

alexpott’s picture

A migration group should have a module field to store the providing module and this should be used for dependency calculation. If migrations are config, then migration groups feel like config to me - especially since they are going to be storing configurable data.

mikeryan’s picture

FileSize
5.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,961 pass(es). View
1.82 KB

Took a stab at adding the module field and accounting for it in the dependencies, but it doesn't seem to be working - when creating a group in migrate_upgrade I added module: migrate_drupal, but uninstalling migrate_drupal did not remove the group. What am I missing?

Also, deleting a group should delete the migrations attached to it - added support for that (untested, since the group isn't getting deleted...).

mikeryan’s picture

FileSize
5.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,978 pass(es). View
1.67 KB

Oops, accidentally committed some dpm() calls.

Note that calculateDependencies() is never called.

alexpott’s picture

@mikeryan how did you create the migration group?

mikeryan’s picture

I had migrate_upgrade doing this:

    $group_id = 'upgrade_' . $drupal_version;
    $group_config = \Drupal::configFactory()
      ->getEditable('migrate.migration_group.' . $group_id);
    $group_config->set('id', $group_id);
    $group_config->set('label', $this->t('Upgrade from Drupal !version',
      array('!version' => $drupal_version)));
    // Make the group dependent on migrate_drupal, so it is uninstalled when
    // migrate_drupal is.
    $group_config->set('module', 'migrate_drupal');
    $group_config->set('shared_configuration.source.key', 'upgrade');
    $group_config->set('shared_configuration.source.database', $database);
    $group_config->save();

Looking in the config table, migrate.migration_group.upgrade_6 looked good.

alexpott’s picture

@mikeryan you need to create an entity to use the config entity system. So something like:

$entity = entity_create('migration_group', $values);
$entity->save();
mikeryan’s picture

OK, so I had a false equivalence in my head between a configuration entity and the configuration that defines it. If we have a YAML file in config/install describing a configuration entity, that results in the creation of an entity, but that doesn't mean the entity and the configuration are the same thing. So creating the entity in PHP saves the configuration to the active configuration, but simply saving the configuration does not create an entity.

Basically, replacing the configFactory call with an entity_create did the job. When I uninstalled migrate_drupal, the group and all the migrations it contained were deleted. So, this patch seems to be on the right track - next step is adding some tests.

Thanks!

mikeryan’s picture

Issue tags: -Needs tests
FileSize
9.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,027 pass(es). View

OK, fully ready for review. No interdiff, it would be about the same size as the full patch. These are the changes:

  1. I had previously been merging group configuration in the migration constructor. It makes more sense to do it at load time.
  2. Added tests, for the configuration merge and for deletion.
  3. I've changed the configuration key referencing the group to 'migration_group'. The existing 'migration_groups', which is not directly related to groups in this sense, needs to be renamed, I'll add a separate issue for that.
benjy’s picture

I read the code, it all looks good, nice work. I'd still like to have a test with this patch applies but a few nitpicks below.

  1. +++ b/core/modules/migrate/src/Entity/MigrationGroup.php
    @@ -0,0 +1,83 @@
    + *     "weight" = "weight"
    

    Why do groups need weight?

  2. +++ b/core/modules/migrate/src/Tests/MigrationGroupTest.php
    @@ -0,0 +1,111 @@
    +
    +  protected function setUp() {
    +    $this->strictConfigSchema = FALSE;
    

    Missing docs

mikeryan’s picture

Status: Needs review » Needs work

Why do groups need weight?

Primarily because of copypasta from the Migration definition... Actually, it would be nice for UIs to provide the ability to order groups for display, but given probably 90-95% of sites using migrate will have one migration group, and 99% of the remainder no more than three or four, sorting them isn't a priority, we can hold off till someone eventually opens a minor feature request.

Missing docs

Will add, updated patch coming, thanks.

mikeryan’s picture

Status: Needs work » Needs review
FileSize
9.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,415 pass(es). View
1.04 KB
benjy’s picture

Nice work, I think this is RTBC for me. One small thing below:

+++ b/core/modules/migrate/src/MigrationStorage.php
@@ -90,4 +92,42 @@ protected function addDependency(array &$graph, $id, $dependency, $dynamic_ids)
+        $group = \Drupal::entityManager()->getStorage('migration_group')->load($migration->migration_group);

We could inject this.

mikeryan’s picture

@benjy: I'm not quite sure I follow - what would be injected, the migration_group storage manager?

Thanks.

benjy’s picture

Yes exactly. See ConfigEntityStorage::createInstance() for an example.

benjy’s picture

FileSize
11.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,730 pass(es), 127 fail(s), and 92 exception(s). View
3.21 KB

OK, I've added that. I think this is RTBC.

Status: Needs review » Needs work

The last submitted patch, 22: 2302307-22.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
11.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,730 pass(es). View
617 bytes

Doh! I fixed this once and then un-did it by accident.

anavarre’s picture

  1. +++ b/core/modules/migrate/src/MigrationStorage.php
    @@ -90,4 +141,42 @@ protected function addDependency(array &$graph, $id, $dependency, $dynamic_ids)
    +              // Otherwise, the existing migration value overrides the group value.
    

    80 chars

  2. +++ b/core/modules/migrate/src/Tests/MigrationGroupTest.php
    @@ -0,0 +1,113 @@
    +      'migration_groups' => array('Drupal 6'), // In migration, so will be overrideen.
    

    s/overrideen/overridden (I *think* 80 chars doesn't apply for inline comments)

Also, shouldn't we replace entity_create() by \Drupal::entityManager()->getStorage($entity_type)->create($values); ?

mikeryan’s picture

FileSize
11.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,824 pass(es). View
1.28 KB

@benjy: Thanks for the injection!

@anavarre: Here you go.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Great, I think this is ready. I only added the boiler plate injection so I think i'm still good to RTBC this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: support_shared-2302307-26.patch, failed testing.

mikeryan’s picture

Status: Needs work » Reviewed & tested by the community

Restoring status after testbot glitch.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/migrate/src/Tests/MigrationGroupTest.php
@@ -0,0 +1,113 @@
+    $this->strictConfigSchema = FALSE;

Why is this needed?

Status: Needs review » Needs work

The last submitted patch, 26: support_shared-2302307-26.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
2.65 KB
12.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,964 pass(es), 12 fail(s), and 384 exception(s). View

Wow, great spot. There were a few issues in the tests to do with schema, i've fixed them up.

ultimike’s picture

Status: Needs review » Reviewed & tested by the community

This looks good - it'll be great to get this one committed.

thanks,
-mike

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
    migration_groups:
      type: sequence
      label: 'Migration Groups'
      sequence:
        type: string
        label: 'Group'
    migration_group:
      type: string
      label: 'Active group'

This is going to be super confusing. Also I don't get how we're going to fix the dependency problem if the active group is changeable - what is the situation where a migration can belong to more than one group?

mikeryan’s picture

re: confusing - see the followup issue #2466797: Rename migration_groups key to avoid confusion with MigrationGroup support.

A migration can't belong to more than one group (in the concrete sense of group we're using in this issue). A migration can have more than one tag (what migration_groups has been used for).

mikeryan’s picture

Can we get this back to RTBC?

Thanks.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

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

mikeryan’s picture

Hrrm, let me see if I need to reroll...

mikeryan’s picture

Status: Needs work » Needs review
FileSize
13.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,179 pass(es). View
1.75 KB

The new CCK plugin manager injection conflicted with the migration group storage injection we're adding here - rerolled.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

xjm’s picture

Assigned: Unassigned » alexpott

Assigning to @alexpott for his followup per his request.

alexpott’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Postponed

In a recent meeting we agreed that migration groups should be explored in contrib as part of the migrate UI. Postponing this issue as a result.

benjy’s picture

Assigned: alexpott » Unassigned
Status: Postponed » Closed (won't fix)

I'm going to close this as part of some triage. It is currently in the migrate_plus module.