Polish migrate.api.php API documentation

+1

I am not sure about this. I am concerned that there would be too much detail, so much so that it wouldn't be helpful. Looking at the documentation for the other APIs, none of them are very long. At least not as long as migration would be with the addition of all the source, process and destination classes, which grep tells me is 213 classes.

Maybe it needs to be as proposed but is there an alternative?

Ah, that makes more sense! Option 2 seems reasonable. Although, I like the idea of sub topics for the source, process, and destination plugins, which seems to me that it would make it easier to find the plugin you need. I mean if I am trying to figure out how to process something I want to just look at the process plugins and not be distracted by other information. I keep this list bookmarked for just that reason. But maybe it is the right thing to do, that is, to do option 2 as is. I've had my say and will be content to go with what others think is best.

BTW, should we have a similar thing for the migrate drupal plugins?

Thank you for improving this file! Just a few minor improvements:

1. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,80 @@
   + * (ETL) process. In the Drupal Migrate API the extract phase is called
   

   I would add a comma after API.

2. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,80 @@
   + * can be established. After processing the transformed row is passed to the
   

   I would add a comma after processing.

3. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,80 @@
   + * Examples of migration plugin definitions can be found in @link https://www.drupal.org/node/2917492 Migrate API handbook. @endlink
   

   I would add 'the' before the @link.

4. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,80 @@
     * with \Drupal\migrate\Annotation\MigrateSource annotation, and must be in
   ...
   + * \Drupal\migrate\Annotation\MigrateProcessPlugin annotation, and must be in
   ...
     * annotated with \Drupal\migrate\Annotation\MigrateDestination annotation, and
   

   I'm not sure about the commas before those 'and'.

5. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,80 @@
   + * @link https://api.drupal.org/api/drupal/namespace/Drupal!migrate!Plugin!migrate!source List of source plugins provided by the core Migrate module. @endlink
   ...
   + * @link https://api.drupal.org/api/drupal/namespace/Drupal!migrate!Plugin!migrate!process List of process plugins for common operations provided by the core Migrate module. @endlink
   ...
   + * @link https://api.drupal.org/api/drupal/namespace/Drupal!migrate!Plugin!migrate!destination List of destination plugins for Drupal configuration and content entities provided by the core Migrate module. @endlink
   

   Those links don't seem to work without a specific version at the end, like /8.5.x.

6. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,80 @@
     * \Drupal\migrate\Plugin\MigrateSourcePluginManager class. Source plugin
     * providers are determined by their and their parents namespaces.
   

   I'm not sure this last sentence is true, and the 'by their and their' part is confusing.

I can't do a review right now but ...

5. Only concern is how will we remember to keep it up to date? Do it like the dumps, where we have an issue to keep them up to date?

6. Possibly related to this and then it will be needed. http://cgit.drupalcode.org/drupal/tree/core/modules/migrate/src/Plugin/M...

Only concern is how will we remember to keep it up to date? Do it like the dumps, where we have an issue to keep them up to date?

I propose that we commit the patch with the 8.5.x version argument, but before we do, we open a follow-up to remove them, postponed on #2937926: Namespace URLs don't work without branch.

Possibly related to this and then it will be needed.

I think it's handled appropriately in the current patch. Source providers are a tricky concept, and if we want to document them in migrate.api.php, I propose we open a follow-up for that.

I'm bringing an outside set of eyes (unfamiliar with most items mentioned in this doc). I'm reading for clarity and to confirm that the changes listed in the proposed resolution are in fact applied in all instances. Also confirmed links, that the patch applied cleanly and similar basics.

Outcome: looks good to me. I'm marking RTBC, as I see no additional work required for a commit.

1. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,79 @@
   + * can be established. After processing, the transformed row is passed to the
   

   kudos for fixing the comma splice! (ok, frivolous comment but it deserves appreciation)

Hi everyone. I perused prior to commit and noticed a few things in the places words have been changed:

1. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,79 @@
   + * the term 'load' refers to loading data from the storage.
   

   The definite article in front of storage seems to indicate a particular storage.

2. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,79 @@
   + * data source. The source is typically a database, but it can also be a CSV,
   

   "The data source is typically a database." 'Source' is a phase, right?

3. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,79 @@
   + * data source. The source is typically a database, but it can also be a CSV,
   + * JSON or XML file. The row is sent to the process phase where it is
   

   Is "CSV, JSON or XML" comprehensive, or "for example"?

4. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,79 @@
   + * that a stub needs to be created. For example, if a term has a parent term
   

   Would it make sense to put "stub" in quotes as a newly-introduced term? I'm not sure on this one. I'm really just asking.

5. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,79 @@
   + * process and destination) also use plugins so there are four types of plugins
   

   "Use" or "are defined as"? If it's the same definition as the prior sentence, use the same words.

    * The ETL process is defined as a migration plugin. The three phases (source,
    * process and destination) also use plugins so there are four types of plugins
    * in the migration process.
   

   Assuming I understand this right, what about "The overall ETL process and the three phases (source, process and destination) are defined as migration plugins" and that's all?

Thanks, all!

Should take care of #22.

Thank you!

I just remembered #2640718: Replace i.e. and e.g. with English words in core/modules A-L, so I suggest instead of this:

+++ b/core/modules/migrate/migrate.api.php
@@ -10,77 +10,80 @@
+ * data source. The data source is typically a database, but it can also be
+ * for example, files (e.g., CSV, JSON or XML) or web services (e.g.,

this:

"The data source is typically a database, but it can also be, for example, files (CSV, JSON or XML) ..." and so on.

Here we go.

I spoke to @jhodgdon at Pacific Northwest Drupal Summit and I think we can probably go ahead with this patch without the 8.5.x namespace links -- she expects to get #2937926: Namespace URLs don't work without branch in within a couple of days. So let's re-roll without the mentions of 8.5.x and postpone on that.

This should work with the patch on #2937926: Namespace URLs don't work without branch.

fwiw:

I glanced through additional proposed revisions. Nothing seems substantial enough to justify a delay to committing. No writing is ever perfect. I would respectfully submit that further changes don't quite justify additional delay.

May I propose this ship, with a note that people can then fix 'nits' here and there after the current patch is committed, applying polish over time?

Setting back to rtbc, with thanks to the group for the efforts and the commitment to high quality api documentation.

(If it helps people at all to know, my history as an editor, writer and translator in English spans a few decades. I'm the first to click the 'edit' button when wording or punctuation feels a little 'off', so I truly do write the above recommendation will all respect for the sensibilities of the people continuing to propose improvements).

cross posted with #32:

Let's get all the doc nits addressed in a single patch. We're pretty good at not letting docs patches flounder and the committers are pretty good at committing things once they get to RTBC, so let's do this in a single patch, rather than a follow-up.

1. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,79 @@
   + * the term 'load' refers to loading data from a storage.
   

   I prefered "from storage", but it's just a matter of taste, I don't think "from a storage" is a mistake, so let's leave this like this, unless you think otherwise.

2. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,79 @@
   + * Examples of migration plugin definitions can be found in the @link https://www.drupal.org/node/2917492 Migrate API handbook. @endlink
   

   Should the @link be on a new line?

Regarding #35.1, I think either "from storage" or "from a storage" is fine, although I personally prefer "from storage". In English, "a storage" is descriptive and usually followed by something else, like "a storage unit", or "a storage handler". But it's not a big deal.

Thanks @masipila!

This is ready, as far as I can tell.

I would be great to get a +1 from all the sub-system maintainers just so it's easy to know that you are all +1.

I've read through and the changes look fine to me.

So a +1 from phenaproxima, heddn and quietone would be great.
I think we have one from masipila (wrote the patch) and maxocub (pressed the rtbc).

+++ b/core/modules/migrate/migrate.api.php
@@ -10,77 +10,80 @@
+ * Mirate API destination plugins implement

Oops! There is a typo here -- "Mirate" should be "Migrate" :)

Otherwise, I have read it through and it looks good. Consider this my official +1.

Some more nits since I'm looking at this as well. Otherwise, I like the wording here and am +1 on committing.

1. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,80 @@
   + * to be skipped. Processing can also determine if a 'stub' needs to be created.
   

   I think this is really an example of what /could/ happen in a process plugin. Here's a suggested re-wording.
   ... can do lots of things, like determine if a 'stub'...

2. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,80 @@
   + * For example, if a term has a parent term which hasn't been migrated yet, a
   

   And if we re-word things, perhaps it would follow that wording like the following would be better.

   In this example, if a term has a parent term...

3. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,80 @@
   + * (source, process and destination) also have their own plugin types.
   

   Do we need to mention that these aren't the only plugin types possible in a migration? I don't want to discuss field plugins here, but I also don't want to give the impression this is all inclusive either.

4. +++ b/core/modules/migrate/migrate.api.php
   @@ -10,77 +10,80 @@
   + * (for example CSV, JSON, XML) or fetched from a web service (RSS or REST). The
   

   Nit: For example RSS or REST. This isn't an all-inclusive list of web services.

Re #44.4:

+++ b/core/modules/migrate/migrate.api.php
@@ -10,77 +10,80 @@
+ * (for example CSV, JSON, XML) or fetched from a web service (RSS or REST). The

I would also add an 'or' between JSON and XML: (for example CSV, JSON or XML)

+1 for me too.

For point #3 above, we mention that the 3 phases of the ETL process are mapped by plugins. I am mildly wondering if we should allude to the fact that other plugins may also be provided so don't just think that these are the only pugins you'll encounter... For example, migrate_drupal provides field plugins. Migrate Plus provides data fetcher plugins.

Maybe something like:

The three phases (source, process and destination) also are directly mapped to their own plugin types.
vs. previously:
The three phases (source, process and destination) also have their own plugin types.

This makes it crystal clear that each ETL phase is a plugin and the entire migration is a plugin. Which I think is what we are trying to convey. It is cutting hairs, I know.

+++ b/core/modules/migrate/migrate.api.php
@@ -10,77 +10,81 @@
+ * needed or marked to be skipped. Processing can also determine if a 'stub'
+ * needs to be created. For example, if a term has a parent term which hasn't
+ * been migrated yet, a stub term is created so that the parent relation can be
+ * established, and the stub is updated at a later point. After processing, the

Good point about raising more questions. What if we remove that section entirely? Only a single, special process plugin (migration_lookup) does the stubbing. Why do we call it out here and not any others? Can we just remove it?

I agree with @masipila on this. Stubbing is a critically important concept and any good overview of the Migrate API should at least mention it.

However, perhaps we can avoid disagreement here and discuss it as its own concept. Outside of the process pipeline. In a new section of the API doc. And mention that stubbing happens during the process pipeline while touching on the topic. I don't think I disagree that it isn't an important topic. I just don't think of it as tightly connected to the process pipeline explanation.

Or, I can step aside from my bikeshedding. We get this in and open a follow-up to hammer out potentially different language.

Discussed this is in IRC. We'll open a follow-up to flesh out my concerns about singling out stubbing as the only process pipeline example. So once that is opened, I'm good with RTBC.

I've caught up, having read the issue and the IRC discussion as well and I support RTBC here.

A new section 'Other key concepts of the Migrate API' for stubbing, incremental and rollback as nicely resolve where/how to add this information and it is reasonable to do that in a followup. Yet, I do realize the masipila would like to complete it here. I see the merit in that too.

However, I too found a nit but that should not hold up RTBC, it is too minor.

+++ b/core/modules/migrate/migrate.api.php
@@ -10,77 +10,81 @@
+ * ETL refers to loading data into the storage while in a typical Drupal context
+ * the term 'load' refers to loading data from storage.
  *

Can the 'into' and 'from' be emphasized? I only ask because I had to read it twice but leave it if you think it isn't the right thing to do.

The last work here, making two followups and answering my question in #55 have been done. Regarding my question about emphasizing two words SHOUTING would be awful in my opinion and since it seems there are no other options let's just leave it is as. There is nothing incorrect about the sentence in question. Although, masipila does leave that open for a committer to decide.

No changes to the patch since my last read and everyone is in agreement so this is good to go.

+1 on RTBC.

Committed and pushed ecfdf60433 to 8.6.x and 2d0151b8f1 to 8.5.x. Thanks!

Backported to 8.5.x since this is a docs only change.