Problem/Motivation
The useful-but-intimidating migration process plugin gets overzealous when it completely fails to look up a destination entity -- it throws MigrateSkipRowException. This is a) outside the bounds of what it should be allowed to do, and b) wholly unnecessary, because the skip_on_empty plugin can be used to implement that behavior explicitly on a case-by-case basis.
Proposed Resolution
The migration plugin should never throw MigrateSkipRowException. If it fails the lookup, it's good enough for it to implicitly return NULL.
Remaining Tasks
The usual: patch, review, commit.
API Changes
When the migration plugin fails a lookup, the appropriate course of action will have to be declared explicitly by the migration. (Skip row, skip processing, do further processing, ...?)
Comment | File | Size | Author |
---|---|---|---|
#7 | 2560671-7.patch | 5.91 KB | phenaproxima |
#4 | interdiff-2560671-2-4.txt | 4.44 KB | phenaproxima |
#4 | 2560671-4.patch | 5.57 KB | phenaproxima |
#2 | 2560671-2.patch | 512 bytes | phenaproxima |
Comments
Comment #2
phenaproximaInitial patch.
Comment #4
phenaproximaFixing the test failures and deleting the now-defunct unit test of the Migration plugin.
Comment #5
neclimdulIf we're removing the skip exception, we should remove the associated use from the top of the file.
Its sad this was the only thing this was testing... I asked phena to make a follow up to add it back.
Comment #6
phenaproximaFollow-up issue created.
Comment #7
phenaproximaFixed #5.1.
Comment #8
mikeryanCode looks good to me, RTBC assuming tests pass.
Comment #10
webchickI asked Adam why we want to do this, and he laid out the following scenario:
Makes sense.
I also asked about the test coverage removal, but that is because the only thing the tests were proving is that the behaviour we're removing works, so... yeah. :) We still do need more robust testing for this behaviour in general—for example, the exact scenario laid out above—but since we don't currently have such test coverage it doesn't really make sense to hold this significant UX improvement up on that. Let's make sure there's a follow-up for it though, if one doesn't already exist.
Committed and pushed to 8.0.x. Thanks!