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
The code of the Gettext component is not consistent with the current Drupal coding and documentation standards.
Proposed resolution
Fix the docblock comments of the Gettext related files in Locale module.
Comment | File | Size | Author |
---|---|---|---|
#14 | drupal-locale-docblock-2935199-14.patch | 9.02 KB | Sutharsan |
#14 | interdiff-2935199-13-14.txt | 752 bytes | Sutharsan |
#13 | drupal-locale-docblock-2935199-13.patch | 8.99 KB | Sutharsan |
#7 | drupal-locale-docblock-2935199-7.patch | 8.99 KB | Sutharsan |
#7 | interdiff-2935199-6-7.txt | 2.71 KB | Sutharsan |
Comments
Comment #2
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComment #4
borisson_This patch still applies (but not cleanly) and everything that is being changed in it looks solid.
Comment #5
alexpott@param string $install_profile
I don't really understand how this improves the documentation.
Whilst there are other examples in core this doesn't work in PHPStorm or on https://api.drupal.org/api/drupal/core%21modules%21book%21src%21Form%21B...
Should be a fully qualified class name here.
This looks like a regression too. In fact we probably ought to assert that class implements StringInterface since we rely on it.
Comment #6
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedA re-reroll to begin with...
Comment #7
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComments in #5 addressed:
Yes. Corrected.
I prefer keep my @param and @return short and not to copy (too much of) the function description. But not important enough, so reverted.
Removed from the patch. No other getters and setters in this class use an
@see
referencing to their property (any more?).Agree, removed from the patch.
Agree, but not in this issue. It is about documentation.
Comment #8
borisson_The changes look solid, and I agree that the assertion is something that we should do in a followup. Can you open a followup for that @Sutharsan?
Comment #9
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedFollow-up: #2977079: StringDatabaseStorage::dbStringLoad should check its class argument.
Comment #11
larowlanThis needs a comment, other than that, looks good to me
Comment #13
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedRe-rolled the #7 patch.
Comment #14
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComment added as per #11.
Comment #16
joachim CreditAttribution: joachim commentedLGTM.
Comment #18
xjmThanks for your work on this.
Typically, we shouldn't fix "all the problems" in one module. We should fix the individual kinds of errors, across all of core, and then enable the phpcs rule for that kind of error to avoid the rule regressing. This patch includes a grab-bag of the following kinds of coding standards issues:
@param
@throws
@return
s
to the one-summary verbNow, writing new
@param
and@return
documentation requires humans reading and understanding the code, so it makes sense to add them together in one patch for a given API. However, it makes less sense to mix in rewrites of existing parameter documentation, whitespace fixes, etc. I have to open all the files in this patch to review the changes (in order to verify that the added documentation is correct), so mixing in other sorts of changes and other APIs increases the reviewer burden a bit.For more information, see: https://www.drupal.org/core/scope
Alright, now that I got the obligatory scope discussion out of the way:
This isn't quite a sentence. I'd suggest:
(Note that "cannot" is one word in our content standards.)
Things are rarely just
array
; we can usually be more specific. They are usuallystring[]
,bool[]
,array[]
, etc. Evenmixed[]
gives more info. What kind of array is it?Also "the read options" and "The options to set" don't really explain what "options" are. It needs a reference to wherever the structure of these options is defined.
"String storage objects" isn't adding information beyond the typehint. What string storage objects?
I'm guessing this means the string storage object for the given language and options? Let's say that. Otherwise it's confusing .
What shape of array?
What is a write result report? Where is its structure defined? What references should I look at to know about it?
What are writer options?
Either is correct, I think.
Maybe "The matching source strings"? "The source strings" isn't very helpful.
Thanks!
Also, setting to 8.8.x, since docs fixes are eligible for backport to the production branch.