API page: https://api.drupal.org/api/drupal/core%21modules%21migrate%21src%21Plugi...

> 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 configuration needs to provide the name of the method to be called via the "method" key.

That's not a clear sentence.

CommentFileSizeAuthor
#6 MigrateProcessInterface-docblock-fixed-2936821-6.patch1.44 KBmsankhala

Comments

joachim created an issue. See original summary.

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

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

msankhala’s picture

msankhala commented

@joachim I am not an expert in migration. What do you think should be the clear doc here?

joachim’s picture

joachim commented

How about:

> A process plugin will typically implement the transform() method to perform its work. However, it is possible instead for a process plugin to 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.

msankhala’s picture

msankhala commented

@joachim This makes perfect sense and more clear than the existing doc.

msankhala’s picture

msankhala commented
Status: Active » Needs review
Issue tags: +Novice
StatusFileSize
new MigrateProcessInterface-docblock-fixed-2936821-6.patch1.44 KB

Here is the patch.

lomasr’s picture

lomasr commented

I think we can include path to transform() method also core/modules/migrate/src/ProcessPluginBase.php. Which overrides public function MigrateProcessInterface::transform.

marxjohnson’s picture

marxjohnson commented
Status: Needs review » Reviewed & tested by the community

The patch applies fine and makes a lot more sense than the previous text. I don't think it's necessary to add a the full path of the file containing ProcessPluginBase. There's already a @see tag in the docblock for the MirgrateProcessInterface class so it should be linked when the docs are compiled, and PHPStorm found it fine for me on Ctrl+Click. In addition, you can see the transform() method in the MigrateProcessInterface file if you need to know the signature.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Crediting @joachim for filing the issue and reviewing. Crediting @lomasr and @marxjohnson reviewers for discussing the improvement.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 15946fea43 to 8.6.x and ad12560f98 to 8.5.x. Thanks!

As a docs improvement backported to 8.5.x.

+++ b/core/modules/migrate/src/Plugin/MigrateProcessInterface.php
@@ -9,10 +9,15 @@
  * migrate.migration.d6_field_instance_widget_settings.yml for examples.

migrate.migration.d6_field_instance_widget_settings.yml doesn't exist anymore so let's fix that as we go.

  • alexpott committed 15946fe on 8.6.x 
    Issue #2936821 by msankhala, joachim, lomasr, marxjohnson: unclear docs...

  • alexpott committed ad12560 on 8.5.x 
    Issue #2936821 by msankhala, joachim, lomasr, marxjohnson: unclear docs...
msankhala’s picture

msankhala commented

@alexpott Should the mention of migrate.migration.d6_field_instance_widget_settings.yml should be removed in another follow-up issue or I should submit the patch on this same issue?

Status: Fixed » Closed (fixed)

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