idMap source and destination id filtering requires keys

This is a great start, and a much more logical and developer-friendly way to look up IDs. I'm on the fence about whether we really need more test coverage (specifically, a fail patch for PostgreSQL).

1. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -428,8 +426,8 @@ protected function getFieldSchema(array $id_definition) {
   +      $query = $query->condition("map.$source_id", $source_id_values[$field_name], '=');
   

   Nit: No need for $query =. Or, for that matter, the = operator in condition().

2. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -467,11 +465,11 @@ public function getRowsNeedingUpdate($count) {
   +      $query = $query->condition("map.$destination_id", $destination_id_values[$field_name], '=');
   

   Ditto.

3. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -481,14 +479,20 @@ public function lookupSourceID(array $destination_id) {
   +    // The Migration process plugin doesn't currently have a way to provide the
   +    // source id keys so we have a compatibility layer for that. The order of
   +    // the source ids could be confused when using the Migration process plugin
   +    // and multiple source ids.
   

   I find this comment kind of confusing. Can it be reworded?

4. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -481,14 +479,20 @@ public function lookupSourceID(array $destination_id) {
   +      $query = $query->condition("map.$source_id", $have_keys ? $source_id_values[$field_name] : array_shift($source_id_values), '=');
   

   Ditto with $query = and the condition operator.

It looks fine to me, but I'd like @mikeryan's input for RTBC.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -428,8 +426,8 @@ protected function getFieldSchema(array $id_definition) {
+      $query->condition("map.$source_id", $source_id_values[$field_name], '=');

I agree with phenaproxima, let's take the opportunity to remove the needless ", '='" in all these query conditions.

As for deleteBulk() - in D7 that was used in association with bulk deletions on rollback, it has no known use case in D8, can we remove it here? Or, should I remove it in the rollback patch?

http://cgit.drupalcode.org/drupal/tree/core/modules/migrate/src/Plugin/m...

some this notaffected by patch? probably should return IDs with values

ZOMG. It's a friggin' fail patch, testbot.

lgtm

@benjy $id_map = $this->idMap->getRowBySource($this->currentSourceIds) as I pointed is not a getSourceIdValues() but protected variable that poorly documented

The posgres fails seem to be almost random at this point

I'm thinking the same thing, frankly. Take a look at this, which was just committed by @alexpott: #2572897: Promote and sticky options migrate to empty checkboxes. Given the patch in question, those test failures make absolutely no sense.

Looks great! I love this change -- makes things MUCH easier to understand.

FYI, this is breaking on the rollback support committed yesterday, working on it...

Well, that was harder than it looks... Ended up changing lookupSourceID() to return a keyed array to match how the source ID arrays are being used throughout, and currentDestination (added by rollback) to do the key thing like the rest of the patch. Also changed the executable rollback() to use deleteDestination() since that's what it's there for (although it actually never worked - fixed that) instead of the silly serialized source key. And, took the liberty of removing the unused bulkDelete()...

@Mixologic and @catch told me on IRC that we're OK RTBCing and committing based on DrupalCI results. If @benjy is OK with the latest patch, I am too.

Fixed some Eg. -> e.g. usages and committed and pushed to 8.0.x. Thanks!