Problem/Motivation

In #3189481: [Meta] Add source plugin documentation to the codebase documentation was added for the migrate source plugins. However, a mistake was made which resulted in the api doc being hard to read. This issue is to correct that.

Proposed resolution

Change * For available configuration keys, refer to the parent classes: to
* For available configuration keys, refer to the parent classes. and add a blank line.

Remaining tasks

Review the latest patch, which is documentation.

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
FileSize
30.4 KB
joachim’s picture

Status: Needs review » Reviewed & tested by the community
Matroskeen’s picture

Assigned: Unassigned » Matroskeen
Status: Reviewed & tested by the community » Needs review

I'm sorry to push it back, but I'd like to take some time and review it more carefully.
As a main developer of #3189481: [Meta] Add source plugin documentation to the codebase meta, I understand there might be more issues that we can handle in this task.

I promise to get back with a review later this week.
Thanks :)

Matroskeen’s picture

To give a bit more context, here is what we're trying to fix.

From: https://api.drupal.org/api/drupal/core%21modules%21block%21src%21Plugin%...
To: https://api.drupal.org/api/drupal/core%21modules%21comment%21src%21Plugi...

As you can see, the @see section is a few sections below, so the comment may be confusing because you'll expect parent classes to be right after the semicolon.


I applied the patch and didn't find any occurrences of "refer to the parent classes:" - so far so good!

I think we can use an opportunity of this task and make the following improvements to make our source plugins consistent:

1) Drupal\block\Plugin\migrate\source\Block
"Drupal block source from database." -> "Drupal 6/7 block source from database."

2) Drupal\block\Plugin\migrate\source\d6\BlockTranslation
"Gets i18n block data from source database." -> "Drupal 6 i18n block data from database."

3) Drupal\block\Plugin\migrate\source\d7\BlockTranslation
"Gets i18n block data from source database." -> "Drupal 7 i18n block data from database."

4) Drupal\block_content\Plugin\migrate\source\d6\BoxTranslation
Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation
"Gets" can be removed and "source" can be added.

5) Drupal\language\Plugin\migrate\source\Language
"Drupal language source from database." -> "Drupal 6/7 language source from database."

6) Drupal\migrate_drupal\Plugin\migrate\source\Variable
"Drupal variable source from database." -> "Drupal 6/7 variable source from database."

7) Drupal\migrate_drupal\Plugin\migrate\source\VariableMultiRow
"Multiple variables source from database." -> "Drupal 6/7 multiple variables source from database."

8) Drupal\migrate_drupal\Plugin\migrate\source\d6\VariableTranslation
Drupal\migrate_drupal\Plugin\migrate\source\d7\VariableTranslation
Drupal version is missing

9) Drupal\user\Plugin\migrate\source\d7\UserEntityTranslation
"Provides Drupal 7 user entity translations source plugin." -> "Drupal 7 user entity translations source from database."

10) Drupal\user\Plugin\migrate\source\UserPictureInstance
Drupal\user\Plugin\migrate\source\ProfileField
"Drupal 6/7" can be added.

11) Drupal\taxonomy\Plugin\migrate\source\d7\TermLocalizedTranslation
Drupal\taxonomy\Plugin\migrate\source\d6\VocabularyTranslation
Drupal\taxonomy\Plugin\migrate\source\d6\TermLocalizedTranslation
"source" and "from" can be swapped

12) Drupal\node\Plugin\migrate\source\d7NodeEntityTranslation
"Provides Drupal 7 node entity translations source plugin." -> "Drupal 7 node entity translations source from database."

13) Drupal\menu_link_content\Plugin\migrate\source\d6\MenuLinkTranslation
"Gets Menu link translations from source database." -> "Drupal 6 i18n menu link translations source from database."

14) Drupal\menu_link_content\Plugin\migrate\source\d7\MenuLinkLocalized
"Gets localized menu link translations from source database." -> "Drupal 7 localized menu link translations source from database."

