Add documentation to SourcePluginBase source plugin

Added configuration keys. Not quite sure what else to include on this one because it is an abstract and so examples aren't really applicable.

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -15,6 +15,12 @@
  * The base class for all source plugins.

This isn't strictly true. Contrib doesn't have to use this base class. Let's remove the word "all" and I'm fine. Yes, I know, it is a super small nit.

Good point, @heddn. Change made.

Docs seem sufficient, given this is an abstract class.

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -13,7 +13,13 @@
+ * - high_water_property: Information on the property used as the high-water mark.

Violates 80 cols rule.

A trivial fix of the comment length. Docs only change. Putting this back to RTBC.

As per issue summary "make sure [...] working with translations are included. Add suitable examples too.". So looks like this is not getting to where it intended to go.

I must be missing something then, because I cannot find anything special for i18n migrations. Which means we probably need an IS update. I do see in getIds() where in I18nProfileField we are adding multiple columns to make the ids unique, but that has another issue to update its docs in #2728869: Document public function MigrateSourceInterface::getIds return value better. There doesn't seem to be anything else needed in the source base plugin for i18n migrations.

this doesn't really do what was asked in #10. Specifically, it isn't more specific. Nor is does it touch the high_water_property.

This doesn't seem like a troublesome Novice issue. But before posting a patch, please review all the comments in #8 and #10. There's been a couple patches now that have very little value and haven't done really anything. Except give the possibility of commit credit. Please be careful to review the feedback and list what you are providing in the patch and what is still outstanding.

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -13,7 +13,13 @@
   + * Available configuration keys:
   

   There's no examples as requested in #8.

2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -13,7 +13,13 @@
   + * - cache_counts: (optional)(Integer) If set cache the source count.
   + * - skip_count: (optional)(Integer) If set not attempt to count the source.
   + * - track_changes: (optional)(Integer) If set track changes to incoming data.
   

   There should be a space between )(

   This isn't any more specific yet either.

3. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -13,7 +13,13 @@
   + * - high_water_property: Array of names used for high-water mark.
   

   Missing (optional). Missing better description too.

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -13,7 +13,13 @@
+ * - high_water_property: Array of names used for high-water mark.

high_water_property is not an "array of names" - it is an array containing a single property name, an an optional table alias.

Fixed #14 and #15. Working on example as mentioned in #8 and update it once completed.

Fixed 80 character length issue.

@shashikant_chauhan in your patch there are more than 80 characters in a line.

@pk188, You don't have to count the characters in patch file instead you should count in file after applying the patch.

See https://www.drupal.org/docs/develop/standards/coding-standards#linelength for more information.

Assigning for review

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -13,7 +13,14 @@
+ * - cache_counts: (optional) (Integer) If set cache the source count.
+ * - skip_count: (optional) (Integer) If set not attempt to count the source.
+ * - track_changes: (optional) (Integer) If set track changes to incoming data.
+ * - high_water_property: It is an array of single value (optional table alias)
+ *   which have the high_water_property corresponding to it.

This documentation doesn't seem very helpful to me and the formulation could be improved.

Those configuration keys are associated with the class properties, which are already documented, and should be coherent. For instance, cache_counts, skip_count & track_changes are said to be boolean on the properties, but integers here.

Also, even if this is an abstract class, we should and can provide some examples, something like:

@code
source:
  plugin: some_source_plugin_name
  cache_counts: true
  ...
@endcode

It should also be mentioned that the high_water_property cannot be used with the track_changes property, as said on line 179:
throw new MigrateException('You should either use a highwater mark or track changes not both. They are both designed to solve the same problem');
So this should absolutely be included in the documentation and we should have code examples for both possibilities, explaining their differences.

Unrelated failure.

I don't think this should be tagged Novice since it requires an good understanding of the migration system.
Back to Needs work.

Removed instances of "(boolean)".
Marked high_water_property as optional.
Moved and edited the high_water_property sentence.

Added explanation to the examples.

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -13,7 +13,44 @@
   + * - cache_counts: (optional) If set cache the source count.
   ...
   + * - track_changes: (optional) If set track changes to incoming data.
   

   Nit: comma between set, and the rest of the sentence.

2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -13,7 +13,44 @@
   + * - skip_count: (optional) If set not attempt to count the source.
   

   Nit: Add comma. And if set, /do/ not attempt to count.

3. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -28,22 +30,28 @@
   + * Changes to incoming data are watched (because track_changes is true) which
   

   comma or period should be inserted before word, "which".

4. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
   @@ -28,22 +30,28 @@
   + * In this example skip_count is true which means count() will not attempt to
   

   Comma after, "In this example". And another comma after word "true".

Changes made according to comment #33

Made change suggested by @quietone in #35.

All feedback is now addressed.

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -13,7 +13,44 @@
+ * - high_water_property: (optional) It is an array of single value (optional
+ *   table alias), which have the high_water_property corresponding to it.

I'm doing an implementation with high_water_property right now. And 1) it is complicated, 2) this little two line comment isn't helping.

From getHighWatermark()

The high water mark defines a timestamp stating the time the import was last run. If the mark is set, only content with a higher timestamp will be imported.

From getHighWaterField()
It looks like an array keyed by 'name' and (optionally) 'alias'.

The values of name and alias are concatenated with a period and used in SqlBase for a db query condition.

So, let's reword this:
high_water_property: (optional) It is an array of key/values for a column name and (optional) alias in the source database that defines a timestamp stating the time the import was last run. If the mark is set, only content with a higher timestamp will be imported.

And it looks like our example should add the alias part to the docs.

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -13,7 +13,44 @@
+ *   high_water_property:
+ *     name: something

Should be:

high_water_property:
  name: changed
  alias: n

The above will get converted into n.changed when running the database query.

Tagging.

Addressing my own feedback.

Beautiful.

Very minor punctuation correction.

I think this can be safely returned to RTBC!

Thanks all for making sure this looks good! Committed to 8.5.x and cherry-picked to 8.4.x.