Problem/Motivation

While working on #2798363: Implement --idlist options on rollback and messages, I noticed some really wild and crazy things going on in the rollback section of the migrate executable. We loop over the id_map, but then never use the row we are looping over. But as a side "benefit" it advances the rows forward.

Proposed resolution

Let's make the code more extensible and more readable so things like #2798363: Implement --idlist options on rollback and messages can only change a small amount of code, not large hunks of code.

It would be nice, if possible, to backport this to 8.6 as we have a patch (w/ passing tests) in migrate_tools waiting for this feature.

Remaining tasks

  • Add a getter for the id_map so we can filter it.
  • Refactor the foreach to actually use the value in a loop.

User interface changes

n/a

API changes

n/a

Data model changes

n/a

CommentFileSizeAuthor
#2 2999478.patch3.7 KBheddn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Active » Needs review
FileSize
2999478.patch3.7 KB

Based on my reading of http://php.net/manual/en/class.filteriterator.php, I think this small change here will do just the trick.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

I see what you are trying to do. This solves the issue nicely and lets contrib filter the map list. It also matches the rollback iterator loop over the map with the import loop over the source.

I'm setting to NW based on needing IS update, since you changed plans there and didn't implement the steps outlined. Otherwise I'll RTBC

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
Status: Needs review » Reviewed & tested by the community
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed 2973442 and pushed to 8.7.x. Thanks!

  • alexpott committed 2973442 on 8.7.x 
    Issue #2999478 by heddn, mikelutz: Refactor MigrateExecutable::rollback...
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

Any chance on a backport to 8.6?

Status: Fixed » Closed (fixed)

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

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented

> Any chance on a backport to 8.6?

+1 for that. The 8.6 lights in #2 are green too.
I suppose this will only get attention if we open a followup.