Problem/Motivation

Follow up to #3198608: trackLastImported YAML key to enable tracking the last import date of a row is undocumented and confusing. and also much needed.

Document the available definition properties of a migration defined in a yml.

Steps to reproduce

Go to class Migration and see no list or explanation of the properties.

Proposed resolution

Write the documentation

Remaining tasks

Patch
Review
Commit

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

Status: Active » Needs review
Issue tags: +documentaion
Parent issue: #3225227: Fix source plugin documentation »
FileSize
2.59 KB

OK, here is a start. Reviews welcome. Not running test because this is a doc only change.

huzooka’s picture

Status: Needs review » Needs work
Issue tags: -documentaion +API Documentation
  1. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -17,6 +17,70 @@
      * The migration plugin represents one single migration and acts like a
    

    What if we change also this line? I'd rather start this sentence with "A migration plugin instance..." It isn't crucial, but I've seen a lot of confusion already in regards of what is a plugin class, what is a plugin annotation, what is about a plugin definition, and what is an actual instance.

  2. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -17,6 +17,70 @@
    + *
    + * Available definition keys:
    

    It would be nice to see some notes here about the migration yaml files (see the issue title). Something like

    "Migration yml files in migrations subdirectories are plugin annotations written in Yaml format (they're are almost complete plugin definitions)..."

  3. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -17,6 +17,70 @@
    + * - label: The migration description.
    

    It's a label, not a description. See Migration::label:

    The human-readable label for the migration.

  4. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -17,6 +17,70 @@
    + * - process: The definition for the migrate process plugin pipeline.
    

    Can this be somehow formulated as "process pipelines of the destination properties"?

  5. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -17,6 +17,70 @@
    + *   optional migrations must be run first and completed successfully, but only
    

    Afaik nothing enforces that optional dependencies MUST be executed before running an actual migration.

    See e.g. MigrationPluginManager@L191 $required_dependency_graph = (new Graph($required_dependency_graph))->searchAndSort();.

  6. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -17,6 +17,70 @@
    + * - provider:
    

    TODO I guess.

  7. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -17,6 +17,70 @@
    + * Example with all keys:
    

    Example with all keys misses provider.

quietone’s picture

Status: Needs work » Needs review
FileSize
2.7 KB
3.07 KB

@huzooka, thanks!

This patch responds to all the points in #3.

joachim’s picture

Status: Needs review » Needs work

Looks good, but a few typos:

  1. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -14,9 +14,78 @@
    + * Migrations are plugin annotations defined using YAML format and placed in the
    

    I don't think 'annotations' is the right word here.

    Should simply say 'Migrations are plugins defined ...'

  2. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -14,9 +14,78 @@
    + * - deriver: (optional) An option fully qualified path to a deriver class.
    

    Sentence doesn't read properly.

  3. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -14,9 +14,78 @@
    + *   'optional' listing the migration that this migration depends on. The
    

    should be 'listing the migrationS'

  4. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -14,9 +14,78 @@
    + *   required migration must be run first and completed successfully. The
    

    missing plural 's' here too

  5. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -14,9 +14,78 @@
    + *   optional migrations will be execute if they are present.
    

    'executeD'

  6. +++ b/core/modules/migrate/src/Plugin/Migration.php
    @@ -14,9 +14,78 @@
    + * - provider: (optional) An array of module machine names that must be enabled.
    

    I've been using 'dependencies' for this, probably because I saw it once elsewhere. Does that work too, or have I been doing this pointlessly?

huzooka’s picture

@joachim, with respect: dependencies isn't a thing in in Migrate API's migration definitions/annotations (name they as you prefer).

You're mixing these with Migrate Plus migration configuration entities.

And regarding to your #1 point: the word "plugin" is ambiguous. If you just say "plugin", then we don't know what do you refer. It might be a (base) plugin definition described by the yaml files (or the definition of a derivative), or a plugin instance created by the manager.

quietone’s picture

Status: Needs work » Needs review
FileSize
1.63 KB
3.05 KB

Thanks for the reviews.

Changes for #6 2-6.

This sentence needs work and my brain is not up to it at the moment. Using YAML is only one way to create a migration plugin.
"Migrations are plugin annotations defined using YAML format and placed in the directory MODULENAME/migrations."

