See: #2459159: Cannot reinstall module after uninstall

This seems to come up over and over with migration implementations. Having to add an enforced dependency key on the parent module always doesn't seem to make sense. Presumably migrations always need to depend on the module they are migrating data for?

References:
https://www.drupal.org/node/2235409
https://www.drupal.org/node/2404447

Files: 
CommentFileSizeAuthor
#70 2460529.70.patch100.29 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,526 pass(es). View
#70 67-70-interdiff.txt3.71 KBalexpott
#67 2460529.67.patch100.78 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,562 pass(es). View
#67 65-67-interdiff.txt2.82 KBalexpott
#66 59-65-interdiff.txt2.61 KBalexpott
#65 2460529.65.patch99.42 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,375 pass(es). View
#60 54-59-interdiff.txt2.11 KBalexpott
#59 2460529_58.patch98.72 KBchx
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Comments

xjm’s picture

alexpott’s picture

Priority: Normal » Major

Yep and we should be able to calculate this and not have to use enforced dependencies. Considering Migrate is using config entities it has to embrace them fully which means it needs to embrace config dependencies. For starters, the config plugin should add a dependency on the module based on the provided config_name.

alexpott’s picture

Turns out this is not so simple migrate_drupal is a special flower that is kind of providing configuration on behalf of lots of modules.

We could move all of the configuration into the optional folder and work on it from there but then we're going to need to improve the installation of optional configuration. It will to detect the following situation:

  1. Install minimal
  2. Enable migrate and migrate_drupal - at this point only the migrations that have their dependencies satisfied will be created. d6_aggregator_settings would not be created because it depends on the aggregator module.
  3. Install aggregator - at this point we should create d6_aggregator_settings but in HEAD this won't occur because we only search optional configuration for config beginning with the module name (in this case aggregator) and we only do that if the module provides a config entity type. What this means is that we'll have to read all optional configuration, filtering out the objects that already exist or that don't begin with an installed module name and check to see if the extension that is being installed is in the list of dependencies - if it is then create it.

If the wish to behave like that would only be for Migrate then I think this would just be another nail in the Migration configuration entity coffin - however I think is a module wants to provide a configuration entity and lots of optional integration then this would be very useful.

alexpott’s picture

Status: Active » Needs review
FileSize
45.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,311 pass(es), 88 fail(s), and 127 exception(s). View

This will probably fail hard on migrate tests because (afaik) futz with dependencies and don't always set up a complete working environment.

Also I really think having migrations in config is wrong - once migrate drupal has the d7 migrations active config and config exports is going to be full of things which can not and should be configured. But as I said in #4 I think similar problems will exist in contrib for modules that want to provide a configuration entity type and example integrations.

alexpott’s picture

You can test the patch in #5 by following the step in #4 after installing migrate_drupal d6_aggregator_settings will not created. Install aggregator and it is. Uninstall aggregator and it is removed.

Status: Needs review » Needs work

The last submitted patch, 5: 2460529.5.patch, failed testing.

alexpott’s picture

Title: Migrations probably always need to depend on the providing module » Migrations need to use the configuration entity dependency system
Status: Needs work » Needs review
FileSize
67.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,422 pass(es), 2 fail(s), and 1 exception(s). View
33.33 KB

So I think we should split out the CMI changes into a separate issue that blocks this one. I think this issue should aim to remove migration_dependencies and replace it with config entity dependencies. I asked in IRC what migration means by optional dependencies and according to @mikeryan1 they are UI ordering so I think they should move into a more appropriate key name.

The patch attached continues to build on the work and is perhaps green (fingers-crossed).

mikeryan’s picture

There seems to be some confusion over the migration_dependencies, as if it somehow conflicts with configuration entity dependencies. That's a different kind of dependency - it has nothing to do with whether the migration is valid or the config entity should be installed, it's saying "don't run this migration until the migrations it depends on have completed" (e.g., you don't want your node migration, which references migrated users, to run before you've migrated all the users).

I do admit to remaining confused over exactly how config dependencies are supposed to work. I will say that I always envisioned that a contrib module would provide the migration config entities for migrating its data, and I think core modules should do the same - e.g., the aggregator module, rather than migrate_drupal, should contain the default migration config for its own entities.

alexpott’s picture

