Problem/Motivation

\Drupal\migrate\Plugin\MigrateProcessInterface documents that multiple entry points can be used in the process plugin, but that is logic that lives on the \Drupal\migrate\ProcessPluginBase class.

Proposed resolution

Move the documentation from MigrateProcessInterface to where the mentioned logic is implemented, i.e. \Drupal\migrate\ProcessPluginBase and adjust as needed.

Remaining tasks

Write a patch.

User interface changes

N.A.

API changes

N.A.

Data model changes

N.A.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marvil07 created an issue. See original summary.

marvil07’s picture

Assigned: marvil07 » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
2.18 KB

One fix.

zymbian’s picture

+1 nice fix.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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.

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

marvil07’s picture

Component: documentation » migration system
Status: Needs review » Needs work
Issue tags: +Needs reroll
Related issues: +#2936821: unclear docs in MigrateProcessInterface

Moving component, marking as NW for a reroll.
This probably still makes sense even if the text changed a bit at #2936821: unclear docs in MigrateProcessInterface.

quietone’s picture

@marvil07, thanks for moving this to the migration.system! Now it will get some attention, the migrate maintainers work on that component and while I occasionally search for certain tags, I haven't searched the documentation component.

I wonder if this would benefit from an example? Something along these lines.

 * @code
 * public function foo($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
 *   ...
 * }
 * public function bar($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
 *   ...
 * }
 *
 * process:
 *   destination:
 *     plugin: new_process_plugin
 *     method: foo
 *     source: baz
 * @endcode
 * In this example a new process plugin, NewProcessPlugin defines methods
 * foo and bar. The example process pipeline uses the method 'foo'.
  1. +++ b/core/modules/migrate/src/Plugin/MigrateProcessInterface.php
    @@ -9,12 +9,6 @@
    - * A process plugin can use any number of methods instead of (but not in
    - * addition to) transform with the same arguments and then the plugin
    

    I'd prefer commas instead of parenthesis.

  2. +++ b/core/modules/migrate/src/Plugin/MigrateProcessInterface.php
    @@ -9,12 +9,6 @@
    - * "method" key. See \Drupal\migrate\Plugin\migrate\process\SkipOnEmpty and
    

    Move the 'See' to an @see.

AkashKumar07’s picture

Assigned: Unassigned » AkashKumar07

I am working on this.

AkashKumar07’s picture

Assigned: AkashKumar07 » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
945 bytes

Please review the patch.

quietone’s picture

Status: Needs review » Needs work

@AkashkumarOSL, thanks for working on this issue. Good to see progress here. Even for a small patch it helps the reviewer if you provide an a comment about what you did or did not do. Speaking for myself, before I look at the patch I would like to know if you have made fixes for 9.1 and 9.2 or had questions about it.

Looking at this again I see that we should just move the doc paragraph from MigrateProcessInterface to ProcessPluginBase. I mean, ignore the latest patch here and start from HEAD, then move the text from MigrateProcessInterface to ProcessPluginBase. Although the last line of that text will not make sense in ProcessPluginBase. I will leave it up to the creator of the next patch to decide what to do.

chandrashekhar_srijan’s picture

Status: Needs work » Needs review
FileSize
1.38 KB

Added doc paragraph in the ProcessPluginBase for method multiple() for Drupal 9.1.x
Please review.

chandrashekhar_srijan’s picture

FileSize
600 bytes

Added interdiff for #13

quietone’s picture

Issue summary: View changes
Status: Needs review » Needs work

@chandrashekhar_srijan, thanks for the patch and interdiff, nice.

+++ b/core/modules/migrate/src/ProcessPluginBase.php
@@ -39,7 +45,12 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+   * Indicates whether the returned value requires multiple handling.
+   *
+   * @return bool
+   *   TRUE when the returned value contains a list of values to be processed.
+   *   For example, when the 'source' property is a string and the value found
+   *   is an array.

This can be removed. It is out of scope.

What needs to happen here is to move the paragraph at the top of MigrateProcessInterface to ProcessPluginBase.

The doc for MigrateProcessInterface can be this

 * Migrate process plugins transform the input value.For example, transform a
 * human provided name into a machine name, look up an identifier in a previous
 * migration and so on.

and what was there gets moved to ProcessPluginBase, with a few tweaks.

 * Process plugins using extending this class  can use any number of methods,
 * thus offering different alternatives ways of processing. In this case, the
 * transform() method should not be implemented, and the plugin configuration
 * must provide the name of the method to be called via the "method" key. Each
 * method must have the same signature as transform(). The base class
 * \Drupal\migrate\ProcessPluginBase takes care of implementing transform() and
 * calling the configured method. See
 * \Drupal\migrate\Plugin\migrate\process\SkipOnEmpty and
 * d6_field_instance_widget_settings.yml for examples.

I still think there is room for improving the text. Ideas welcome!

bandanasharma’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
2.01 KB

Added the new patch and make the changes as per @quietone comment.

mikelutz’s picture

