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 to protected $*
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sutharsan created an issue. See original summary.

Sutharsan’s picture

Assigned: Sutharsan » Unassigned
Status: Active » Needs review
FileSize
36.75 KB

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

John Cook’s picture

Status: Needs review » Needs work
Issue tags: +Novice

I'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 to protected.

After searching the codebase for rouge underscores I found the following:

core/lib/Drupal/Component/Gettext> grep -nr '$_' *
PoItem.php:32:   * @see $_plural
PoItem.php:54:   * @see $_plural
core/lib/Drupal/Component/Gettext> grep -nr '\->_' *
core/lib/Drupal/Component/Gettext>

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.

stefanvojkic’s picture

Assigned: Unassigned » stefanvojkic
mikispeed’s picture

Issue tags: +SprintWeekend2018
aryashreep’s picture

Assigned: stefanvojkic » aryashreep
aryashreep’s picture

Made the changes as per #4 in PoItem.php file. Now the varibales are without underscore.

[ Asia ]

    [ India ]

    1. [ Bangalore ] - [ January 27 ] - Details
aryashreep’s picture

Status: Needs work » Needs review
Sutharsan’s picture

@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

welly’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

The 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?

Lan Anh’s picture

please review,
I took the patch from #2
and replaced the protected properties with private

webflo’s picture

+++ b/core/lib/Drupal/Component/Gettext/PoStreamReader.php
@@ -577,7 +577,7 @@ public function parseQuoted($string) {
-  private function shortenComments($comment) {
+  protected function shortenComments($comment) {

The patch looks good, but we should not change the visibility of this method.

Lan Anh’s picture

Sry, the guy next to me is distracting me with his ICQ-Login attemps.

webflo’s picture

Status: Needs work » Reviewed & tested by the community
aryashreep’s picture

Assigned: aryashreep » Unassigned
mikispeed’s picture

Status: Reviewed & tested by the community » Needs work

I still do not see #4 addressed:

/core/lib/Drupal/Component/Gettext (8.6.x)$ grep -nr '$_' *
PoItem.php:32:   * @see $_plural
PoItem.php:54:   * @see $_plural

Setting back to Needs work.

  1. +++ b/core/lib/Drupal/Component/Gettext/PoItem.php
    @@ -31,21 +31,21 @@ class PoItem {
        * @see $_plural
    

    Remove `_` from variable name.

  2. +++ b/core/lib/Drupal/Component/Gettext/PoItem.php
    @@ -53,7 +53,7 @@ class PoItem {
        * @see $_plural
    

    Remove `_` from variable name.

aryashreep’s picture

Status: Needs work » Needs review
FileSize
588 bytes
36.76 KB

Fixed the #4 issue, please review.

mikispeed’s picture

Status: Needs review » Reviewed & tested by the community

This now looks good, results:

core/lib/Drupal/Component/Gettext (8.6.x)$ grep -nr '$_' *
core/lib/Drupal/Component/Gettext (8.6.x)$ grep -nr '\->_' *

thanks

Sutharsan’s picture

Thanks 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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Component/Gettext/PoMemoryWriter.php
    @@ -12,13 +12,13 @@ class PoMemoryWriter implements PoWriterInterface {
    +  private $items;
    

    Why not protected?

  2. +++ b/core/lib/Drupal/Component/Gettext/PoStreamReader.php
    @@ -577,7 +577,7 @@ public function parseQuoted($string) {
    -  private function shortenComments($comment) {
    +  protected function shortenComments($comment) {
    

    I'm not sure we should be changing function scope.

  3. +++ b/core/lib/Drupal/Component/Gettext/PoStreamWriter.php
    @@ -12,21 +12,28 @@ class PoStreamWriter implements PoWriterInterface, PoStreamInterface {
    +  protected $langcode;
    

    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.

mohit1604’s picture

Assigned: Unassigned » mohit1604
mohit1604’s picture

Assigned: mohit1604 » Unassigned
Status: Needs work » Needs review
FileSize
912 bytes
36.43 KB

Did changes as per #22, please review it :)

Sutharsan’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

The 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Component/Gettext/PoStreamReader.php
    @@ -273,24 +273,24 @@ private function readLine() {
    +          return NULL;
    

    The change from return; to return NULL is out-of-scope.

  2. +++ b/core/lib/Drupal/Component/Gettext/PoStreamWriter.php
    @@ -104,7 +111,7 @@ public function close() {
    -    $result = fwrite($this->_fd, $data);
    +    $result = fputs($this->fd, $data);
    

    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.

mohit1604’s picture

Assigned: Unassigned » mohit1604
mohit1604’s picture

Assigned: mohit1604 » Unassigned
Status: Needs work » Needs review
FileSize
1.01 KB
36.41 KB

@alexpott, Thanks for the review, did changes as per #26.Providing interdiff of patch #24 and #28.

Sutharsan’s picture

Status: Needs review » Reviewed & tested by the community

The 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.

alexpott’s picture

Status: Reviewed & tested by the community » Postponed
Related issues: +#2935193: Fix broken exceptions in Gettext component

I 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

Sutharsan’s picture

Status: Postponed » Needs work

Gettext issue is now committed to 6.x and 5.x. Back to needs work to re-roll the patch.

longwave’s picture

Status: Needs work » Needs review

Reroll, no interdiff.

longwave’s picture

Patch did not attach, trying again.

Sutharsan’s picture

Status: Needs review » Reviewed & tested by the community

I've checked the differences between the #28 and #33 patches and found only expected differences. No new changes introduced by #33.

Back to RTBC.

alexpott’s picture

Adding review credit.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed d9f9378 and pushed to 8.6.x. Thanks!

  • alexpott committed d9f9378 on 8.6.x
    Issue #2935190 by mohit1604, aryashreep, Lan Anh, Sutharsan, longwave,...

Status: Fixed » Closed (fixed)

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