joachim’s picture

> @joachim, with respect: dependencies isn't a thing in in Migrate API's migration definitions/annotations (name they as you prefer).

Yup, that's what I meant -- I've been using these incorrectly as they have no effect! Must be a leftover from config migrations. Thanks for the clarification.

> "Migrations are plugin annotations defined using YAML format and placed in the directory MODULENAME/migrations."

Annotations are metadata in source code comments. In Drupal, we use Doctrine Annotations, and they're the '@Foo{}' type syntax in PHP comments, and we use them to put plugin definitions in the same code file as the PHP class for that plugin.

The plugin definition is the annotation. But for a YAML plugin, the plugin definition is the YAML. The definition is an array of data. The *format* of that definition is either an annotation, or a YAML file. And the plugin itself is an instantiated object of a particular class, which holds the definition array.

> Using YAML is only one way to create a migration plugin.

Do you mean using plugin derivers?

I would put:

> Migrations are plugins defined as YAML files and placed in the directory MODULENAME/migrations.

and if we want to also mention using derivatives, we can do so further down.

We could link to the API topic on plugins (https://api.drupal.org/api/drupal/core%21core.api.php/group/plugin_api/9...) but that only mentions annotation plugins and not YAML, so it's not that relevant.

huzooka’s picture

The plugin definition is the annotation. But for a YAML plugin, the plugin definition is the YAML.

Imho this is not true for migrations. Think about fieldable entity migrations: their definition contains the process pipelines of their fields as well, while their YAML file does not. And these field processes changing per derivative. If you get the instance of d7_node_complete:article, and check its plugin definition, it will contain lot more then the corresponding (base) d7_node_complete.yml file.

joachim’s picture

+++ b/core/modules/migrate/src/Plugin/Migration.php
@@ -49,8 +49,7 @@
  * deriver: Drupal\taxonomy\Plugin\migrate\D7TaxonomyTermDeriver
- * provider:
- *   - custom_module
+ * provider: custom_module

So is provider actually a single-valued property? In which case, does it work the same as the provider in other plugins, where if it's not specified, it defaults to the module where the plugin is defined?

joachim’s picture

> Imho this is not true for migrations. Think about fieldable entity migrations: their definition contains the process pipelines of their fields as well, while their YAML file does not. And these field processes changing per derivative. If you get the instance of d7_node_complete:article, and check its plugin definition, it will contain lot more then the corresponding (base) d7_node_complete.yml file.

That's true for all types of plugin. Derivatives change the basic definition.

I'm not sure we're actually arguing the same point here! I'm merely saying that the word 'annotation' refers to a particular thing and doesn't apply in the context of migration plugins.

quietone’s picture

Adding a note here to consider adding here or elsewhere the 'translations' property that can be used on the destination.

quietone’s picture

FileSize
656 bytes
3.05 KB

#10. Yes. I got mixed up. It is the source plugins that can have multiple providers, See \Drupal\migrate\Plugin\Discovery\AnnotatedClassDiscoveryAutomatedProviders.

In the hope of making progress, I have changed the second paragraph to use the terminology from \Drupal\migrate\Plugin\MigrationPluginManager::getDiscovery where the migration yaml files are referred to as 'configurations'.

Not running tests because this is documentation only.

quietone’s picture

In regard to #12, I added more @see directing to the destination plugins and source plugins in migrate module. I can't think of anything else to add here.

joachim’s picture

Status: Needs review » Reviewed & tested by the community

LGTM!

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

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 3226401-14.patch, failed testing. View results

Spokje’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC per #15 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.

alexpott’s picture

Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed ce85149c336 to 10.0.x and 85723106f69 to 9.4.x and 372a1fe0b78 to 9.3.x. Thanks!

Backported to 9.3.x as a docs improvement.

  • alexpott committed ce85149 on 10.0.x
    Issue #3226401 by quietone, joachim, huzooka: Add documentation of...

  • alexpott committed 8572310 on 9.4.x
    Issue #3226401 by quietone, joachim, huzooka: Add documentation of...

  • alexpott committed 372a1fe on 9.3.x
    Issue #3226401 by quietone, joachim, huzooka: Add documentation of...

Status: Fixed » Closed (fixed)

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