It would be nice if migrations could support some migrations throwing exceptions in prepareRow() and prepare(). I would love if I could throw a MigrateSkipRecordException() which would cause a message to be logged, and the current item skipped in the migration. I find doing this to be very difficult with the current code. I cannot get any combination of saveMessage() or queueMessage() to work.
Related:
#1421974: Allow messages to be saved in prepareRow()
#1982564: Migration::saveQueuedMessages() is not called for records that are skipped via prepareRow()
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | exception_status-1990612-7.patch | 2.15 KB | mikeryan |
Comments
Comment #1
dave reidComment #2
mikeryanWouldn't fixing #1982564: Migration::saveQueuedMessages() is not called for records that are skipped via prepareRow() address this? In the next version of the API throwing an exception should be the mechanism instead of passing the FALSE flag up through the prepareRow() chain, but for now I don't think it's good DX to introduce a 2nd mechanism to achieve the same thing.
Comment #3
mikeryanComment #4
dave reidAs someone who was new to Migrate at one point, and who now has more of an understanding, I always find remembering how to fail, skip, or allow certain rows to continue. In my perfect world, here's the three conditions that I would expect to encounter and how I would expect to handle them:
Condition 1: Log some kind of message/warning/error but still migrate this row
Ideal: Use $this->saveMessage() regardless if I'm in prepareRow() or prepare().
Actual: Use $this->queueMessage() if in prepareRow(), but $this->saveMessage() in prepare().
Condition 2: Log some kind of message/warning/error and skip this single row and proceed with the rest
Ideal: Use throw new MigrateSkipRowException(t('My error message')) to log a message and skip the current row.
Actual: Use $this->queueMessage() and return FALSE if in prepareRow(), but $this->saveMessage() in prepare(). To skip the row, use return FALSE if in prepareRow(), but ???? in prepare().
I don't know how to skip a record in prepare(). Maybe it's not even possible??
Condition 3: Fail the current migration and stop processing the rest of the entries
Ideal: Use throw new MigrationAbortException(t('My error message')).
Actual: Use throw new MigrateException(t('My error message'));
Summary
Basically in my ideal world I want to use exceptions when it means I should stop running the current row. Having the exception names make it clear what will happen when I throw them helps make them self-descriptive to new developers.
Please let me know if I'm incorrect in my 'Actual' assumptions above. I would love to document these assumptions in a handbook page about errors and skipping records either on Commonly implemented Migration methods or under Advanced topics.
Comment #5
dave reidHopefully this clears things up what I was trying to express a little bit more.
Comment #6
mikeryanYour ideal scenarios are all right on the mark, and are the way things should work in Migrate V3. Right now, the Migrate V2 API has accumulated some technical debt, and for many things "fixing" the WTFs runs the risk of breaking code that expects things to work the way they do now, or at least confusing the issue by introducing alternate means to do what's done now - I'm reluctant to do any significant refactoring at this stage of maturity of the Migrate V2 API.
Note that the model is that the decision whether or not to process a row is baked into the source plugin - for each row it reads from the source data, it decides to skip rows on its own (based, for example, on whether they've already been migrated), then for anything it's not already skipping asks the migration class for its opinion via prepareRow(). Once it's got the OK there, the assumption is that we're committed to processing that row. Can you describe the use case where the decision can't be made until prepare() time?
Now, actually, throwing an exception in prepare() will cause the current record to be skipped, although with a hard-coded STATUS_FAILED. Adding a status to MigrateException would allow you to throw an IGNORE there, that could easily be added without disrupting anything.
A process can be stopped with $migration->stopProcess(). If this were done in the middle of processing an item (e.g., from prepareRow() or prepare()) that item would be done before the process stopped, however, so it's not like throwing an exception. I'm curious, what's the use case for aborting an entire migration from within the migration itself?
Thanks.
Comment #7
mikeryanTry this - I've added a new third parameter to MigrateException, by passing MigrateMap::STATUS_IGNORED when you trhow it you should be able to do what you want. Untested apart from passing simpletests...
Comment #8
mikeryanSilence implies consent;). Committed.
Comment #9.0
(not verified) commentedAdding related issues