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 consistent with the current Drupal coding and documentation standards.
Proposed resolution
- Remove the leading underscore from variable names.
- Use consistent cases for properties.
- Change
private $_*
properties toprotected $*
Comment | File | Size | Author |
---|---|---|---|
#33 | 2935190-32.patch | 36.4 KB | longwave |
#28 | 2935190-28-D8.patch | 36.41 KB | mohit1604 |
#28 | interdiff.txt | 1.01 KB | mohit1604 |
#24 | 2935190-24-D8.patch | 36.43 KB | mohit1604 |
#24 | interdiff.txt | 912 bytes | mohit1604 |
Comments
Comment #2
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComment #4
John Cook CreditAttribution: John Cook at Creode commentedI've checked the patch. The only things that are being changed are the properties. The initial underscore ('_') is being removed and the scope is being increased from
private
toprotected
.After searching the codebase for rouge underscores I found the following:
This means that there are some instances in the comments that should be changed to match the updated code.
Setting back to Needs work for now. I've added the 'Novice' tag for updating the comments.
Comment #5
stefanvojkic CreditAttribution: stefanvojkic as a volunteer commentedComment #6
mikispeed CreditAttribution: mikispeed commentedComment #7
aryashreep CreditAttribution: aryashreep as a volunteer commentedComment #8
aryashreep CreditAttribution: aryashreep as a volunteer commentedMade the changes as per #4 in PoItem.php file. Now the varibales are without underscore.
[ Asia ]
[ India ]
Comment #9
aryashreep CreditAttribution: aryashreep as a volunteer commentedComment #10
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commented@aryashreep, thanks for picking this up. Can you also provide an iterdiff? This greatly helps to review the changes you made without re-evaluating the whole patch. See: https://www.drupal.org/documentation/git/interdiff
Comment #11
welly CreditAttribution: welly at Fat Beehive commentedLooks good to me
Comment #12
Gábor HojtsyThe patch from Sutharsan affects a lot more files in this component. Is that not needed anymore? As per the issue scope this would cover the whole component no?
Comment #13
Lan Anh CreditAttribution: Lan Anh as a volunteer commentedplease review,
I took the patch from #2
and replaced the protected properties with private
Comment #14
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedThe patch looks good, but we should not change the visibility of this method.
Comment #15
Lan Anh CreditAttribution: Lan Anh as a volunteer commentedSry, the guy next to me is distracting me with his ICQ-Login attemps.
Comment #16
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #17
aryashreep CreditAttribution: aryashreep as a volunteer commentedComment #18
mikispeed CreditAttribution: mikispeed commentedI still do not see #4 addressed:
Setting back to Needs work.
Remove `_` from variable name.
Remove `_` from variable name.
Comment #19
aryashreep CreditAttribution: aryashreep as a volunteer commentedFixed the #4 issue, please review.
Comment #20
mikispeed CreditAttribution: mikispeed at Develomon commentedThis now looks good, results:
thanks
Comment #21
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedThanks so far.
Shameless plug: ;) There is more cleanup work on the Gettext component that needs review. See #1861442: [meta] Review of Drupal\Component\GetText files
Comment #22
alexpottWhy not protected?
I'm not sure we should be changing function scope.
This used to be dynamically declared right? So it was public. My first thought was that tt should stay public but with an @todo to make protected in D9. But actually it was prefixed by an underscore so it was meant to be private so I think this is okay.
Comment #23
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedComment #24
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedDid changes as per #22, please review it :)
Comment #25
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedThe interdiff does cover #22.
Checked that no (other) function scope changes in this patch.
Checked that only "private $_*" properties were changed to "protected $*"
Updated issue summary with the latter.
Comment #26
alexpottThe change from
return;
toreturn NULL
is out-of-scope.Why change fwrite to fputs? fputs is an alias of fwrite but I didn't know one was preferred over the other. I think this is an out-of-scope change.
Comment #27
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedComment #28
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commented@alexpott, Thanks for the review, did changes as per #26.Providing interdiff of patch #24 and #28.
Comment #29
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedThe changes in the interdiff does cover the comments in #26.
Checked the code of the patch, I only find the type of changes listed in the issue summary.
Checked Gettext code with applied patch: No more
$_
or->_
in code; No more private properties.Comment #30
alexpottI think we should postpone this on #2935193: Fix broken exceptions in Gettext component - since that is a bugfix that should be backported to 8.5.x
Comment #31
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedGettext issue is now committed to 6.x and 5.x. Back to needs work to re-roll the patch.
Comment #32
longwaveReroll, no interdiff.
Comment #33
longwavePatch did not attach, trying again.
Comment #34
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedI've checked the differences between the #28 and #33 patches and found only expected differences. No new changes introduced by #33.
Back to RTBC.
Comment #35
alexpottAdding review credit.
Comment #36
alexpottCommitted d9f9378 and pushed to 8.6.x. Thanks!