Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
If the source ceases to have data , this is never reflected in the destination. There are two problems (at least), this patch attacks the field level in the content entity base, config entities and raw config entities are also broken but probably needs to be different issue and less severe because this only comes up in reruns, the migrate_drupal path does not really support reruns. And then there is the case when a whole row / entity disappears but that is an entirely different issue.
Proposed resolution
Unset outdated data.
Remaining tasks
Test this. Think it through.
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#92 | interdiff-82-91.txt | 1.13 KB | jofitz |
#92 | 2711353-91.patch | 7.92 KB | jofitz |
#82 | interdiff-2711353-80-82.txt | 974 bytes | rakesh.gectcr |
#82 | 2711353-82.patch | 7.91 KB | rakesh.gectcr |
#81 | interdiff-2711353-73-76.txt | 607 bytes | rakesh.gectcr |
Comments
Comment #2
chx CreditAttribution: chx at Integral Vision Ltd commentedComment #4
BerdirQuick feedback/things to think about:
We can't remove all fields, only those that are actually defined in the migration, otherwise this is going to kill partial migrations that import some additional fields in real fun ways (I expect the patch is going to fail badly in some tests).
What if we change getDestination() to return all fields, even if they don't have a value? If $values is NULL, then things will basically just work? This logic would have to change:
If the second condition is removed, will anything break?
Comment #5
chx CreditAttribution: chx at Integral Vision Ltd commentedComment #6
dawehnerI mean I get why its working, but its absolutely not clear what an empty destination value would do.
Comment #7
chx CreditAttribution: chx at Integral Vision Ltd commentedComment #9
chx CreditAttribution: chx at Integral Vision Ltd commentedComment #10
chx CreditAttribution: chx at Integral Vision Ltd commentedComment #11
chx CreditAttribution: chx at Integral Vision Ltd commentedComment #12
mikeryanComment #13
benjy CreditAttribution: benjy at PreviousNext commentedCould setDestinationProperty() not just accept NULL's and then loop over them to get the empty values in the destination?
Comment #14
chx CreditAttribution: chx at Integral Vision Ltd commented> If the second condition is removed, will anything break?
> Could setDestinationProperty() not just accept NULL's and then loop over them to get the empty values in the destination?
*spreads arms* I have no idea and I am not keen on trying to find out. I don't even want to try to think over the possible ramifications of changing how the destination is stored / set. But I will try to think on it for a few days.
Comment #15
vasi CreditAttribution: vasi at Evolving Web commentedSome of my migrations change existing entities, and use NULL destination fields to indicate "I don't want this field to be changed". I think we need to keep at least some way to do that.
We should probably decide that NULL means one of "don't change this field" or "erase this field". And then use a custom class to indicate the other?
Comment #16
vasi CreditAttribution: vasi at Evolving Web commentedComment #17
mikeryan@vasi: The default assumption when migrating is that the source data is the system-of-record - each time an item is (re-)imported, the destination should be an exact replica of the source data, including omitting data that is not on the source. In D7 we had an explicit system-of-record switch, but in D8 we use overwrite_properties - if this is set, the fields listed there are the only ones to be overwritten by migration, any other fields will be left at their existing D8 values (if any).
Comment #18
vasi CreditAttribution: vasi at Evolving Web commentedOk, sorry for the rant then :)
Comment #19
mikeryanTrailing whitespace.
Trailing whitespace.
Setting to needs work for tests...
Comment #21
iMiksu#9 needs a reroll.
Comment #22
iMiksuWe'll work on this today.
Comment #23
biguzis CreditAttribution: biguzis at Wunder commentedComment #24
biguzis CreditAttribution: biguzis at Wunder commentedRerolled.
Interdiff not available because #9 patch can't be applied.
Comment #25
mikeryanThanks for the reroll - now on to the tests!
Comment #26
quietone CreditAttribution: quietone as a volunteer commentedWill have a go at the test.
Comment #27
quietone CreditAttribution: quietone as a volunteer commentedAdding a test for processRow and EntityContentBase. Also added some comments.
Comment #29
mikeryanComment #30
mikeryanIt looks like this patch is replacing an existing test rather than adding a new one?
Comment #31
quietone CreditAttribution: quietone as a volunteer commentedThanks, testEntityWithStringId() is restored to its former glory.
Comment #32
mikeryanLooks good, thanks!
Comment #34
xjmAs an API addition for a beta experimental module, this one can go in during beta. So, moving to 8.3.x and tagging as rc deadline. If it's not in by RC then we would discuss whether to move to 8.4.x. Thanks!
Comment #35
alexpottLet's make the property match the getter/setter - ie.
protected $emptyDestinationProperties
since they are not empty destinations.Comment #36
jofitz CreditAttribution: jofitz at ComputerMinds commentedSet the property to match the getter/setter, as recommended by @alexpott.
Comment #37
jofitz CreditAttribution: jofitz at ComputerMinds commentedScrub that. I managed to screw up the patch. Try this.
Comment #39
mikeryanSince there's a change in behavior, safest to have a change record in case anyone was counting on that behavior.
Comment #40
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #41
xjmSince 8.3.x is now in commit freeze for its release candidate phase and Migrate is in beta, this disruptive change should now be targeted for 8.4.x. Thanks!
Comment #42
xjmComment #43
jofitz CreditAttribution: jofitz at ComputerMinds commentedUnassigning myself. Sorry, I think I bit off more than I can chew by offering to write the Change Record for this issue.
Comment #44
quietone CreditAttribution: quietone as a volunteer commentedI'll have a go at the change record
Comment #45
quietone CreditAttribution: quietone as a volunteer commentedA start at the change record, https://www.drupal.org/node/2858640.
Comment #46
quietone CreditAttribution: quietone as a volunteer commentedMade a follow up to look into the config entities. #2858708: Migrate never unsets existing data for config entities
Comment #47
Pavan B S CreditAttribution: Pavan B S at Valuebound commenteduse short array syntax (new coding standard).
Applying the patch, please review.
Comment #48
phenaproximaSelf-assigning for review.
Comment #49
phenaproximaLooks great. I have only minor requests.
Nit: Should be "an SQL exception".
More nits: this can just be $entity->version. No need for the get() call. But hey, it works, so whatever.
Let's use Prophecy here ($this->prophesize()) for greater readability.
And here.
Let's use assertCount() here.
Comment #50
yogeshmpawarComment #51
yogeshmpawarUpdated patch as per comment #49 & also added interdiff.
Comment #52
phenaproxima::CLASS should be lowercase.
With Prophecy, we don't need to use things like $this->once(). It's a lot more readable:
$prophecy->getPluginDefinition()->willReturn([])
assertCount() doesn't require a separate call to count(). It should just be this:
$this->assertCount(2, $row->getDestination())
Comment #53
yogeshmpawarThanks @phenaproxima for the suggestions. please check the updated patch & interdiff.
Comment #56
jofitz CreditAttribution: jofitz at ComputerMinds commentedSet up the test prophecy (drawing information from the previous method
testProcessRowPipelineException()
).Comment #58
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrect the test errors.
Comment #59
phenaproximaNit: These are supposed to be in expected, actual order (i.e., $value, $row->getDestinationProperty()), but it'll still work. Fixable on commit.
Otherwise, 'tis a fine thing!
Comment #60
phenaproxima...but it'd be nice to add a little sample code to the change record.
Comment #61
phenaproximaAltered the change record to be a little clearer. I think this is ready to go.
Comment #62
quietone CreditAttribution: quietone as a volunteer commentedWhile this is RTBC, it is a small change to fix the assertion, #59. And, I really, really like the arguments in the expected order.
Comment #63
phenaproximaCoolio. Back to RTBC.
Comment #65
joelpittetErrors looked unrelated
Comment #66
alexpottThis is now no longer true as far as I can see... or at least it needs to be moved. I think a bit more commentary as to what is going on.
If $plugins is not set shouldn't we call ->setEmptyDestinationProperty() - I don't know - thoughts?
Comment #67
mikeryanYes, I think this comment needs to be updated.
I don't believe that's necessary - I have fielded questions from various people about why fields they didn't map in an update migration resulted in existing values being cleared before, and explained the whole system-of-record is source thing, so it seems calling setEmptyDestinationProperty() is unnecessary. Don't immediately see why... but, we should add an assertion for this case (that removing a field mapping from the process pipeline causes the field value to be cleared on a reimport).
Comment #68
rakesh.gectcrComment #70
quietone CreditAttribution: quietone as a volunteer commentedLook like rakesh.gectcr hasn't had time to work on this. On the migrate call, I decided to take this on.
Comment #71
quietone CreditAttribution: quietone as a volunteer commentedReview this with rakesh.gectcr and we agree that there are 2 things left to do.
1) Update the comments (see #66.1 and #67.1)
2) Update the CR
Comment #72
rakesh.gectcrComment #73
rakesh.gectcrIMHO, The CR is fine, I have read and changed the comment block according to that.
Comment #75
maxocub CreditAttribution: maxocub for Acquia commentedAdding a tag so we can see this issue on our Vienna migrate sprint kanban.
Comment #76
rakesh.gectcrComment #77
rakesh.gectcrComment #78
jofitz CreditAttribution: jofitz at ComputerMinds commentedPatch addresses @alexpott's first concern from #66 (@mikeryan addressed his second concern in #67) so this can (finally) be set back to RTBC.
Comment #79
phenaproximaSorta-nit: We don't need to pass a new MigrateMessage() in anymore -- MigrateExecutable will create it on its own.
Not enough to derail RTBC though :)
Comment #80
jofitz CreditAttribution: jofitz at ComputerMinds commentedLet's tidy that up (for completeness).
Comment #81
rakesh.gectcrCool, @phenaproxima agreed..... :)
Here we go with a little cleaner. :D
Putting back to needs review for running the patch.
Comment #82
rakesh.gectcrWe need to remove the
new MigrateMessage()
one more place@JO You are super fast! Cheers!
Comment #83
phenaproximaThanks for humoring me, guys :) Back to RTBC on the assumption that Drupal CI will pass it.
Comment #86
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!
Comment #89
catch8.4.x wasn't happy so I've reverted from both branches for now - might need an 8.4 backport.
Comment #90
jofitz CreditAttribution: jofitz at ComputerMinds commentedI'll have a go at an 8.4 backport.
Comment #91
jofitz CreditAttribution: jofitz at ComputerMinds commentedI was expecting to do a re-roll for 8.4, but the patch applied perfectly. Have set the testbot running the patch from #82 against 8.4.
Comment #92
jofitz CreditAttribution: jofitz at ComputerMinds commentedHere is the patch for 8.4.x (which will also work for 8.5.x just with a redundant parameter passed to MigrateExecutable).
In 8.5.x the second parameter of MigrateExecutable(), $message, is now optional, but it was required in 8.4.x.
Comment #93
heddnBack to RTBC.
Comment #95
jofitz CreditAttribution: jofitz at ComputerMinds commentedIt is no longer possible to have one single patch that applies to both 8.4.x and 8.5.x. The patch in #92 applies to 8.4.x, the patch in #82 applies to 8.5.x (I am re-running the tests on this patch to be sure). I think it is OK to return this to RTBC.
Comment #96
catchCommitted/pushed the separate patches to both branches, thanks for the backport.
Comment #100
quietone CreditAttribution: quietone as a volunteer commentedPublished change record.