15) Drupal\menu_link_content\Plugin\migrate\source\d7\MenuLinkTranslation
"Gets Menu link translations from source database." -> "Drupal 7 i18n menu link translations source from database."

Matroskeen’s picture

I'm uploading a patch because I already did the research and it was easier for me to apply these changes.

Looking for review and hopefully, it makes the documentation of core source plugins consistent. 😃

P.S. There is a separate task for file source plugins, that's why we don't change it here: #3189876: Add documentation for file source plugins.

Matroskeen’s picture

Assigned: Matroskeen » Unassigned
quietone’s picture

I am in two minds about the additional changes which, if we keep, then the title and issue summary needs to be updated. I like them because I value consistency. I dislike them because sometimes it is removing useful information. Take this change,

+++ b/core/modules/block/src/Plugin/migrate/source/d7/BlockTranslation.php
@@ -5,9 +5,10 @@
- * Gets i18n block data from source database.
+ * Drupal 7 i18n block data from database.

Here we are losing the emphasis that the data is coming from the source database. I think that is a distinction worth keeping. This example is also removing the leading verb, which has been a standard for a while. And while that standard states that it is for 'most functions' I'd rather keep it even if that results in inconsistent summary lines.

Right now, I am leaning to just staying with the patch in #2.

Matroskeen’s picture

Here we are losing the emphasis that the data is coming from the source database.

If we use a source plugin, we assume the data is coming from the source (not destination) database.

And while that standard states that it is for 'most functions' I'd rather keep it even if that results in inconsistent summary lines.

The standards are about the functions, but this is documentation for the source plugin class.

In any case, my main motivation is to follow some patterns, that we defined when working on these issues.
Here is the merge request when we already did similar changes: https://git.drupalcode.org/project/drupal/-/merge_requests/336/diffs

Patch #5 also adds some useful information like Drupal version if the plugin is applicable both for Drupal 6 and Drupal 7.

In my opinion, the price for consistency is not expensive - we remove some obvious statements.
However, I'm fine to partially revert changes made in #5 if I wasn't able to convince you 😉

quietone’s picture

@Matroskeen, your right, I did mix up the class and function standards. And thanks for the link to the early issue, where the pattern was introduced.

All together, I withdraw my objections. But as I have made a patch here I can't RTBC.

Can we have another opinion here?

quietone’s picture

Issue summary: View changes
Issue tags: +Drupal South, +Novice

Tagging for Drupal South. Also tagging novice.

This work here is to read the documentation and make sure it makes sense. Many of the changes are to make the documentation consistent.

quietone’s picture

Issue tags: -Drupal South +DrupalSouth

Fix the tag.

Kristen Pol’s picture

Status: Needs review » Needs work

Thanks for the updates. Here's some feedback:

  1. +++ b/core/modules/language/src/Plugin/migrate/source/d6/LanguageContentSettingsTaxonomyVocabulary.php
    @@ -8,7 +8,8 @@
    + * For available configuration keys, refer to the parent classes/
    

    Typo: / => .

  2. +++ b/core/modules/user/src/Plugin/migrate/source/d6/UserPictureFile.php
    @@ -12,7 +12,8 @@
    + * For additional configuration keys, refer to the parent classes.
    

    To be consistent with all the other comments, IMO additional should change to available.

    More places with this:

    ./core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php: * For additional configuration keys, refer to the parent class:
    ./core/modules/migrate_drupal/src/Plugin/migrate/source/Variable.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/migrate_drupal/src/Plugin/migrate/source/d7/VariableTranslation.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/migrate_drupal/src/Plugin/migrate/source/d6/VariableTranslation.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/migrate_drupal/src/Plugin/migrate/source/d8/Config.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/migrate_drupal/src/Plugin/migrate/source/VariableMultiRow.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/language/src/Plugin/migrate/source/Language.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/user/src/Plugin/migrate/source/d6/UserPictureFile.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/field/src/Plugin/migrate/source/d7/FieldInstance.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/field/src/Plugin/migrate/source/d6/FieldInstance.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/system/src/Plugin/migrate/source/Extension.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/migrate/src/Plugin/migrate/source/EmbeddedDataSource.php: * For additional configuration keys, refer to the parent class:
    ./core/modules/migrate/src/Plugin/migrate/source/EmptySource.php: * For additional configuration keys, refer to the parent class:
    ./core/modules/taxonomy/src/Plugin/migrate/source/d7/TermEntityTranslation.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/taxonomy/src/Plugin/migrate/source/d7/Term.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/taxonomy/src/Plugin/migrate/source/d6/Term.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/taxonomy/src/Plugin/migrate/source/d6/TermNode.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/node/src/Plugin/migrate/source/d7/NodeEntityTranslation.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/node/src/Plugin/migrate/source/d7/Node.php: * For additional configuration keys, refer to the parent classes.
    ./core/modules/node/src/Plugin/migrate/source/d6/Node.php: * For additional configuration keys, refer to the parent classes.
    

    For reference:

    KristenBackupMBP:drupal-9.3.x-dev admin$ grep -r "For additional configuration keys" . | wc
          20     220    2717
    KristenBackupMBP:drupal-9.3.x-dev admin$ grep -r "For available configuration keys" . | wc
          84     924   11511
    
