The 'needs_update' field in the 'migrate_map_%' tables is named incorrectly and being used for competing data.

Code from includes/map.inc:

/**
 * Codes reflecting the current status of a map row.
 */
const STATUS_IMPORTED = 0;
const STATUS_NEEDS_UPDATE = 1;
const STATUS_IGNORED = 2;
const STATUS_FAILED = 3;

Three statuses answer the question "What happened to this row during the most recent update?" One answers "Should this row be updated no matter what the next time the migration is run?" What happens when you need to know which rows failed after you set the row to be updated again? You can't.

The (a?) solution to this issue is to create an 'import_status' row and use the original 'needs_update' field as a flag (as it seemed to be named/designed for.) Thoughts?

Attached patch separates needs_update flag data from row import status, includes hook_update_7208() to adjust existing installs. The syntax of the MigrateMap::saveIDMapping() function has been changed to include separate import_stats and needs_update values. Variable defaults now come from the database defaults rather than function and database defaults to better match Drupal standards: If a passed value is NULL, it will not be updated in the database. No more losing your $hash value to set a $rollback_action.

I've checked the following contrib modules: commerce_migrate, g2migrate, migrate_d2d, migrate_extras, phpbb2drupal, TYPO3_migrate, and wordpress_migrate. Only the current dev of migrate_d2d uses the saveIDMapping() function, so a small patch is required for it.

The above code from includes/map.inc changes to:

  /**
   * Map row last import status: Import sucessful.
   */
  const STATUS_IMPORTED = 0;
  /**
   * DEPRECATED Map row last import status: Needs Update.
   * @todo Remove this value in next version: 2.7 or 3.0.
   */
  const STATUS_NEEDS_UPDATE = 1;
  /**
   * Map row last import status: Import sucessful. Ignored during last import.
   */
  const STATUS_IGNORED = 2;
  /**
   * Map row last import status: Failed during last import.
   */
  const STATUS_FAILED = 3;
  /**
   * Map row last import status: Not imported. The row default.
   */
  const STATUS_NOT_IMPORTED = 4;

  /**
   * Map row needs update: False, update not needed.
   */
  const NEEDS_UPDATE_FALSE = 0;
  /**
   * Map row needs update: True, update needed.
   */
  const NEEDS_UPDATE_TRUE = 1;
CommentFileSizeAuthor
#2 migrate-add-import-status-2249191-2.patch17.09 KB13rac1
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

13rac1’s picture

Issue summary: View changes
13rac1’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
17.09 KB

Added patch.

13rac1’s picture

Issue summary: View changes

Issue summary formatting.

mikeryan’s picture

Category: Bug report » Task
Status: Needs review » Postponed (maintainer needs more info)
Related issues: +#1355940: Track ignored rows

So, a little history... Originally, only successfully-imported rows were saved in the map table, and needs_update served as a flag to indicate any rows that should be reimported. However, the concept of ignoring rows by returning FALSE from prepareRow() was introduced, and it was also felt that it would be much more convenient to track the state of all rows that we attempted to import in the map table: #1355940: Track ignored rows. So, the needs_update column became a status column, with new potential values. Renaming the column was considered at the time, but it's so common to explicitly query the map columns (not just in migration code, but in audit queries, views, etc.) that it was felt that the DX improvement of properly naming the column wasn't worth the breakage it would cause. I'm still unenthusiastic about doing this.

And, changing the saveIDMapping() API just won't fly. Contrib is the tip of the iceberg - Migrate is a framework used to implement migration modules, and there's no way of knowing how many custom migration modules people have implemented for their particular scenarios, or how many of them use saveIDMapping(). And, frankly, I'm not seeing a significant functional advantage to separating out needs_update to be distinct from the status - certainly not enough to make an API break.

Sorry, I appreciate that you've put a lot of time into this, but I can't see accepting these changes into Migrate. Setting to "Postponed (maintainer needs more info)" rather than "Closed (won't fix)" should you want to try to make your case...

13rac1’s picture

Since there'll probably never been a 7.x-3.x of Migrate, can we fix this in D8? http://drupalcode.org/sandbox/chx/2105305.git/blob/refs/heads/8.x:/core/... The same argument applies.

mikeryan’s picture

Status: Postponed (maintainer needs more info) » Closed (won't fix)

Not happening on D7 - D8 names this column source_row_status.