Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#70 | 2460529.70.patch | 100.29 KB | alexpott |
#70 | 67-70-interdiff.txt | 3.71 KB | alexpott |
#67 | 2460529.67.patch | 100.78 KB | alexpott |
#67 | 65-67-interdiff.txt | 2.82 KB | alexpott |
#66 | 59-65-interdiff.txt | 2.61 KB | alexpott |
Comments
Comment #2
xjmComment #3
alexpottYep 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 providedconfig_name
.Comment #4
alexpottTurns 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:
d6_aggregator_settings
would not be created because it depends on the aggregator module.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.
Comment #5
alexpottThis 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.
Comment #6
alexpottYou 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.Comment #8
alexpottSo 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).
Comment #9
mikeryanThere 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.
Comment #10
alexpottThis 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.
Comment #12
alexpottSo I think I've got most of the relevant dependencies added...
Comment #14
alexpottOh look at that... config dependencies making things more robust and better tested :)
Comment #15
mikeryanClose - 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.
Comment #16
mikeryanComment #17
mikeryanI 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.
Comment #18
alexpottSo 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.
Comment #19
xjmSo I thought of a couple of usecases that are worth considering/explaining (contrary to my original issue title yesterday):
Comment #20
alexpott@xjm at the moment this patch is ensuring the the migration has dependencies on:
So to answer your points in order:
Comment #21
mikeryanThose 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.
Comment #22
alexpottSo if we are to proceed with this:
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.Comment #23
benjy CreditAttribution: benjy at CodeDrop commentedI think those 4 migrations depending on migrate_drupal seems fine.
Here's the issue for migration groups: #2302307: Support shared configuration between migrate groups
Comment #24
alexpottConsidering 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.
Comment #27
phenaproximaPatch needs a re-roll. Self-assigning.
Comment #28
phenaproxima@alexpott said on IRC that he'd rather do the reroll, so...here you go!
Comment #29
alexpottTalked to @phenaproxima in IRC since this involved the ConfigInstaller - I was already working on the reroll.
Comment #31
alexpottFixing the test
Comment #32
alexpottA couple of fixes.
Comment #34
alexpottMore failyay!
Comment #36
phenaproximaA few more...
Comment #37
phenaproximaThe 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.
Comment #38
phenaproximaActually 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. :)
Comment #39
chx CreditAttribution: chx commented+ return $this->migration_dependencies + array('required' => [], 'optional' => []);
Short array style is encouraged but mixed style is frowned upon.
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?
Comment #40
alexpott@chx thanks for the review.
explode('.', $this->config->getName(), 2)[0]
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.
Comment #41
chx CreditAttribution: chx commentedRegarding 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.
Comment #42
alexpottConversation with @chx in IRC
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:
d6_user_contact_settings
get a dependency on the contact module?d6_node_setting_status
get a dependency on the node module? And all of the entity type providers used by:Comment #43
chx CreditAttribution: chx commentedMost 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
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.
Comment #44
chx CreditAttribution: chx commentedSome 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.
Comment #45
alexpottWell 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.
Comment #47
benjy CreditAttribution: benjy at CodeDrop commentedCan we inject this?
This is an interesting pattern, i'd have more expected that we'd override hasTrustedData() ?
Needs injecting
Comment #48
phenaproximaCan 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.
Comment #49
phenaproximaWhoops. Fixing status.
Comment #50
alexpottThanks @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.
Comment #52
chx CreditAttribution: chx commentedI am having trouble creating an interdiff. Here's what I've done:
Comment #53
alexpott@chx nice work. The fundamental changes look fine to me.
I think we should drop this and just have new plugins and use them in the Drupal 6 migrations.
Comment #54
chx CreditAttribution: chx commentedIntroducing md_empty and md_entity:field_storage_config plugins.
Comment #56
chx CreditAttribution: chx commentedNo real change here, just fixing the test fail with passing in an entity manager mock.
Comment #58
chx CreditAttribution: chx commentedNice.Entity::getEntityTypeId had substr($plugin_id, 7) hardwired, replaced with str_replace('entity:', '', $plugin_id);
Comment #59
chx CreditAttribution: chx commentedD'oh that won't work either, of course. Stupid me. Let's just override the method.
Comment #60
alexpottUploading an interdiff of 54 to 59 for those following along.
Comment #61
benjy CreditAttribution: benjy at CodeDrop commentedI 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.
Comment #65
alexpottAnd now #22 is solved! Nice.
Comment #66
alexpottUploading missing interdiff.
Comment #67
alexpottLet's improve the documentation as per #61 and also make sure we comment on the difference between configuration dependencies and migration dependencies.
Comment #68
chx CreditAttribution: chx commentedI added a note on constants/entity_type to https://www.drupal.org/node/2171833 .
Comment #69
benjy CreditAttribution: benjy at CodeDrop commentedI'm glad about this :)
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.
Comment #70
alexpott#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.
Comment #71
benjy CreditAttribution: benjy at CodeDrop commentedGreat, RTBC!
Comment #73
catchLooked 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!