When loading multiple migrations together, they should be returned in dependency order. For example, with the migrate_example submodule of migrate_plus, the node migration depends on the term and user migrations, and in turn the comment migration depends on the node migration. Enabling only the necessary modules for migrate_example, drush migrate_status loads the migrations and gets them in alphabetical order by ID:
Group: beer Total Imported Unprocessed
beer_comment 5 0 5
beer_node 3 0 3
beer_term 3 0 3
beer_user 4 0 4
If we then do nothing but enable migrate_drupal in addition, we get the desired ordering:
Group: beer Total Imported Unprocessed
beer_term 3 0 3
beer_user 4 0 4
beer_node 3 0 3
beer_comment 5 0 5
This is because migrate_drupal provides a load plugin that does the ordering. Migrate itself should handle the ordering.
Note that the dependency handling in migrate_drupal's load plugin is intertwined with #2507607: [META] Replace Cthulhu-forsaken load plugins with migration builders.
The straightforward thing to do seems to be decoupling the dependency ordering from the dynamic loading in migrate_drupal and putting it into migrate itself. However, I've started working on the UI for migrate (in the migrate_tools submodule of migrate_plus) and find that even with migrate_drupal enabled, migrations displayed using ConfigEntityListBuilder come out ordered by label. I tracked down this behavior to ConfigEntityBase::sort(), which sorts first by weight and then by label. So, for this use case to work, I believe we'll need to either set weights based on the dependency order, or override sort() to take dependencies into account - hopefully this would address the direct entity_load_multiple() case as well without messing with the load plugin business.
Try the weight/sort() approach and see if it works.
N/A
None
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | migration_dependency-2513028-11.patch | 7.18 KB | mikeryan |
| #4 | interdiff.txt | 1.11 KB | mikeryan |
| #4 | migration_dependency-2513028-4.patch | 6.46 KB | mikeryan |
| #1 | migration_dependency-2513028-1-test-only.patch | 3.41 KB | mikeryan |
Comments
Comment #1
mikeryanWith a test-only patch to demonstrate the problem.
Note that I did not touch migrate_drupal's sorting - it still needs (until #2507607: [META] Replace Cthulhu-forsaken load plugins with migration builders) its own loadMultiple() to deal with dynamic migrations.
I suppose dependsOn() could cache its results for performance, but I think that would be premature optimization. Most dependency trees will not be very wide or deep - the worst-case scenario is actually the Drupal upgrade process, so let's wait and see if there is a performance issue there.
Comment #4
mikeryanWe need to account for specified dependencies that don't actually exist (particularly since many migrate_drupal tests don't install dependent migrations). There's still something funky in the upload test, but will pick that up later...
Comment #6
mikeryanI've fixed the storage test, simple enough. The upload tests are frustrating - not least because, while MigrateUploadTest fails both individually and as part of MigrateDrupal6Test in the testbot, locally it only fails in the MigrateDrupal6Test case. I have found one curious thing here, relating to the work done in #2417549: Drupal\migrate_drupal\Tests\d6\MigrateFileTest fail in MigrateTestBase. That introduced a migrateDumpAlter() pseudo-hook, only implemented by the file migration, to rewrite one file row to a random temporary filename. This hook gets invoked in MigrateDrupal6Test, but apparently not by MigrateUploadTest.
Digging deeper, maybe that's a red herring, but wanted to capture that thought...
Comment #7
mikeryanI'm 90-95% sure the actual change is good, and it's simply exposing an existing problem in the upload migration (or at least its tests). Meanwhile, it looks like #2514168: Streamline migrate_drupal integration tests may be exposing the same thing (or something related), so let's focus on getting that fixed, then maybe that will clean up this issue as well.
Comment #10
mikeryanThis is a significant impediment to implementing non-migrate_drupal migrations.
Comment #11
mikeryanAt least part of the remaining problem seems to be triggered by the CCK load plugin - let's get #2507607: [META] Replace Cthulhu-forsaken load plugins with migration builders in and the fix will be simpler.
Comment #12
mikeryanLooking more deeply, what I'm seeing at the moment is MigrateUploadTest passing on its own, but failing within MigrateDrupal6Test. The full migration test skips the individual tests' setUp(), and d6_upload needs that stuff done. The thing that's perplexing me is how this manages to pass normally, and why sorting the migrations has any effect on it...
It's been asked before, and I'm coming on board - do we really need the full migration test? It's extremely hackish and fragile, and I don't know of an instance where it has revealed a real bug - it only seems useful for finding bugs in itself.
Comment #13
mikeryanOy... The problem with the upload test in the full migration scenario is that it lacks a dependency on d6_upload_field_instance, previously d6_upload_field and d6_upload_field_instance just happened to run first so everything was fine, but the dependency sorting results in d6_upload running first and dying. So, just add the missing dependency and we're good to go? Noooo.... It then died because d6_user_picture_field is in the dependency chain but is not getting created from the template, because it requires the image module. But, it's in the dependency chain via d6_node->d6_user, not via the new dependency, so why didn't it die before? And just adding the image module to the upload test gives complaints about a missing d6_field_instance, so why is that not getting created from the template in the first place?
Comment #14
mikeryanComment #15
mikeryanBlocked by #2543768: Missing dependencies in d6_upload, but tests still fail with that patch applied, continuing to investigate...
Comment #16
mikeryanAt chx's suggestion, I'm looking at adapating ConfigDependencyManager's Graph-based sorting here. That probably won't address the test failures, but should be more efficient.
Comment #17
mikeryanI'm feeling now like we need to get #2530030: Create the migrate builder plugin type (or the full #2507607: [META] Replace Cthulhu-forsaken load plugins with migration builders) in to deal with this - there is some existing Graph-based dependency sorting going on now in MigrationStorage::buildDependencyMigration(), but that's all mixed up with the load plugin stuff that we're trying to get rid of. Once we remove that business, we can have a more straight-forward implementation of graph-based sorting here.
Comment #18
phenaproximaUnblocked!
Comment #19
mikeryanThe load plugin removal means that loadMultiple() now does the correct sorting whether or not migrate_drupal is enabled. Meanwhile, looking at the web UI side in migrate_tools, I see ConfigEntityListBuilder::load() does the loadMultiple() (where we sort by dependencies) then calls the entity class sort() which by default sorts by weight and label, breaking our desired sort. In the patches above I was trying to implement an entity class sort() which would sort by dependencies, but given we're starting out with the right sort, the best solution is for that UI to override ConfigEntityListBuilder::load() to simply return the loaded entities that are already in the right order. No need to change anything in core...