Category: Bug report » Task
Status: Needs review » Needs work
Issue tags: +Novice

This copied the docs from the interface to the base plugin, but didn't remove/change the docs on the interface. Back to NW for the adjustments needed to the interface.

ayushmishra206’s picture

Made the changes please review.

ayushmishra206’s picture

Status: Needs work » Needs review
mikelutz’s picture

Status: Needs review » Needs work

I think we still need some of that documentation on the interface. It just needs to make sense.

ayushmishra206’s picture

Status: Needs work » Needs review
FileSize
2.84 KB
2.84 KB

Made the requested changes in #20 from #15. Please review

mikelutz’s picture

Requeuing the test.

quietone’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/ProcessPluginBase.php
    @@ -12,10 +12,26 @@
    + * Process plugins using extending this class  can use any number of methods,
    

    That first sentence doesn't read well. s/using extending/extending/
    Please check the spacing in this paragraph. There is at least one double space where it should be a single space.

  2. +++ b/core/modules/migrate/src/ProcessPluginBase.php
    @@ -12,10 +12,26 @@
    + * calling the configured method. See
    + * \Drupal\migrate\Plugin\migrate\process\SkipOnEmpty and
    + * d6_field_instance_widget_settings.yml for examples.
    

    Delete these and add @see d7_field_formatter_settings.yml to the end of the existing @sees.

  3. +++ b/core/modules/migrate/src/ProcessPluginBase.php
    @@ -12,10 +12,26 @@
    + *
    + * Migrate process plugins transform the input value.For example, transform a
    + * human provided name into a machine name, look up an identifier in a previous
    + * migration and so on.
    + *
    

    This is a repeat of what is in MigrateProcessInterface and does not need to be repeated here.

I applied that patch and there are two sections of @see in ProcessPluginBase instead of one. See Drupal API documentation standards for order of documentation sections for more details on that.

ayushmishra206’s picture

Assigned: Unassigned » ayushmishra206
ayushmishra206’s picture

Assigned: ayushmishra206 » Unassigned
Status: Needs work » Needs review
FileSize
1.67 KB
2.57 KB

Please review if this is what you recommended. Thanks @quietone

sarvjeetsingh’s picture

Status: Needs review » Needs work

hey @ayushmishra206, i tried to apply the patch and I found two white space errors on line 33 and 39 of the patch.
Other than that, all the changes as suggested above are done.

ayushmishra206’s picture

Status: Needs work » Needs review
FileSize
2.57 KB

Thanks for the review i solved those errors in this patch.

meghasharma’s picture

Assigned: Unassigned » meghasharma
meghasharma’s picture

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

patch applied cleanly..
now there is no error in the #27 patch, all the changes as suggested above are done.

quietone’s picture

Status: Reviewed & tested by the community » Needs work

@ayushmishra206, thanks for continuing to work on this. I just found a few final items.

I applied the patch and read the comments and found a few things that could be improved. I checked and there are no coding standard errors.

  1. +++ b/core/modules/migrate/src/ProcessPluginBase.php
    @@ -12,10 +12,20 @@
    + * offering different alternatives ways of processing. In this case, the
    

    s/alternatives/alternative

  2. +++ b/core/modules/migrate/src/ProcessPluginBase.php
    @@ -12,10 +12,20 @@
    + * The base class
    + * \Drupal\migrate\ProcessPluginBase takes care of implementing transform() and
    + * calling the configured method.
    

    This last sentence isn't needed since this is ProcessPluginBase.

  3. +++ b/core/modules/migrate/src/ProcessPluginBase.php
    @@ -12,10 +12,20 @@
    + * @see d7_field_formatter_settings.yml
    

    Let's move this to below the SkipOnEmpty line and keep all the classes together.

ayushmishra206’s picture

Assigned: Unassigned » ayushmishra206
ayushmishra206’s picture

@quietone happy to work! I have made the changes, please review.

ayushmishra206’s picture

Assigned: ayushmishra206 » Unassigned
Status: Needs work » Needs review
asishsajeev’s picture

Assigned: Unassigned » asishsajeev
asishsajeev’s picture

Status: Needs review » Reviewed & tested by the community

Patch applied successfully.
I Read the comments. All the changes suggested above are done.

asishsajeev’s picture

Assigned: asishsajeev » Unassigned
alexpott’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed a677705b3b to 9.1.x and 9b0a971a8e to 9.0.x and 0f3ef3d489 to 8.9.x. Thanks!

Backported to 8.9.x as this is a documentation only change.

  • alexpott committed a677705 on 9.1.x
    Issue #2949400 by ayushmishra206, chandrashekhar_srijan, bandanasharma,...

  • alexpott committed 9b0a971 on 9.0.x
    Issue #2949400 by ayushmishra206, chandrashekhar_srijan, bandanasharma,...

  • alexpott committed 0f3ef3d on 8.9.x
    Issue #2949400 by ayushmishra206, chandrashekhar_srijan, bandanasharma,...

Status: Fixed » Closed (fixed)

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