The source_provider and minimum_schema_version are two important but optional properties for source plugins and they need documentation so people know how to use them. This also means refactoring a bit the way source requirements are handled.

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Add properties Yes add source_provider and minimum_schema_version with TRUE defaults to the MigrateSource annotation class Yes
Document each property Yes Document as generically as possible since these are not Drupal specific. Yes

 

Original Description
It seems that source_provider was left out of being documented in any annotations. Currently it provides requirement checking functionality for source modules in DrupalSqlBase. The question this raises, is this intended to be functionality of the migrate module or is it specific to Drupal in the migrate_drupal module?

CommentFileSizeAuthor
#15 2559345-15.patch1.34 KBphenaproxima
#13 interdiff.txt1.12 KBlostkangaroo
#13 document-2559345-13.patch774 byteslostkangaroo
#10 document-2559345-10.patch836 byteslostkangaroo
#7 document_source_provider-2559345-6.patch840 byteslostkangaroo

Comments

lostkangaroo created an issue. See original summary.

lostkangaroo’s picture

lostkangaroo commented

Discussion in IRC is leaning towards adding the annotation in the most generic form into MigrateSource as an optional property. This still allows for overriding by migrate_drupal if needed.

Another property was identified "minimum_schema_version" that should be included in this. This would be renamed to minimum_version to make it more generic.

benjy’s picture

benjy commented

Yeah, it's interesting that these fields are on the plugin definition because these keys are only used in DrupalSqlBase and therefore only sources that extend that.

So I think the proposal is to:

a) Document the properties on the annotation.
b) Implement RequirementsInterface from SourcePluginBase?
c) Provide a default implementation on SourcePluginBase that returns TRUE and then let DrupalSqlBase override it.

Alternatively, we could move the source_provider stuff out of the source annotation and into an interface/method that can be implemented by sources that require such requirement checks.

lostkangaroo’s picture

lostkangaroo commented
Title: Document source_provider annotation property » Document source_provider and minimum_schema_version annotation properties
lostkangaroo’s picture

lostkangaroo commented
Issue summary: View changes
Issue tags: +Barcelona2015, +Novice, +Needs documentation

I like the second option but don't see the need for such an interface when the annotation itself would be sufficient and already is being used everywhere.

a) yes
b) is pretty much what I was thinking.
c) I am thinking of leaving it as an abstract method to be implemented by source plugins. It might become a bit of an annoyance but most migrations should provide its own base that would handle source dependencies.

neclimdul’s picture

neclimdul
He/Him
commented

b) c) I'm confused about how these help clarify things. Returning true won't actually help document the property will it?

lostkangaroo’s picture

lostkangaroo commented
StatusFileSize
new document_source_provider-2559345-6.patch840 bytes

Just adding the annotation portion until the b) c) parts can be addressed.

lostkangaroo’s picture

lostkangaroo commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: -Barcelona2015, -Novice

Status: Needs review » Needs work

The last submitted patch, 7: document_source_provider-2559345-6.patch, failed testing.

lostkangaroo’s picture

lostkangaroo commented
StatusFileSize
new document-2559345-10.patch836 bytes

Just noticed a small mistake in the patch left over from some experimentation earlier that didn't match the docblock text. No interdiff since this is just too small to need one.

benjy’s picture

benjy commented

Regarding #6, as mentioned on IRC, it's for consistency, so that the only implementation isn't in a Drupal specific source. It will be needed once the base class implements RequirementsInterface

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented 
+++ b/core/modules/migrate/src/Annotation/MigrateSource.php
@@ -45,4 +45,20 @@ class MigrateSource extends Plugin {
+  /**
+   * An array of source dependencies used to determine source data availability.
+   * This is up to the source plugin to determine how this is accomplished.
+   *
+   * @var mixed
+   */
+  public $source_provider = [];

I talked to @lostkangaroo on IRC and I think that source_provider should not be any specific type. I think its type, meaning, and applicability to a given set of source data should be for the source plugin to decide, in order to keep this annotation property as generic as possible. Same goes for minimum_version.

lostkangaroo’s picture

lostkangaroo commented
Status: Needs work » Needs review
StatusFileSize
new document-2559345-13.patch774 bytes
new interdiff.txt1.12 KB

Removed the default empty arrays and modified the doc text to accommodate. I think we should spin off #3 b)c) into a separate issue at this point.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

I like it, but I'd like to get @benjy and @mikeryan's opinions before RTBCing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
StatusFileSize
new 2559345-15.patch1.34 KB

Added more detail to the comments after discussing with @benjy on IRC.

benjy’s picture

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

Great, this looks good now. Thanks!

lostkangaroo’s picture

lostkangaroo commented

+1 RTBC lets do this

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs documentation +rc eligible

This is just docs so is totally rc eligible.

Cleaned up the first sentences of both on commit so they fit on one line, per coding standards https://www.drupal.org/coding-standards/docs.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed d3c5e54 on 8.0.x 
    Issue #2559345 by lostkangaroo, phenaproxima, benjy, neclimdul: Document...

Status: Fixed » Closed (fixed)

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