Problem/Motivation

The current doc block for MigrateDestinationInterface::import()'s return value reads:

   * @return mixed
   *   The entity ID or an indication of success.
   */

This is is incorrect. The return value should be array|bool. The method should return an array of destination ids indexed in the same order as they are defined in MigrateDestinationInterface::getIds() to save the ids to the idmap, or TRUE to report success and not save IDs to the idmap (such as for configs) or FALSE to indicate an error.

There is no situation where the plugin would return a scalar entity id as it would throw an error when the idmap attempted to iterate over it. It is also impropert to assume that a destination is even saving an entity, we shouldn't make any assumptions about what the destination plugin is doing.

All core plugins already have return values that meet this criteria.

Proposed resolution

Update the documentation to be clear.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 interdiff_9-11.txt839 bytesNeslee Canil Pinto
#11 3074047-11.patch978 bytesNeslee Canil Pinto
#9 interdiff_7-9.txt994 bytesNeslee Canil Pinto
#9 3074047-9.patch978 bytesNeslee Canil Pinto
#7 interdiff_2-7.txt1.01 KBNeslee Canil Pinto
#7 3074047-7.patch981 bytesNeslee Canil Pinto
#2 3074047-2.drupal.Update-MigrateDestinationInterfaceimport-return-type-documentation.patch978 bytesmikelutz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikelutz created an issue. See original summary.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
Status: Active » Needs review
FileSize
3074047-2.drupal.Update-MigrateDestinationInterfaceimport-return-type-documentation.patch978 bytes
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +mwds2019

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

daffie’s picture

daffie CreditAttribution: daffie commented
Version: 8.9.x-dev » 9.1.x-dev
Component: database system » migration system

This is a migration issue.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Yes, this does improve the documentation. I like it.

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented
FileSize
3074047-7.patch981 bytes
interdiff_2-7.txt1.01 KB

Changes ids to id's.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/migrate/src/Plugin/MigrateDestinationInterface.php
@@ -103,8 +103,11 @@ public function fields(MigrationInterface $migration = NULL);
+   *   An indexed array of destination id's in the same order as defined in the

I don't understand why id is changed to the possessive form it really is a plural. But the change caused me to take a take a second look and I realize that it should be IDs. So, I think it should be 'destination IDs'.

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented
Status: Needs work » Needs review
FileSize
3074047-9.patch978 bytes
interdiff_7-9.txt994 bytes
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Needs work

Thanks for the change! You may not believe this but now I see that 'id map' should be 'ID map' (there are two instances of that).

Really sorry for not seeing that earlier. (I must admit that 17 days of lockdown does have an adverse effect on me).

Neslee Canil Pinto’s picture

Neslee Canil Pinto
he/him
Primary language English
CreditAttribution: Neslee Canil Pinto at Specbee commented
Status: Needs work » Needs review
FileSize
3074047-11.patch978 bytes
interdiff_9-11.txt839 bytes

Made changes from #10

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thanks very much Neslee Canil Pinto!

This looks fine to me now, the capitalization of ID is correct everywhere and the explanation of the returned value is correct.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Documentation

Committed and pushed bbbed21e90 to 9.1.x and 1ad04e7c6f to 9.0.x and 638a526694 to 8.9.x and f33a51be4f to 8.8.x. Thanks!

As this is a documentation fix I've backported this to all the current branches.

  • alexpott committed bbbed21 on 9.1.x 
    Issue #3074047 by Neslee Canil Pinto, mikelutz, quietone: Update...

  • alexpott committed 1ad04e7 on 9.0.x 
    Issue #3074047 by Neslee Canil Pinto, mikelutz, quietone: Update...

  • alexpott committed 638a526 on 8.9.x 
    Issue #3074047 by Neslee Canil Pinto, mikelutz, quietone: Update...

  • alexpott committed f33a51b on 8.8.x 
    Issue #3074047 by Neslee Canil Pinto, mikelutz, quietone: Update...

Status: Fixed » Closed (fixed)

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