Remove broken idlist handling, replace with more robust exception handling

Please do not credit me, all I changed is a sizeof to count. In general we do not use PHP provided aliases to functions.

OK the patch moving code around mislead me, this is not correct.

The priority order is in SourcePluginBase::next() and idList is way way higher than idmap. If idList is specified then idmap needs to be ignored.

Edit: + // \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::__construct( ensures the idlist is single long. comment needs to be removed but I am sure we will find more reasons to reroll.

Edit: Funny drupal.org glitch, the patch uploaded here is https://www.drupal.org/files/issues/2522012_17.patch the interdiff is https://www.drupal.org/files/issues/interdiff_14284.txt and this is likely to be the one to be committed.

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -198,6 +190,23 @@ protected function getModuleHandler() {
   +      // @TODO: Add support for multiple IDs https://www.drupal.org/node/2529744
   +      if (count($this->getIds()) != 1) {
   +        throw new \LogicException('Source IDs can only be specified by a list if there is only one id.');
   +      }
   

   Given this, i'm wondering if we should either remove idList entirely until we find a way to bring it back or just fix it all here? It's only going to be utilised by contrib?

2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -103,6 +103,21 @@ protected function prepareQuery() {
   +    // keys, so throw an exception if the key is multi-value.
   

   Nothing here throws an exception?

3. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -103,6 +103,21 @@ protected function prepareQuery() {
   +          $field = $definition['alias'] . '.' . $field;
   

   Is joining alias.field like this correct? Eg, will it impact other database drivers? I know this is SqlBase but we have made fixes previously so it works with MS SQL and the like.

4. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -116,19 +131,8 @@ protected function initializeIterator() {
   +    if (!$this->getIdList()) {
   

   Does this mean that we never join on the map table if we have an id list? That doesn't seem right? EDIT: this is the correct behaviour SourcePluginBase::next() handles processing the row.

5. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -200,7 +204,7 @@ protected function initializeIterator() {
   +    return $this->prepareQuery()->countQuery()->execute()->fetchField();
   

   Why do we call prepareQuery() now? Is it so count is correct when you provide an id list?

Separating out into the two separate methods as we discussed in IRC looks good from the patch but it's hard to follow. I'll try review this properly locally later today.

So, an important point to note is that the ID list is purely a runtime concept - on a given migration run you will be able to say something like

drush mi articles --idlist=45,890

And on that run only import those two specific records from the source. The ID list is not a fundamental alteration of the logical query - the count() method should always return the count of all available source records (not that it would be likely to be called in this context anyway).

Keep in mind that other source plugins need to implement ID lists by iterating the full source looking for matching IDs, and this represents the logic of the feature. Adding the list of IDs directly to the SQL query is purely an optimization available only to the SQL source plugin and should not be treated as a "real" modification to the logical query represented by the source.

My feeling is that the idlist should not use configuration, since it's strictly a run-time option - attached is a version with a simple setter. Anticipating #2529744: SourcePluginBase does not handle multi-value IDs being implemented as a follow-up, I've also changed the ID list to an array of id key arrays, rather than an array of scalar IDs, so the API won't have to change with that follow-up (it will just need to figure out how to handle multi-value IDs when they do come in).

As previously, waiting on the count() patch to get the testing straightened out.

Also, see this patch being used in the real world at #2432977: Implement --idlist option on migrate-import.

Looking good, the split up into new methods seems like a nice clean-up, few nitpicks and questions below:

1. +++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
   @@ -62,4 +62,12 @@ public function __toString();
   +   * @param array[] $id_list
   +   */
   +  public function setIdList(array $id_list);
   

   Missing param docs.

2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -84,11 +84,12 @@
   -   * @var array
   +   * @var array[]
       */
   -  protected $idList = array();
   +  protected $idList = [];
   

   Is array[] valid syntax for an array of arrays? Change which seems to be just syntax.

3. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -303,14 +306,14 @@ public function rewind() {
   -      $row = new Row($row_data, $this->migration->getSourcePlugin()->getIds(), $this->migration->get('destinationIds'));
   +      $row = new Row($row_data, $this->getIds());
   

   Nice!

4. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -92,103 +87,152 @@ protected function select($table, $alias = NULL, array $options = array()) {
   +    if (isset($high_water_property['name']) && ($high_water = $this->migration->getHighWater()) !== '') {
   

   Is checking the highwater against an empty string still a thing? I thought we made that NULL ages ago.

5. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/FieldInstancePerViewMode.php
   @@ -22,7 +22,9 @@ class FieldInstancePerViewMode extends ViewModeBase {
   +    $query = $this->query();
   +    $this->addQueryInfo($query);
   +    $result = $query->execute();
   

   It's a bit worrying that every source has to remember to addQueryInfo()? Seems like something that could easily be overlooked?

I discussed this with phenaproxima a day or two ago, sorry I didn't update the issue - I believe we should be able to remove idlist handling entirely from core and put it in contrib's hands if we implement a prepareRow event as a followup to #2535458: Dispatch events at key points during migration. I wouldn't abandon the approach in this issue entirely pending a POC, but I wouldn't invest significant time in it either until we see if that works as I imagine. The basic idea would be the clients could listen to prepareRow events and skip rows there if they don't match the idlist (the general functionality), and they could also do a query alter on SQL sources to optimize the queries up front.

To expand on my last thought - I see idlist as purely a front-end feature, and with some core tweaks that would be generally useful (rather than narrowly targeted to idlist handling) I think the contrib side can do idlists itself. On the core side, the main thing that I think is missing is a way to skip a row without recording it in the ID map - right now prepareRow(), when receiving a FALSE return from a hook, unconditionally records it as STATUS_IGNORED in the map table, which isn't appropriate in the idlist case. My thinking is to add a boolean to MigrateSkipRowException to indicate whether this particular row skippage should be recorded as "ignored", or the skip should simply be done silently. So, this patch as I'm conceiving it at the moment would extend the exception and add handling of the exception to prepareRow(), as well as removing the existing broken idlist stuff.

On the contrib side, the response to prepareRow() would check the incoming source ID(s) against the idlist, and if they don't match throw the don't-record-as-ignore MigrateSkipRowException - this provides the general functionality for all source plugins. Next step would be altering SQL plugin queries as an optimization in that case.

Let's see how this all works out...

Here it is - the functional changes (adding a boolean option to MigrateSkipRowException, and catching and handling it in prepareRow) are actually fairly small and simple, the bulk of the line changes are boilerplate - removing idlist references from the tests in particular, moving prepareRow() calls in source plugins and removing their returns (no existing source plugins skip rows), adding a couple parent::prepareRow($row) calls to source plugins that were missing them, and removal of the previous (non-functional) idlist handling re-indenting a chunk of code.

Also, note that the patch #2432977: Implement --idlist option on migrate-import uses this to implement --idlist for the drush migrate-import command.

Looking at writing tests, #2544690: Create new source plugin taking data from plugin config would be very handy...

Let's get #2544690: Create new source plugin taking data from plugin config in and use it for tests.

In the meantime, I realized removing the returns from the specific source plugins' parent::prepareRow() was wrong - I was thinking propagating the skips was now event-driven, but these are actually above the place where the events are caught (SourcePluginBase::prepareRow(), a.k.a. 'parent' in those calls). So, they need to propagate the FALSE that results from a hook throwing the event, up to next(). I still want to refactor that and just use events completely up the chain, but I'll revisit that when following up in #2488836: Refactor prepareRow() to add events in addition to hook - for now, we'll keep the existing behavior and just do what's needed to allow contrib to throw the skip events (and in particular to implement the idlist functionality).

I did keep the ContactCategory, Upload, and D7 File prepareRow() fixes (they weren't calling the parent at all), as well as the missing return in CckFieldValues - one might argue fixing the existing prepareRow() bugs isn't in scope, but since those bugs prevent the row skips we're dealing with from working for those sources, I'm comfortable fixing them here.

A nascent test module is in here, waiting on #2544690: Create new source plugin taking data from plugin config to provide the test data.

1. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -754,7 +754,7 @@ public function destroy() {
   -   * @todo Support idlist, itemlimit.
   +   * @todo Support itemlimit.
   

   I know this is pre-existing but is there an issue for itemLimit we can link here?

Actually, we should take the @todo for itemlimit out, the plan is to also do that entirely in contrib: #2432983: Implement --limit option on migrate-import. That is dependent on #2545672: Handle various migration interruption scenarios, which could use a review (hint hint;).

Retitled to reflect the current direction. Building on the status/interruption patches recently committed, we need to extend MigrateSkipRowException to permit skipping a row without writing the map table, and we also need to be able to handle that exception at prepareRow() time.

Rerolled. Hoping #2544690: Create new source plugin taking data from plugin config gets in soon so we can have a nice simple test, but I can write an ugly one if need be...

OK, I've implemented a test without the benefit of the nice source plugin, so it has its own.

I'm having trouble coming up with a clearer name that recordInMap, anyone else have any thoughts?

#2544690: Create new source plugin taking data from plugin config is in (woohoo!), so I'll reroll the test here to make use of the new source plugin.

Here we go, interdiff shows how much nicer the embedded_data plugin makes testing.

Didn't thoroughly look at the tests but the rest looks OK and removing idList from core is fine by me.

1. +++ b/core/modules/file/src/Plugin/migrate/source/d6/Upload.php
   @@ -47,6 +47,7 @@ public function prepareRow(Row $row) {
   +    return parent::prepareRow($row);
   
   +++ b/core/modules/file/src/Plugin/migrate/source/d7/File.php
   @@ -71,6 +71,7 @@ public function prepareRow(Row $row) {
   +    return parent::prepareRow($row);
   

   Looking in SourcePluginBase::prepareRow() it does all the map stuff, which is nothing like what most source plugins will do in prepareRow(). Could we move that follow-up so calling parent::prepareRow() in source plugins isn't so critical? Maybe that code could move to an event after #2488836: Refactor prepareRow() to add events in addition to hook

2. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -754,7 +754,7 @@ public function destroy() {
   +   * @todo Support itemlimit.
   

   Did we have an issue for this?

Could we move that follow-up so calling parent::prepareRow() in source plugins isn't so critical? Maybe that code could move to an event after #2488836: Refactor prepareRow() to use events and exceptions

Yes, I definitely want to follow up with that issue to cleanup prepareRow() - even if I can't get buy-in to the event concept, it needs some refactoring (see also #2558047: [meta] Dealing with conditions during migration (messaging/exceptions) is confusing and inconsistent).

re: "@todo Support itemlimit." - I should have removed that comment with #2545672: Handle various migration interruption scenarios, the item limit feature is now implemented entirely in contrib as a result.

Removed obsolete references to itemlimit.

I was a bit skeptical about this, since instead of fixing idlist, it's removing it. However, Mike assures me that there's a sister patch in the Migrate Plus queue to add it back, and benjy says in #64 that he's ok with this not being in core. It still seems like a really useful feature to me, so maybe we can try and add it back in 8.1.x or so.

Committed and pushed to 8.0.x. Thanks!