quietone’s picture

Status: Needs work » Needs review
FileSize
746 bytes
38.24 KB

#13.1 Fixed
#13.2. I don't think this should change. When a source plugin itself provide a configuration key or keys the word 'additional' is used and when the source plugin does not the term 'available' is used. It would seem off to say 'additional' keys when there were none on the source plugin.
Hope that makes sense.

Yicks, sorry to do a novice issue.

Amber Himes Matz’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the patch and looked at the MR mentioned in #9 which was a good reference for ensuring consistency in documenting these source plugins. Thank you @Matroskeen for that link and for your feedback/input.

The patch in #14 looks good to me! Thanks @Kristen Pol and @Matroskeen for the reviews and @quietone for the patch.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a re-roll.

Matroskeen’s picture

Assigned: Unassigned » Matroskeen

No worries, will do 👌

Matroskeen’s picture

Assigned: Matroskeen » Unassigned
Status: Needs work » Reviewed & tested by the community

Hmm, it applies cleanly to 9.3.x and 9.2.x. I tried patch #14.
I also triggered a re-test to receive a confirmation from test runner.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
git ac https://www.drupal.org/files/issues/2021-08-24/3225227-14.patch                                                                                           15.6s  Mon 18 Oct 11:21:31 2021
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 39153  100 39153    0     0   130k      0 --:--:-- --:--:-- --:--:--  130k
error: patch failed: core/modules/block/src/Plugin/migrate/source/Block.php:6
error: core/modules/block/src/Plugin/migrate/source/Block.php: patch does not apply
error: patch failed: core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php:7
error: core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php: patch does not apply
error: patch failed: core/modules/migrate_drupal/src/Plugin/migrate/source/VariableMultiRow.php:5
error: core/modules/migrate_drupal/src/Plugin/migrate/source/VariableMultiRow.php: patch does not apply

It really does not. You need to pull - I've committed a load of migrate stuff this morning.

Matroskeen’s picture

Status: Needs work » Needs review
FileSize
38.27 KB

I see. I was using git apply --3way and it resolved the merge conflict automatically.

Matroskeen’s picture

Status: Needs review » Reviewed & tested by the community

Tests are running, so I assume it was applied successfully.

alexpott’s picture

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

Committed and pushed 7c646f0241 to 9.3.x and 1a5f7f751d to 9.2.x. Thanks!

As this is only docs changes backported to 9.2.x

  • alexpott committed 7c646f0 on 9.3.x
    Issue #3225227 by Matroskeen, quietone, Kristen Pol: Fix source plugin...

  • alexpott committed 1a5f7f7 on 9.2.x
    Issue #3225227 by Matroskeen, quietone, Kristen Pol: Fix source plugin...

Status: Fixed » Closed (fixed)

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