Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#20 | 3225227-20.patch | 38.27 KB | Matroskeen |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedComment #3
joachim CreditAttribution: joachim as a volunteer commentedComment #4
MatroskeenI'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 :)
Comment #5
MatroskeenTo 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."
Comment #6
MatroskeenI'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.
Comment #7
MatroskeenComment #8
quietone CreditAttribution: quietone as a volunteer commentedI 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,
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.
Comment #9
MatroskeenIf we use a source plugin, we assume the data is coming from the source (not destination) database.
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 😉
Comment #10
quietone CreditAttribution: quietone as a volunteer commented@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?
Comment #11
quietone CreditAttribution: quietone as a volunteer commentedTagging 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.
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedFix the tag.
Comment #13
Kristen PolThanks for the updates. Here's some feedback:
Typo:
/
=>.
To be consistent with all the other comments, IMO
additional
should change toavailable
.More places with this:
For reference:
Comment #14
quietone CreditAttribution: quietone as a volunteer commented#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.
Comment #15
Amber Himes MatzReviewed 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.
Comment #16
alexpottNeeds a re-roll.
Comment #17
MatroskeenNo worries, will do 👌
Comment #18
MatroskeenHmm, 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.
Comment #19
alexpottIt really does not. You need to pull - I've committed a load of migrate stuff this morning.
Comment #20
MatroskeenI see. I was using
git apply --3way
and it resolved the merge conflict automatically.Comment #21
MatroskeenTests are running, so I assume it was applied successfully.
Comment #22
alexpottCommitted 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