API documentation of DrupalSqlBase source plugin needs to be improved.

CommentFileSizeAuthor
#19 interdiff-2921033-17-19.txt745 bytesmasipila
#19 2921033-19.patch2.41 KBmasipila
#17 interdiff-14-17.txt746 bytesjofitz
#17 2921033-17.patch2.42 KBjofitz
#14 interdiff-10-14.txt980 bytesjofitz
#14 2921033-14.patch2.43 KBjofitz
#10 interdiff-6-10.txt1.41 KBjofitz
#10 2921033-10.patch2.43 KBjofitz
#6 interdiff-2921033-2-6.txt1.08 KBmasipila
#6 2921033-6.patch2.41 KBmasipila
#2 2921033-2.patch2.42 KBmasipila
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

masipila created an issue. See original summary.

masipila’s picture

masipila CreditAttribution: masipila as a volunteer commented
FileSize
2921033-2.patch2.42 KB

Updated class documentation and improved some method documentation to comply with coding standards (third person singular tense verb).

masipila’s picture

masipila CreditAttribution: masipila as a volunteer commented
Status: Active » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Postponed

A hole in one! This looks fantastic to me.

However, I'd like to postpone it on #2862671: Add documentation to SqlBase source plugin, since that contains important documentation that is very relevant to DrupalSqlBase. Once that one lands, I think this can go direct to RTBC.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Related issues: +#679890: Using "Please" in the interface 
+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -13,10 +13,17 @@

@@ -13,10 +13,17 @@
+ * For available configuration keys, please refer to the parent classes

Just a note that our style guidelines indicate that the word "please" should be omitted. Reference: https://www.drupal.org/node/604342 (See also: #679890: Using "Please" in the interface).

masipila’s picture

masipila CreditAttribution: masipila as a volunteer commented
FileSize
2921033-6.patch2.41 KB
interdiff-2921033-2-6.txt1.08 KB

Removed the 'please' and added the period to the list items.

I'll leave this to 'Postponed' status. We can trigger the testbot once #2862671: Add documentation to SqlBase source plugin lands.

Markus

masipila’s picture

masipila CreditAttribution: masipila as a volunteer commented
Status: Postponed » Needs review
Issue tags: -drupal.org documentation +API Documentation

#2862671: Add documentation to SqlBase source plugin is now RTBC. The block for this was only a soft block, setting this to NR now so that this could be committed at the same go.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work

Minor stuff. As always.

  1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -13,10 +13,17 @@
+ * when writing source plugins that use a Drupal database a source, for example:

    Should be "...use a Drupal database as a source..." (missing 'as') 

  2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -13,10 +13,17 @@
+ * - For full list, please refer to the methods of this class.

    This should be its own paragraph, not a bullet point. 

  3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -13,10 +13,17 @@
+ * For available configuration keys, refer to the parent classes

    Need to have a colon after "classes". Also, maybe the SqlBase and SourcePluginBase references should have @see annotations?

anpolimus’s picture

anpolimus
Primary language Ukrainian
Location Kyiv
CreditAttribution: anpolimus as a volunteer and at Drupal Ukraine Community commented
Issue tags: +CSKyiv18
jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
2921033-10.patch2.43 KB
interdiff-6-10.txt1.41 KB

Addressed nits from #8.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

masipila’s picture

masipila CreditAttribution: masipila as a volunteer commented

Many thanks Jo for wrapping up this and the SqlBase source plugin issue! Thumbs up!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -13,10 +13,19 @@
+ * - Check if given module exists in the source database.

    Nit: s/if given/if the given/ 

  2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -13,10 +13,19 @@
+ * For full list, please refer to the methods of this class.

    Nit: s/For full/For a full/

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
2921033-14.patch2.43 KB
interdiff-10-14.txt980 bytes

Addressed nits from #13.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Thanks, Jo. Back to RTBC!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -13,10 +13,19 @@
+ * This class provides general purpose helper methods that are commonly needed

Can this just be 'Provides...'?

Otherwise looks good.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
2921033-17.patch2.42 KB
interdiff-14-17.txt746 bytes

Made the change requested in #16.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community
masipila’s picture

masipila CreditAttribution: masipila as a volunteer commented
Status: Reviewed & tested by the community » Needs review
FileSize
2921033-19.patch2.41 KB
interdiff-2921033-17-19.txt745 bytes

The word 'please' slipped back in to the patch in #10. xjm asked to remove the 'please' in #5. The attached patch removes it.

Hopefully this is the last patch to this issue.. :)

Markus

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Great, everything addressed.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -52,7 +61,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
+   * Retrieves all system data information from the source Drupal database.

Personally, I really like the consistent use of 'the source Drupal database' throughout. It leaves no ambiguity at all about the source.

  • catch committed aee6bea on 8.5.x 
    Issue #2921033 by Jo Fitzgerald, masipila, phenaproxima, xjm, Wim Leers...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed f24bbc2 on 8.4.x 
    Issue #2921033 by Jo Fitzgerald, masipila, phenaproxima, xjm, Wim Leers...

Status: Fixed » Closed (fixed)

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