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.
Problem/Motivation
#2340401: Fill commented entity during stub comment saving shows that non-ID yet required fields break the migration process plugin.
Proposed resolution
- Move no_stub to the destination plugin. It's not a per-usage thing; there are things which simply can't be stubbed in a sensible way. Filter formats is an example. This is Win(TM) 1.
- Stub process needs to equal the complete process instead of just IDs.
$process = $migration->get('process')
- If this breaks some processing then we need to provide sensible defaults. This is doable since the source configuration itself is the default even if it's mostly just used for constants. It can provide a default for any key. Note how we didn't need to resort to this.
- We need to add the defaults to the stub row
$stub_row = new Row($values + $migration->get('source'), $source_ids);
. The availability of constants for stub rows is Win(TM) 2. This is something the parent issue was unable to resolve cleanly. - Finally the
Entity
destination should be changed to use only the ids achieving the same result as before patch. However, since the constants now are available inside the destination, comment can override the new method for this and use entity_type as well.
Remaining tasks
Make the patch pass :) Make exception tidier perhaps?
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#32 | interdiff.txt | 859 bytes | benjy |
#32 | 2411233-31.patch | 13.34 KB | benjy |
#31 | interdiff.txt | 1.47 KB | benjy |
#31 | 2411233-30.patch | 14.18 KB | benjy |
#28 | interdiff.txt | 1.03 KB | benjy |
Comments
Comment #1
chx CreditAttribution: chx commentedComment #3
benjy CreditAttribution: benjy commentedNew patch fixes the fatal. The MigrationTest really isn't testing much to be honest. It was previously using the no_stub to skip most of the code in transform() and claiming to be a test for no_stub.
I vote for just deleting this test entirely? We have plenty of integration tests using the migrate process plugin.
Comment #5
benjy CreditAttribution: benjy commentedAdded schema.
Comment #7
chx CreditAttribution: chx commentedCrappy tests can be deleted yes.
Comment #8
benjy CreditAttribution: benjy commentedThe latest fails are because "plugin" is in the source but not in the values. That means we can remove it from the source or add it to the values. I went with the latter.
Comment #9
benjy CreditAttribution: benjy commentedRemoved the test.
Comment #12
chx CreditAttribution: chx commentedBah, you shouldn't listen to me :) here's the relevant code from
Source
therefore the source configuration needs to be added to the values not the ids :/
Comment #13
chx CreditAttribution: chx commentedComment #15
chx CreditAttribution: chx commentedat first look, this means that filters needs a default array in the source (easy) but at second look, is this trying to stub a format?? Didn't we (try to) ban that?
Comment #16
andypost@chx maybe add a parent migration to import filter and pass stubbing?
Comment #17
chx CreditAttribution: chx commentedd6_comment and d6_custom_block both depend on d6_filter_format already
Comment #18
benjy CreditAttribution: benjy commentedSome fixes.
Comment #20
chx CreditAttribution: chx commentedSee BlockPluginId:
->transform($delta
where $delta is boxes.bid and not blocks.bid. The test confused blocks.bid for boxes.bid. Also, custom blocks can't be stubbed since there's a unique index containing info.As for comments, as the issue summary says, the source for comments.created (timestamp) needs a default. I went with 0.
I am leaving file for others :)
Comment #22
chx CreditAttribution: chx commentedComment #24
chx CreditAttribution: chx commentedHrm, file is not failing? Let's try this; the latest fail is trivial to fix.
Comment #26
benjy CreditAttribution: benjy commentedNew patch attached.
Comment #27
benjy CreditAttribution: benjy commentedPosted the last patch a little early, new patch coming soon.
Comment #28
benjy CreditAttribution: benjy commentedOK, try this.
Comment #31
benjy CreditAttribution: benjy commentedOK, this should be closer.
Comment #32
benjy CreditAttribution: benjy commentedAccidentally left some changes in the Comment entity.
Comment #34
chx CreditAttribution: chx commentedComment #35
hosef CreditAttribution: hosef commentedThis looks good. It works for me, and all tests pass.
Comment #36
chx CreditAttribution: chx commentedComment #37
alexpottmigration changes are not blocked in beta. Committed 9eb9661 and pushed to 8.0.x. Thanks!