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.
This issue is part of #1861442: [meta] Review of Drupal\Component\GetText files that aims to clean up the Gettext component.
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 in the Gettext component files.
Comment | File | Size | Author |
---|---|---|---|
#24 | 2935189-nr-bot.txt | 168 bytes | needs-review-queue-bot |
#14 | drupal-gettext-docblock-2935189-13.patch | 18.57 KB | Sutharsan |
#14 | interdiff-2935189-12-13.txt | 1.01 KB | Sutharsan |
#13 | drupal-gettext-docblock-2935189-12.patch | 18.29 KB | Sutharsan |
#10 | drupal-gettext-docblock-2935189-10.patch | 18.87 KB | Sutharsan |
Comments
Comment #2
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComment #3
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedWrong patch, review this one.
Comment #4
cilefen CreditAttribution: cilefen at Institute for Advanced Study commentedComment #6
mikispeed CreditAttribution: mikispeed at Develomon commentedWell, this is changed in #2935190 and will need work after that one is merged.
This is not clear. I would rephrase to be clear what is default value and what is behavior if -1. For example:
Comment #7
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedAgree that will require a re-roll, but I did this for separation of issues.
Comment 2 addressed.
One missing @param type hint found.
Comment #8
mikispeed CreditAttribution: mikispeed at Develomon commentedThis now looks good, marking as Reviewed.
Comment #9
alexpottWe should open a followup as I strongly suspect that the exceptions thrown in \Drupal\Component\Gettext\PoStreamWriter::close(), ::write() and ::getURI() don't actually work because they are not namespaced properly. They all need to be prefixed with a
\
This should be
* @return null|void
sine the returns are just
return;
Comment #10
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedSee #2935193: Fix broken exceptions in Gettext component
I guess you mean
@return false|void
. Although void as return type was been introduced in PHP 7.1, it makes sense to use it in documentation to express that no value is returned intentionally. PHP 7.0 and below implicitly uses NULL. See https://wiki.php.net/rfc/void_return_type for info.Comment #11
borisson_instead of NULL otherwise, this could say nothing otherwise? We changed the @return, so we should probably update this as well?
Not 100% sure that is something that we should do though. Otherwise this looks solid!
Comment #12
borisson_Setting to needs work based on #11.
Comment #13
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedA re-roll to start with...
Comment #14
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComment #11 addressed.
Comment #24
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.