Problem/Motivation

Whenever a migrate source plugin does not check (or does not returns with) the return value of its parent's prepareRow() method, it causes that it is impossible to skip rows by returning FALSE in hook_migrate_prepare_row() or in hook_migrate_MIGRATION_ID_prepare_row()implementations.

See SourcePluginBase::prepareRow() (link to git.drupalcode.org).

Remaining tasks

  • Review
  • Commit
  • Smile

Tasks to be done

  1. #3187426: Ignored parent::prepareRow() return value in TermTranslation and in TermLocalizedTranslation

    2.

Finished tasks

  • Modified MigrateSourceTestBase.php so that the return value of prepareRow is tested for all source plugins.
  • That test found all the source plugins with this problem
  • Fix all the source plugins
CommentFileSizeAuthor
#8 3187433-8.patch9.26 KBquietone
#8 interdiff-6-8.txt2.26 KBquietone
#6 3187433-6.patch7.71 KBalexpott
#6 3187433-6-test-only.patch2.96 KBalexpott
#3 3187433-3.patch4.71 KBPooja Ganjage

Comments

huzooka created an issue. See original summary.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Issue summary: View changes
Pooja Ganjage’s picture

Pooja Ganjage commented
StatusFileSize
new 3187433-3.patch4.71 KB

Hi,

Creating a patch for this issue.

Please review the patch.

Let me know if any suggestions.

Thanks.

Pooja Ganjage’s picture

Pooja Ganjage commented
Status: Active » Needs review
quietone’s picture

quietone commented
Related issues: +#2952291: Dispatch the PREPARE_ROW event. Deprecate prepare_row_alter hooks

Adding related issues that deprecates the hook_prepare_row

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 3187433-6-test-only.patch2.96 KB
new 3187433-6.patch7.71 KB

Let's see if we can add generic testing of this.

Status: Needs review » Needs work

The last submitted patch, 6: 3187433-6.patch, failed testing. View results

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-6-8.txt2.26 KB
new 3187433-8.patch9.26 KB

@alexpott, thank you. I just started to think about a test, which catch asked for in 3187426 #10/a> and came to read this issue see what was here and voila, a test! You have been fixing quite a few of my mistakes lately. Thanks.

I reviewed the patch and modified all the source plugins that require changing to have 'return parent::prepareRow($row);' if it is the last statement in prepareRow or 

    if (!parent::prepareRow($row)) {
      return FALSE;
    }

if the call to the parent is at the beginning of prepareRow. This case is common in the i18n translation source plugins.

So, lets see if this breaks anything.

quietone’s picture

quietone commented
Title: [META] Eliminate parent::prepareRow() calls with ignored return value from all migrate source plugins » Eliminate parent::prepareRow() calls with ignored return value from all migrate source plugins
Issue summary: View changes

Closed #3187426: Ignored parent::prepareRow() return value in TermTranslation and in TermLocalizedTranslation. The source plugins altered there are included here.

That means that the test and all the fixes are in one patch and having this as a meta doesn't make sense. Removing the Meta from the title.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

The added testing tests that every migrate source plugin returns false when a migrate row should be skiped. All instances where migrate source plugins where not doing that are fixed.
The patch adds documentation where it is needed.
All code changes look good to me.
For me it is RTBC.

  • catch committed 8a57d1e on 9.2.x 
    Issue #3187433 by alexpott, quietone, Pooja Ganjage, huzooka, daffie:...

  • catch committed 50ea22d on 9.1.x 
    Issue #3187433 by alexpott, quietone, Pooja Ganjage, huzooka, daffie:...
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

Status: Fixed » Closed (fixed)

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