This issue contains the whole of #2460847: Allow optional configuration to be installed when its dependencies are met and should be postponed on that - but we can continue working to add all of the dependencies so not actually postponing.

Status: Needs review » Needs work

The last submitted patch, 8: 2460529.8.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
91.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,529 pass(es), 2 fail(s), and 2 exception(s). View
57.61 KB

So I think I've got most of the relevant dependencies added...

Status: Needs review » Needs work

The last submitted patch, 12: 2460529.12.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.28 KB
92.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2460529.14.patch. Unable to apply patch. See the log in the details link for more information. View

Oh look at that... config dependencies making things more robust and better tested :)

mikeryan’s picture

Close - uninstalling migrate_drupal now leaves only 4 migration config entities behind:

migrate.migration.d6_block_content_body_field
migrate.migration.d6_block_content_type
migrate.migration.d6_upload_field
migrate.migration.d6_user_picture_field

Looks like they just need migrate_drupal added to their module dependencies.

mikeryan’s picture

Status: Needs review » Needs work
mikeryan’s picture

I should point out that, unlike the previous condition, re-enabling migrate_drupal with the orphaned migrations present works fine, no errors about already-existing configuration.

alexpott’s picture

Status: Needs work » Needs review

So those migrations have absolutely no code dependency on anything provided by migrate_drupal afaics. If we want them to depend on it then we'll have to enforce it somehow but it that right thing to do? I'm not convinced.

xjm’s picture

So I thought of a couple of usecases that are worth considering/explaining (contrary to my original issue title yesterday):

  • In Drupal 7, Taxonomy fields belong to the Taxonomy module. In Drupal 8, they are as of today a specific configuration of Entity Reference field, that also depends on Taxonomy. Presumably these migrations would belong to ER in D8, with a dependency on Taxonomy?
  • In Drupal 7, some functionality was provided by funkatron.module, which no longer exists and has been merged into a new module in Drupal 8, cobblepot.module. Presumably in D8 the Cobblepot module provides a migration for Funkatron data and automatically depends on Cobblepot as well.
  • The maintainer for a wonder_contrib.module doesn't want to include migration, so I make my own wonder_contrib_migration.module that has no functionality of its own, but merely provides migrations for wonder_contrib. Obviously the migrations need to depend on wonder_contrib, but do they need to depend on wonder_contrib_migration?
alexpott’s picture

@xjm at the moment this patch is ensuring the the migration has dependencies on:

  • the source plugin provider
  • the target plugin provider
  • the entity type of any plugins derived from the existence of en entity type
  • the entity type provider if the migration has a constant labelled entity_type
  • the module if the migration has a constant labelled entity_type

So to answer your points in order:

  • We probably should depend on the d8 field type provider too - however entity reference is a core type and the entity reference module is scheduled to be deleted, Already these migrations depend on taxonomy because they are migration to an entity type provided by that module.
  • The cobblepot will at least provide the source plugin to get funkatron's data so yep the migration will depend on Cobblepot
  • Depends on whether or not wonder_contrib_migration has to provide a plugin - it is does then it will depend on it. If it does not atm it won't. But this should be discussed more. I will open an issue to discuss automatically adding a dependency on the extension causes the config to be created during installation.
mikeryan’s picture

Those migrations might not require migrate_drupal plugins, but do they make any sense in the absence of migrate_drupal? If you've uninstalled migrate_drupal, why would you want a few random migrations sticking around?

That being said, now that the leftover migrations don't prevent the module from being re-enabled (i.e., this fixes #2459619: migrate_drupal cannot be uninstalled and reinstalled), it's not a showstopper for me... I expect, though, that if it remains as-is there will be issues periodically opened down the road asking why disabling migrate_drupal left those migrations behind.

I look at wonder_contrib_migration the same way - if this module is not providing any code to support the migration, only configuration, it seems like a WTF if uninstalling the module has no effect - the configuration stays.

alexpott’s picture

So if we are to proceed with this:

  • migrate.migration.d6_block_content_body_field
  • migrate.migration.d6_block_content_type
  • migrate.migration.d6_upload_field
  • migrate.migration.d6_user_picture_field

Need to depend on migrate_drupal because they are part of the Drupal 6 migration - which is provided by that module. It looks like we need a migration group entity or some way of determining which module provides the group.

