Refactor prepareRow() to add events in addition to hook

SourcePluginBase::prepareRow() should invoke $this->migration->prepareRow() instead of hooks.

That means to use this in the migration you need to hook_entity_info_alter() and extend the migration entity? I don't think we want that.

> if you were to update a typical D7 migration module to D8 converting field mappings etc. to configuration, almost all of the remaining necessary PHP code would be prepareRow() implementations.

No way! It would be in process plugins. prepare row is not supposed to do much. It is supposed to help creating a "canonical" representation of the source object whatever it is. Once that's done, it's game over. I hope that hook will be rarely used.

I'm not sure I agree with removing the hook unless we have something nearly as easy without having to replace the entire source.

Also, I don't see the benefit of the exception instead of a return value when it is caught again in the same class or the parent? It would have made more sense if the exception propagated to the executable. Would that solve our issue with the source currently needing the executable for the saveMapping on skipped rows?

> Would that solve our issue with the source currently needing the executable for the saveMapping on skipped rows?

Just to make sure we see clearly,

      $this->migrateExecutable->saveQueuedMessages();
      $id_map->saveIdMapping($row, array(), MigrateIdMapInterface::STATUS_IGNORED, $this->migrateExecutable->rollbackAction);

this is all we use the executable in the source base ; id_map is from the entity itself. But this is material for #2443617: Find a way for SourcePluginBase to get MigrateExecutable

I don't see much benefit in having an exception vs a return boolean if it just to be caught in a parent implementation. However, I don't think it's too late to add an event if we think that's the best approach, it would make more sense to catch an exception that was thrown by an event listener.

but I didn't get to changing the existing prepareRow() implementations into event listeners.

I don't think we should ever do this. If I'm a source plugin, I have a prepareRow() method that I can overload and just call my parent, I shouldn't have to implement an event listener to do stuff there, that's for others that want to extend/change/react based on what the default source does.

I think that blocker's gone now.

If wanted to add the event, we could do that in 8.3.x (doesn't mean we have to of course, and if it's just hooks vs. events I personally prefer #2237831: Allow module services to specify hooks for that). Bumping down to 8.3.x for that reason, but moving to minor since I don't think this is enabling anything new? The return change seems like the smallest difference here.

Sorry I haven't read the whole discussion just having a comment on this and one idea to share (which I've already implemeted):

> SourcePluginBase::prepareRow() should dispatch a PREPARE_ROW event instead of invoking hooks.

Not only that. It should allow to override in child classes. I don't like EVENT system here at all because
1) event handlers are totally unconnected from the migration
2) get called on every sneeze! I.e. there are always multituide of empty calls
3) from (1) -> you have to write if-s to just filter out unnecessary events, not related to your migration.

That said, here is a solution:

1) add checkRow() method to SourcePluginBase:

  /**
   * Checks row data and skips the row in case of errors.
   *
   * It is similar to prepareRow() but can skip rows and save them to map-table.
   * It can also throw MigrateSkipRowException.
   *
   * @param \Drupal\Migrate\Row $row
   *   The row object.
   *
   * @throws \Drupal\migrate\MigrateSkipRowException
   *
   * @return bool|NULL
   *   FALSE if this row needs to be skipped.
   */
  public function checkRow(Row $row) { }

2) rewrite prepareRow like this:

  /**
   * {@inheritdoc}
   */
  public function prepareRow(Row $row) {
    $result = TRUE;
    try {
      // here should go a hook-event part, I'm skipping it for now
      // ...
      // ...
      $result = $this->checkRow($row); /// <<< NEW LINE
      $skip = $result === FALSE; /// <<< NEW LINE
      $save_to_map = TRUE;
    }
    catch (MigrateSkipRowException $e) {
      $skip = TRUE;
      $save_to_map = $e->getSaveToMap();
      if ($message = trim($e->getMessage())) {
        $this->idMap->saveMessage($row->getSourceIdValues(), $message, MigrationInterface::MESSAGE_INFORMATIONAL);
      }
    }

    // We're explicitly skipping this row - keep track in the map table.
    if ($skip) {
      // Make sure we replace any previous messages for this item with any
      // new ones.
      if ($save_to_map) {
        $this->idMap->saveIdMapping($row, array(), MigrateIdMapInterface::STATUS_IGNORED);
        $this->currentRow = NULL;
        $this->currentSourceIds = NULL;
      }
      $result = FALSE;
    }
    elseif ($this->trackChanges) {
      // When tracking changed data, We want to quietly skip (rather than
      // "ignore") rows with changes. The caller needs to make that decision,
      // so we need to provide them with the necessary information (before and
      // after hashes).
      $row->rehash();
    }
    return $result;
  }
}

Now in any Source child we can easily do (example):

  /**
   * {@inheritdoc}
   */
  public function checkRow(Row $row) {
    // Prepare `destination_file_uri` source property for `entity:file` destination plugin
    // We are mirroring remote directory structure
    $local_file_uri = NULL;
    if ($value = $row->getSourceProperty('image_path')) {
      // We support only local paths starting with "/".
      if (strpos($value, '/') === 0) {
        $path = substr($value, 1);
        $local_file_uri = $this->get('local_files_uri') . '/' . $path;
      }
      else {
        throw new MigrateSkipRowException(t('Not handled file uri: @uri', ['%uri' => $value]));
      }
    }
    $row->setSourceProperty('destination_file_uri', $local_file_uri);
  }

OR... we could just return FALSE.

> SourcePluginBase::prepareRow() should dispatch a PREPARE_ROW event instead of invoking hooks.
> Catch MigrateSkipRowException in SourcePluginBase instead of relying on a FALSE return.

That's two changes, not one. This should be split into two issues.

> Catch MigrateSkipRowException in SourcePluginBase instead of relying on a FALSE return.

That's a small, easy change, and an improvement to DX, as returning FALSE to skip means that developers have to figure out what to return if you don't want to skip. I've seen lots of migration source plugins that return TRUE here to mean the row is ok, when all they need to do is fall out of the method.

The current behaviour of relying on a FALSE return can be kept for BC.

Perhaps we should re-boot the conversation here on adding an event. See https://github.com/drush-ops/drush/pull/3402 where we are trying to bring things from migrate_run (migrate_tools fork) into drush core. In there, we kinda need an event unless we do really hacky things.

@joachim, I split the event constant, event class and event dispatching in #2952291: Dispatch the PREPARE_ROW event. Deprecate prepare_row_alter hooks.

Re-worked the IS to remove the try/catch stuff. If we want to handle that, we can open that in a new issue. This is now only focused on the MigratePrepareRowEvent moving from migrate_plus into core.

@heddn, in #33 I split the event part in a new issue #2952291: Dispatch the PREPARE_ROW event. Deprecate prepare_row_alter hooks