Follow-up to #2558047: [meta] Dealing with conditions during migration (messaging/exceptions) is confusing and inconsistent

Problem/Motivation

Right now we have the confusing situation where a prepareRow() implementation wanting to save any per-row messages needs to call queueMessage(), whereas in process or destination plugins you would use queueMessage(). This is a historical WTF from Migrate 2 - we want to clear any pre-existing messages for each row before adding any for the current run, but the clear was done after prepareRow() is called, so it couldn't safely add messages directly without immediately losing them. The queueMessage() API was added to allow this - messages generated by prepareRow() were queued, then added to the map after the clear. Let's clean this up - clear the messages before prepareRow(), and dump the whole queue business. Ideally, we can come up with a solution that does not require the source plugin to have direct knowledge of the migration or executable classes, and make one small step towards #2543552: Modernize migration source plugins.

Proposed resolution

TBD

Remaining tasks

TBD

User interface changes

N/A

API changes

Removal of queueMessage().

CommentFileSizeAuthor
#15 manage_per_row_messages-2567571-15.patch796 bytesmikeryan
#7 manage_per_row_messages-2567571-7.patch8.33 KBmikeryan
#3 manage_per_row_messages-2567571-3.patch8.27 KBmikeryan

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented

Ooh, this will let us finally close #2443617: Find a way for SourcePluginBase to get MigrateExecutable...

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Active » Needs review
StatusFileSize
new manage_per_row_messages-2567571-3.patch8.27 KB

Simply moving the message clear to next() before calling prepareRow() (from prepareRow() after the prepare_row hooks) enables us to dump a bunch of unnecessary crap, in particular decoupling the source plugin from the executable. One outstanding issue - I had to remove a unit test case checking that the clear (ID map delete) is called, from testImportWithValidRowNoDestinationValues(), because the mocked source plugin mocks rewind() preventing that code path from executing. Need to figure out a better way to test that...

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Issue tags: +Needs change record

Status: Needs review » Needs work

The last submitted patch, 3: manage_per_row_messages-2567571-3.patch, failed testing.

The last submitted patch, 3: manage_per_row_messages-2567571-3.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs work » Needs review
StatusFileSize
new manage_per_row_messages-2567571-7.patch8.33 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

@mikeryan explained on IRC that this is a DX improvement. Saving messages is now going to be done using $migration->getIdMap()->saveMessage(), which is still a bit WTFey, but at least it's consistent and more sane than queueMessage() and saveQueuedMessages(). Plus, we get to lose a bunch of LOC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Nice diffstat. Seems like it removes a lot of unneeded complexity, rock.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 2ee4a7f on 8.0.x 
    Issue #2567571 by mikeryan: Manage per-row messages more rationally
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Issue tags: -Needs change record

Change record draft added.

benjy’s picture

benjy commented

I like the amount of red in this patch :) just one question

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -199,11 +191,8 @@ public function prepareRow(Row $row) {
+        $this->migration->getIdMap()->saveIdMapping($row, array(), MigrateIdMapInterface::STATUS_IGNORED);

@@ -307,6 +296,12 @@ public function next() {
+        $this->idMap->delete($this->currentSourceIds, TRUE);

How come in the same class we have a $this->idMap and a $this->migration->getIdMap()?

They reference the same thing?

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented

Yep, prepareRow() should be using $this->idMap... Is it worth a follow-up patch for that?

benjy’s picture

benjy commented

Yeah, may as well have it right. Should be easy enough and consistency is always nice.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Fixed » Needs review
Issue tags: +Quick fix
StatusFileSize
new manage_per_row_messages-2567571-15.patch796 bytes

OK, here it is.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

*claps hands with glee*

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: manage_per_row_messages-2567571-15.patch, failed testing.

Status: Needs work » Needs review

mikeryan queued 15: manage_per_row_messages-2567571-15.patch for re-testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Reviewed & tested by the community

Restoring RTBC after testbot WTF.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 6f8ba53 on 8.0.x 
    Issue #2567571 follow-up by mikeryan, benjy: Small simplification to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture

quietone commented

Published change record.