benjy’s picture

I think those 4 migrations depending on migrate_drupal seems fine.

Here's the issue for migration groups: #2302307: Support shared configuration between migrate groups

alexpott’s picture

Considering that we still expect migrations to be hand crafted we need to exempt migration configuration from being trusted during an install.

To solve #22 we will end up moving all migrations to the module that should provide them so it will no longer be an issue.

phenaproxima queued 14: 2460529.14.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 14: 2460529.14.patch, failed testing.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Patch needs a re-roll. Self-assigning.

phenaproxima’s picture

Assigned: phenaproxima » Unassigned

@alexpott said on IRC that he'd rather do the reroll, so...here you go!

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.67 KB
85.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,291 pass(es), 1 fail(s), and 0 exception(s). View

Talked to @phenaproxima in IRC since this involved the ConfigInstaller - I was already working on the reroll.

Status: Needs review » Needs work

The last submitted patch, 29: 2460529.28.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
674 bytes
85.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,293 pass(es). View

Fixing the test

alexpott’s picture

FileSize
825 bytes
85.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,502 pass(es), 93 fail(s), and 77 exception(s). View

A couple of fixes.

Status: Needs review » Needs work

The last submitted patch, 32: 2460529.32.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
543 bytes
85.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,290 pass(es), 4 fail(s), and 0 exception(s). View

More failyay!

Status: Needs review » Needs work

The last submitted patch, 34: 2460529.34.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
86.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,285 pass(es). View
1.85 KB

A few more...

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks good to me and appears to work well. I'd feel even more confident if someone else gave it a +1, but as far as I'm concerned this is RTBC.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review

Actually I just realized that even though @alexpott wrote 99.999% of this patch and it's fair for me to RTBC it from a technical standpoint, I made a few changes myself so I probably shouldn't. :)

chx’s picture

Status: Needs review » Needs work

+ return $this->migration_dependencies + array('required' => [], 'optional' => []);

Short array style is encouraged but mixed style is frowned upon.

  public function trustData() {
    // Migrations cannot be trusted since they are often write by hand or as the
    // result of a copy/paste.
    $this->trustedData = FALSE;

a) trustData vs trustedData b) they are often written? grammar check.
+ public function getConfiguration();
Shouldn't we extend from ConfigurablePluginInterface instead?
+ list($provider,) = explode('.', $this->config->getName(), 2);
Why not [0] instead of list since 5.4.0 you can dereference directly so explode()[0]
+abstract class DestinationBase extends PluginBase implements MigrateDestinationInterface, RequirementsInterface, DependentPluginInterface {
+abstract class SourcePluginBase extends PluginBase implements MigrateSourceInterface, DependentPluginInterface {
see, that's the DependentPluginInterface I recommended that the Migration{plugintype}Interface should now descend from
+ $this->addDependency('module', \Drupal::entityManager()->getDefinition($this->configuration['constants']['entity_type'])->getProvider());
sad panda. SourcePluginBase needs to hardwire entity?

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.85 KB
86.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,297 pass(es). View

@chx thanks for the review.

  1. Fixed short array and long array mixing.
  2. a) The method is a verb and the property is an adjective + noun :) - fixed the grammar in the comment. No migrations are not written to often and the as the above failyays prove it's hard to get all the dependencies correct so recalculating on save even during install is worth it.
  3. Used explode('.', $this->config->getName(), 2)[0]
  4. MigrateSourceInterface and MigrateDestinationInterface now both extend DependentPluginInterface
  5. Yes constants.entity_type is used all of the place

@phenaproxima re #36 I'd argue you're in an ideal spot to rtbc this - your patch just fixed the dependencies in existing migrations.

chx’s picture

Regarding constants/entity_type: in a generic migration case, the migration must define its dependencies explicitly. Short of running the migration first it's practically impossible to predict d6_field_instance_widget_settings will always yield node as entity_type -- there is nothing really that stops the source plugin from overriding the "constants" array. It's not really constants, it's just defaults, we called it constants to perhaps better explain what's happening but remember, there's nothing special about 'constants' -- the whole source plugin configuration is used as defaults for Row.

