Posted by GPrince17 on

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.
Files: 
CommentFileSizeAuthor
#22 clean_up_text_module-2380411-22.patch3.28 KBareke
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,031 pass(es).
[ View ]

Comments

GPrince17’s picture

GPrince17 commented
Status:Active» Needs review
StatusFileSize
new textcamelCase.patch2.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,328 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
GPrince17’s picture

GPrince17 commented
Parent issue:» #1811638: [meta] Clean-up Test members - ensure property definition and use of camelCase naming convention

Status:Needs review» Needs work

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

pwolanin’s picture

pwolanin commented

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

cilefen’s picture

cilefen commented
Issue summary:View changes
cilefen’s picture

cilefen commented

@GPrince17: See the update to the parent issue.

cilefen’s picture

cilefen commented
Issue summary:View changes
cilefen’s picture

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

hussainweb commented
Status:Needs work» Needs review
StatusFileSize
new interdiff-1-9.txt835 bytes
new clean_up_text_module-2380411-9.patch3.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,739 pass(es).
[ View ]

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

subhojit777 commented
Status:Needs review» Reviewed & tested by the community

Code looks good. Marking this as RTBC for now.

subhojit777’s picture

subhojit777 commented
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

tibbsa commented

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

subhojit777 commented

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

tibbsa’s picture

tibbsa commented
Status:Needs work» Needs review
areke’s picture

areke commented
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

cilefen commented

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

cilefen’s picture

cilefen commented
Status:Reviewed & tested by the community» Needs work

adminUser and webUser are not documented.

areke’s picture

areke commented
Status:Needs work» Needs review
StatusFileSize
new clean_up_text_module-2380411-18.patch3.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,816 pass(es).
[ View ]

Added documentation as per #17.

tibbsa’s picture

tibbsa commented
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

areke commented
StatusFileSize
new clean_up_text_module-2380411-20.patch3.28 KB

Added the fixes to the errors mentioned in #19.

tibbsa’s picture

tibbsa commented

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

areke commented
Status:Needs work» Needs review
StatusFileSize
new clean_up_text_module-2380411-22.patch3.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,031 pass(es).
[ View ]

Did I get it this time? :P

tibbsa’s picture

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

alexpott commented
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.