The text module uses Test class members with underscored names. Some examples are big_user, web_user and admin_user, but there could be others. According to our coding conventions, these should be renamed to bigUser, webUser and adminUser. In addition, some properties are undefined but should be.

See the parent issue #1811638: [meta] Clean-up Test members - ensure property definition and use of camelCase naming convention.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because this is a coding standards change.
Issue priority Not critical because coding standard changes are not critical.
Unfrozen changes Unfrozen because it only changes automated tests.
Disruption There is no disruption expected from this sort of change.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

GPrince17’s picture

Status: Active » Needs review
FileSize
2.25 KB
GPrince17’s picture

Status: Needs review » Needs work

The last submitted patch, 1: textcamelCase.patch, failed testing.

pwolanin’s picture

Looks like you missed a usage in TextFieldTest.php line 115

cilefen’s picture

Issue summary: View changes
cilefen’s picture

@GPrince17: See the update to the parent issue.

cilefen’s picture

Issue summary: View changes
cilefen’s picture

Title: Clean-up Text module test members to use camelCase naming convention » Clean-up Text module test members - ensure property definition and use of camelCase naming convention
hussainweb’s picture

Status: Needs work » Needs review
FileSize
835 bytes
3.07 KB

The error was probably due to changing web_user to webUser. I have changed it in the parent class StringFieldTest which is in field module. It is not strictly in the scope of this ticket, but it is a small enough change rather than setting up dependencies to the issue for field module's ticket. We will just need to do a simple reroll depending on which goes in first. There is no issue yet for fixing these issues in the field module, which means we should be fine.

subhojit777’s picture

Status: Needs review » Reviewed & tested by the community

Code looks good. Marking this as RTBC for now.

subhojit777’s picture

Assigned: Unassigned » subhojit777
Status: Reviewed & tested by the community » Needs work

I am very sorry. This issue needs work.

egrep -rl '\$this\->[a-z]+_[a-z_]+' core/modules/text/src/Tests/
core/modules/text/src/Tests/TextFieldTest.php
tibbsa’s picture

That's odd. With this patch applied here, that command does not turn anything up for me.

$ git checkout -b text-test
Switched to a new branch 'text-test'
$ git apply -v ./clean_up_text_module-2380411-9.patch
Checking patch core/modules/field/src/Tests/String/StringFieldTest.php...
Checking patch core/modules/text/src/Tests/TextFieldTest.php...
Applied patch core/modules/field/src/Tests/String/StringFieldTest.php cleanly.
Applied patch core/modules/text/src/Tests/TextFieldTest.php cleanly.
$ egrep -rl '\$this\->[a-z]+_[a-z_]+' core/modules/text/src/Tests/
$

The only thing I could think that might trip this up (though the regexp should keep it from happening) are things like this, but this is valid:

      $entity->{$field_name}->value = str_repeat('x', $i);

Are you sure you tested against a patched copy?

subhojit777’s picture

Will recheck it again today. Was a bit sleepy when I was testing this :-)

tibbsa’s picture

Status: Needs work » Needs review
areke’s picture

Status: Needs review » Reviewed & tested by the community

Marking as RTBC. As tibbsa suggested, I don't think subhojit777 had applied the patch before searching for the text.

$ egrep -rl '\$this\->[a-z]+_[a-z_]+' core/modules/text/src/Tests/
core/modules/text/src/Tests//TextFieldTest.php
$ git apply -v clean_up_text_module-2380411-9.patch
Checking patch core/modules/field/src/Tests/String/StringFieldTest.php...
Checking patch core/modules/text/src/Tests/TextFieldTest.php...
Applied patch core/modules/field/src/Tests/String/StringFieldTest.php cleanly.
Applied patch core/modules/text/src/Tests/TextFieldTest.php cleanly.
$ egrep -rl '\$this\->[a-z]+_[a-z_]+' core/modules/text/src/Tests/
$ 
cilefen’s picture

@areke: Thanks for reviewing. I have not checked this yet. Did you make sure all properties are declared and documented?

cilefen’s picture

Status: Reviewed & tested by the community » Needs work

adminUser and webUser are not documented.

areke’s picture

Status: Needs work » Needs review
FileSize
3.19 KB

Added documentation as per #17.

tibbsa’s picture

Status: Needs review » Needs work

Strictly speaking this should probably have...

* @var \Drupal\user\UserInterface

... for each of the docblocks as well.

areke’s picture

Added the fixes to the errors mentioned in #19.

tibbsa’s picture

Almost. :)

$ git apply -v clean_up_text_module-2380411-20.patch
clean_up_text_module-2380411-20.patch:13: trailing whitespace.
   * @var \Drupal\user\UserInterface
Checking patch core/modules/field/src/Tests/String/StringFieldTest.php...
Checking patch core/modules/text/src/Tests/TextFieldTest.php...
Applied patch core/modules/field/src/Tests/String/StringFieldTest.php cleanly.
Applied patch core/modules/text/src/Tests/TextFieldTest.php cleanly.
warning: 1 line adds whitespace errors.

Coding standards - Indenting and Whitespace

areke’s picture

Status: Needs work » Needs review
FileSize
3.28 KB

Did I get it this time? :P

tibbsa’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 86ced51 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed 86ced51 on 8.0.x
    Issue #2380411 by areke, hussainweb, GPrince17: Clean-up Text module...

Status: Fixed » Closed (fixed)

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