In a migrate_drupal case, we have much better control. We know what the source plugin is doing, we know what the process is doing and we are quite sure it'll be always node so if we want, we can save the explicit dependency on node by moving this code to the relevant migrate_drupal class, DrupalSqlBase.

alexpott’s picture

Conversation with @chx in IRC

11:54 alexpott
chx: well if the SourcePlugin that is implemented by the migration can determine the dependencies it should
11:55 alexpott
chx: and why not have a generic ability to declare a constant that adds a dependency on that provider of that entity type
11:55 alexpott
chx: i.e. what's the harm in this?
11:55 alexpott
chx: "there is nothing really that stops the source plugin from overriding the "constants" array. It's not really constants"
11:56 alexpott
chx: is kinda scary
11:57 alexpott
chx: but if a plugin does that then it's up to the plugin to correctly implement calculateDependencies
11:59 chx
alexpott: a generic source plugin has no clue really about dependencies
11:59 chx
alexpott: it perhaps know it will yield a contsants/entity_type every row which is always node but what happens after, hell knows
11:59 ultimike has left IRC (Ping timeout: 256 seconds)
12:00 chx
alexpott: in general, source yields an arrow of source propery => source value
12:00 chx
alexpott: it has no idea what happens to it
12:01 alexpott
chx: yeah I get that
12:01 chx
alexpott: you can then process it ... and then save it to a third party system
12:01 chx
alexpott: no, source in generic cant yield any dependencies
12:02 chx
alexpott: hell... even in the migrate_drupal case it should be the _entity_ that yields that in full knowledge of the source AND process AND destination confgiuration
12:02 alexpott
chx: I agree
12:02 alexpott
chx: but this information is not available
12:03 chx
oh?
12:03 chx
who have eaten it?
12:03 alexpott
chx: because the constant system is working incorrectly
12:04 alexpott
chx: constants should be decoupled from source
12:04 chx
maybe? but they are not constants, they are defaults and that was deliberate?
12:05 chx
it seemed like a simple way to get it
12:05 chx
but
12:05 chx
if you want
12:05 chx
we can provide a separate defaults section
12:05 chx
not sure how much better that is?
12:06 alexpott
chx: imo this conversation just convinces me yet again that migration configuration is not configuration

So the problem here is that we have a situation where migration does not want to behave like configuration. I agree that in an ideal world the source plugin should not have any idea about dependencies in the destination system BUT in order for this to be a reality we need to answer the following questions:

  1. Why are constants not constant? @chx said that source plugin's constants are more like defaults. But having changeable configuration at run-time is not configuration behaviour. Configuration assumes that the static configuration is the runtime state.
  2. If we don't use dependencies from the source plugin how can d6_user_contact_settings get a dependency on the contact module?
  3. If we don't use dependencies from the source plugin how can d6_node_setting_status get a dependency on the node module? And all of the entity type providers used by:
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_block_content_body_field.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_comment.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_comment_entity_display.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_comment_entity_form_display.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_comment_entity_form_display_subject.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_comment_field.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_comment_field_instance.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_comment_type.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_field.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_field_formatter_settings.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_field_instance.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_field_instance_widget_settings.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_node_setting_promote.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_node_setting_sticky.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_upload_entity_display.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_upload_entity_form_display.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_upload_field.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_upload_field_instance.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_user_picture_entity_display.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_user_picture_entity_form_display.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_user_picture_field.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_user_picture_field_instance.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_user_profile_entity_display.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_user_profile_entity_form_display.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_user_profile_field.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_user_profile_field_instance.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_vocabulary_entity_display.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_vocabulary_entity_form_display.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_vocabulary_field.yml
    • ./core/modules/migrate_drupal/config/install/migrate.migration.d6_vocabulary_field_instance.yml
chx’s picture

Most of this seems easy to me: add explicit dependencies to the migration entities.

> If we don't use dependencies from the source plugin how can d6_user_contact_settings get a dependency on the contact module?
By adding

dependencies:
  - contact

to the migration entity? Why should this be automatically derivable? More on this later.

> But having changeable configuration at run-time is not configuration behaviour. Configuration assumes that the static configuration is the runtime state.

