Problem/Motivation

MigrateDestinationInterface::rollback() has a $destination_identifier parameter.

This is an array of ID values, which are keyed by the key names.

The documentation should say that the array is keyed.

Issue fork drupal-3247205

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

joachim created an issue. See original summary.

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
commented
Assigned: Unassigned » beatrizrodrigues

I'll work on this

beatrizrodrigues opened merge request !1398

beatrizrodrigues closed merge request !1398

beatrizrodrigues opened merge request !1399

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
commented
Assigned: beatrizrodrigues » Unassigned
Status: Active » Needs review

I changed the documentation from MigrateDestinationInterface::rollback() parameter.

anagomes’s picture

anagomes commented
Assigned: Unassigned » anagomes
anagomes’s picture

anagomes commented
Assigned: anagomes » Unassigned
Status: Needs review » Reviewed & tested by the community

The documentation now states that the array is keyed, this solves the issue.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

Thanks for working on this. I think we can do better with that description. Saying that an array is keyed by key names seems self evident to me. And we can refer to the getIds, which will help the reader.

I think this is more informative.

   * @param array $destination_identifier
   *   An associative array of destination IDs for the object that is being
   *   rolled back. The array keys are defined by the destination plugin. 
   * 
   * @see \Drupal\migrate\Plugin\MigrateDestinationInterface::getIds
joachim’s picture

joachim commented

How about:

> The array keys are defined by the destination plugin's INTERFACE::getIds() method.

so the api docs make the link?

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
commented

I'm sorry, the description that I put is not good, indeed. Thank you for the comment with a better one :)

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
commented
Status: Needs work » Needs review
quietone’s picture

quietone commented
Status: Needs review » Needs work

@joachim, what do you think?

joachim’s picture

joachim commented

It seems ok to me. The destination plugin is a thing. More specifically, it's an object, and being an object it has a class and methods.

If anything, I'd say that this:

> MigrateDestinationInterface::getIds() method used by the destination plugin.

isn't correct, as it makes it sound like getIds() is called BY the plugin. It's not, it's a method on the plugin. The plugin's method.

I guess I'll accept the distinction that the plugin is a concept and it happens to be implemented with a class... in which case, let's say:

> The array keys are defined by the destination plugin class's \Drupal\migrate\Plugin\MigrateDestinationInterface::getIds() method.

Or even stricter:

> The array keys are defined by the destination plugin class's implementation of \Drupal\migrate\Plugin\MigrateDestinationInterface::getIds().

What I do want to nitpick though is:

> * An associative array of destination IDs for the object that is being

I'd rather this said 'row' than 'object'.

quietone’s picture

quietone commented

It appears that I learned the use of the possessive form different than you.

An associative array of destination IDs for the row that is being rolled back.
To me that reads like the row is being rolled back when in really is something else.

joachim’s picture

joachim commented

> It appears that I learned the use of the possessive form different than you.

Possibly! I'm not sure what you mean though. I thought your point was mostly about the plugin being a concept rather than the actual class.

> To me that reads like the row is being rolled back when in really is something else.

Yes, to me too. It is the row that is being rolled back, isn't it? I guess the command the user issued was to roll back the *migration*, but then each row is rolled back one by one. But I've just been to read existing docs, and:

   * Delete the specified destination object from the target Drupal.
   *
   * @param array $destination_identifier
   *   The ID of the destination object to delete.
   */
  public function rollback(array $destination_identifier);

so fair enough, let's keep 'object' to be consistent.

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
commented

Thank you for the comments, I'm learning a lot with the discussion. But I couldn't be able to understand if 

   *   An associative array of destination IDs for the object that is being
   *   rolled back. The array keys are defined by the
   *   \Drupal\migrate\Plugin\MigrateDestinationInterface::getIds() method used
   *   by the destination object.

is the right form. I changed the last word to object, following the comment #16 where says

so fair enough, let's keep 'object' to be consistent.

I would change the frist line object to row, as I agree with

Yes, to me too. It is the row that is being rolled back, isn't it?

So, I would do:

   *   An associative array of destination IDs for the row that is being
   *   rolled back. The array keys are defined by the
   *   \Drupal\migrate\Plugin\MigrateDestinationInterface::getIds() method used
   *   by the destination object.

So we do not repeat the word object.

Tell me if I got something wrong, please. If it is fine I do the change to the MR.

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
commented
Status: Needs work » Needs review

So, i did the changes as I said in #17. Please let me know if something is wrong or could be better. Thank you.

lucienchalom’s picture

lucienchalom commented
Status: Needs review » Reviewed & tested by the community 
   *   An associative array of destination IDs for the row that is being
   *   rolled back. The array keys are defined by the
   *   \Drupal\migrate\Plugin\MigrateDestinationInterface::getIds() method used
   *   by the destination object.

This is the present documentation.
What I, as a nonnative speaker and without much context can understand, is that this array is of IDs for each row that will be rolled back by the rollback method. These IDs are from getIDs method and passed as the parameter to rollback().
If this is correct, we can move to RTBC.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

I think this needs work.

The docbloc is now this:

   * Delete the specified destination object from the target Drupal.
   *
   * @param array $destination_identifier
   *   An associative array of destination IDs for the row that is being
   *   rolled back. The array keys are defined by the
   *   \Drupal\migrate\Plugin\MigrateDestinationInterface::getIds() method used
   *   by the destination object.

The summary line states that the destination object is removed but the comment says the row is rolled back. That is contradictory. The summary line is correct in that it is the destination object that is rolled back (and the row removed).

joachim’s picture

joachim commented

> The summary line is correct in that it is the destination object that is rolled back (and the row removed).

I would have said that we roll back a row, and when the row is rolled back, the destination object is deleted.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Component: documentation » migration system
Status: Needs work » Needs review
Issue tags: -Novice, -migrate

Let's get this done.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Fairly confident this document update didn't break nightwatch

quietone’s picture

quietone commented

Just a rebase with not conflicts.

catch closed merge request !1399

  • catch committed aaea5854 on 10.3.x 
    Issue #3247205 by beatrizrodrigues, quietone, joachim:...

  • catch committed d21dca2d on 10.4.x 
    Issue #3247205 by beatrizrodrigues, quietone, joachim:...

  • catch committed 2cc3c276 on 11.0.x 
    Issue #3247205 by beatrizrodrigues, quietone, joachim:...

  • catch committed 4edd2db6 on 11.x 
    Issue #3247205 by beatrizrodrigues, quietone, joachim:...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

Status: Fixed » Closed (fixed)

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