Huh? There is no changable configuration at runtime. Rows have defaults. We provide the default (which, being a default, is quite static and doesn't change between rows or migrations) by using the source configuration as the default. That's all there is.

In other words... migrate does not provide semantics for module: 'constants/module'. It's a default. Whether it gets used, migrate doesn't know and there's no way to know until a row is yielded. And even if the constant gets used, whether it's a Drupal 8 module/provider or a module in university education, migrate certainly has no such knowledge nor do I think this would be easy to codify. How meta do you want to be today :) ?

For migrate_drupal we know the semantics, we know the pattern: constants/entity_type will end up used as a Drupal 8 entity_type. This is a mere convention absolutely not enforced anywhere inside migrate (or migrate_drupal, mostly), not at all. More of a happenstance than anything else. We set the entity_type on a configuration entity and then eventually it gets read by the entity system and that will try to use it as a D8 entity type. We can reuse this knowledge of migrate_drupal and Drupal 8 patterns in general to add a node dependency if source has constants/entity_type: node but this is very strongly a migrate_drupal specific shortcut. It might be too much magic or it might not be, this I can't decide. It feels convenient to me for sure.

chx’s picture

Some random links as to what "entity type" and "module" can mean:

https://msdn.microsoft.com/en-us/library/vstudio/bb738486%28v=vs.100%29....

http://golaem.com/content/doc/golaem-crowd-documentation/entity-type

http://www.ucc.ie/en/international/visiting/modules/

http://en.wikipedia.org/wiki/Module_%28mathematics%29

Just because a migration entity mentions these phrases it doesn't mean they are what Drupal uses them for, irregardless whether their value is a constant or not.

alexpott’s picture

FileSize
2.04 KB
87.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Well I certainly buy the fact that this is Drupal source plugin specific. Move the code to DrupalSqlBase.

#42 was about the fact that I agree with the idea the in fact source plugins should not actually cause configuration dependencies since these are about the destination not the source. But maybe we just have to make do.

Status: Needs review » Needs work

The last submitted patch, 45: 2460529.45.patch, failed testing.

benjy’s picture

  1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
    @@ -144,4 +144,18 @@ protected function variableGet($name, $default) {
    +      $this->addDependency('module', \Drupal::entityManager()->getDefinition($this->configuration['constants']['entity_type'])->getProvider());
    

    Can we inject this?

  2. +++ b/core/modules/migrate/src/Entity/Migration.php
    @@ -478,6 +478,31 @@ public function setTrackLastImported($track_last_imported) {
    +  public function trustData() {
    +    // Migrations cannot be trusted since they are often written by hand and not
    +    // through a UI.
    +    $this->trustedData = FALSE;
    +    return $this;
    

    This is an interesting pattern, i'd have more expected that we'd override hasTrustedData() ?

  3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
    @@ -23,4 +23,19 @@ public function getIds() {
    +      $field_type = \Drupal::service('plugin.manager.field.field_type')->getDefinition($source_configuration['constants']['type']);
    

    Needs injecting

phenaproxima’s picture

Status: Needs work » Needs review
  1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ViewMode.php=
    +  public function calculateDependencies() {
    +    $this->dependencies = parent::calculateDependencies();
    +    if (isset($this->configuration['constants']['targetEntityType'])) {
    +      $this->addDependency('module', \Drupal::entityManager()->getDefinition($this->configuration['constants']['targetEntityType'])->getProvider());
    +    }
    +    return $this->dependencies;
    +  }
    +
        

    Can this be injected?

Beyond this, and @benjy's suggestions above, I'm ready to RTBC. Looks good to me, and the idea of migrations using the core dependency system makes me happy inside.

phenaproxima’s picture

Status: Needs review » Needs work

Whoops. Fixing status.

alexpott’s picture

Status: Needs work » Needs review
FileSize
11.3 KB
95.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Thanks @phenaproxima and @benjy for the reviews.

Re #47.2 we need to override this because $this->trustedData is used with Entity::save().

So I've injected everything. Also discovered that EmptySource needs the dependency handling for constants too - which makes sense and I think is okay because although it's part of migrate it is a special flower.

Status: Needs review » Needs work

The last submitted patch, 50: 2460529.50.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
96.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,449 pass(es), 4 fail(s), and 0 exception(s). View

I am having trouble creating an interdiff. Here's what I've done:

  1. I reverted EmptySource, MigrateSourceInterface, SourcePluginBase EntityFieldStorageConfig.
  2. The code residing in EntityFieldStorageConfig is now overridden in a EntityFieldStorageConfig provided by migrate_drupal.
  3. The same for EmptySource.
  4. DrupalSqlBase implements DependentPluginInterface and carries the DependencyTrait.
  5. The new getConfiguration() from SourcePluginBase is gone.
alexpott’s picture

Status: Needs review » Needs work
FileSize
16.09 KB

@chx nice work. The fundamental changes look fine to me.

+++ b/core/modules/migrate_drupal/migrate_drupal.module
@@ -29,3 +29,17 @@ function migrate_drupal_help($route_name, RouteMatchInterface $route_match) {
+/**
+ * Implements hook_migrate_destination_info_alter().
+ */
+function migrate_drupal_migrate_destination_info_alter(&$definitions) {
+  $definitions['entity:field_storage_config']['class'] = 'Drupal\migrate_drupal\Plugin\migrate\destination\EntityFieldStorageConfig';
+}
+
+/**
+ * Implements hook_migrate_source_info_alter().
+ */
+function migrate_drupal_migrate_source_info_alter(&$definitions) {
+  $definitions['empty']['class'] = 'Drupal\migrate_drupal\Plugin\migrate\source\EmptySource';
+}

I think we should drop this and just have new plugins and use them in the Drupal 6 migrations.

chx’s picture

Status: Needs work » Needs review
FileSize
97.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
7.48 KB

Introducing md_empty and md_entity:field_storage_config plugins.

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

chx’s picture

FileSize
98.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,745 pass(es), 118 fail(s), and 89 exception(s). View

No real change here, just fixing the test fail with passing in an entity manager mock.

Status: Needs review » Needs work

The last submitted patch, 56: 2460529_56.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
98.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,673 pass(es), 119 fail(s), and 95 exception(s). View

Nice.Entity::getEntityTypeId had substr($plugin_id, 7) hardwired, replaced with str_replace('entity:', '', $plugin_id);

chx’s picture

FileSize
98.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

D'oh that won't work either, of course. Stupid me. Let's just override the method.

alexpott’s picture

FileSize
2.11 KB

Uploading an interdiff of 54 to 59 for those following along.

benjy’s picture

I think it would be good to flesh out the comments on the plugins that we've added to migrate_drupal such as EntityFieldStorageConfig. If I was a newcomer, i'd be quickly wondering why migrate_drupal has gone out of it's way to provide it's own plugin rather than use the one in migrate. Sure I can see the addition of calculateDependencies but it isn't clear why migrate_drupal does that and not core migrate.

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

The last submitted patch, 58: 2460529_58.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 59: 2460529_58.patch, failed testing.

alexpott’s picture

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

And now #22 is solved! Nice.

alexpott’s picture

FileSize
2.61 KB

Uploading missing interdiff.

alexpott’s picture

FileSize
2.82 KB
100.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,562 pass(es). View

Let's improve the documentation as per #61 and also make sure we comment on the difference between configuration dependencies and migration dependencies.

chx’s picture

I added a note on constants/entity_type to https://www.drupal.org/node/2171833 .

benjy’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -498,12 +498,6 @@ protected function findDefaultConfigWithUnmetDependencies(StorageInterface $stor
    -    if (strpos($config_name, 'migrate.migration.') === 0) {
    -      return TRUE;
    -    }
    

    I'm glad about this :)

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/DestinationBase.php
    @@ -115,4 +118,11 @@ public function resetStats() {
    +  public function calculateDependencies() {
    +    return $this->dependencies;
    +  }
    

    Would it be better to not have this so the children classes have to implement it in case anyone extends DestinationBase directly?

Other than my second question I think this is ready.

alexpott’s picture

FileSize
3.71 KB
100.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,526 pass(es). View

#69.2 is a great point. There is no need for DestinationBase to implement the DependencyTrait and interface. The destination plugins that have knowledge of additional dependencies can do this for themselves.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Great, RTBC!

  • catch committed 9f4d5e8 on 8.0.x
    Issue #2460529 by alexpott, chx, phenaproxima: Migrations need to use...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Looked at this yesterday and again today, looks like great clean-up and the move from install to optional is definitely necessary.

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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