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.
The user 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
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. |
Comment | File | Size | Author |
---|---|---|---|
#32 | clean_up_user_module-2385211-32.patch | 20.82 KB | subhojit777 |
#31 | clean_up_user_module-2385211-31.patch | 20.2 KB | cilefen |
#26 | clean_up_user_module-2385211-26.patch | 20.1 KB | tadityar |
#23 | interdiff-21-23.txt | 777 bytes | tadityar |
#23 | clean_up_user_module-2385211-23.patch | 19.98 KB | tadityar |
Comments
Comment #1
hussainwebInitial attempt. I removed a couple of properties in
UserPictureTest
which were not being used anywhere. I know it may not strictly be in the scope of the issue, but the properties would have to be renamed anyway, and since they are not used, it's better they are removed.Comment #3
hussainwebThat was quick. Rerolling.
Comment #5
hussainwebIt seems I missed a typo.
Comment #6
tibbsa CreditAttribution: tibbsa commentedSome more work done on this; let's see what the testbot thinks.
Comment #7
Mile23The patch in #6 refactors class-level properties from under_score to camelCase as the coding standards demand.
I know this because I applied the patch, ran my own coding standards review with netbeansdrupalcomposed, and poked through all the test classes to find camel case and underscore violations.
Comment #8
alexpottThis does not need to be a class property - could just be $admin_user since it is only used in a single method and not created by setUp().
These changes look unnecessary to me.
Let's make this like all the others and put the views on one line.
Comment #9
tadityar CreditAttribution: tadityar commentedAltered the change as #8 suggested
Comment #10
subhojit777Looks like its still missing some correction
Comment #11
tibbsa CreditAttribution: tibbsa commentedThe first two are fixed as part of #2380023: Clean-up Comment module Test members - ensure property definition and use of camelCase naming convention and are outside the scope of this issue.
With respect to the last file, however, you are correct. The patch at #8 reverted the addition of the $adminUser class property, but did not revert the doc block, and did not change the instances of
$this->admin_user
that appear intestUserCancelUid1()
.Steps needed to fix:
testUserCancelUid1()
to$this->admin_user
to simply$admin_user
. (This is consistent with how it is done in other testing functions there.)Comment #12
subhojit777@tibbsa okay I will consider them. But I thought these changes were missing in the last patch.
Comment #13
tibbsa CreditAttribution: tibbsa commentedLocal variables like $web_user, when used only within a particular function, do not follow the camelCase naming convention: they should use the underscore_separating convention. See Naming Convention: Functions and Variables in the coding standards.
Class-level properties (variables that are declared within a class but not within a particular function) are to use the camelCasing convention instead. See Naming Conventions (point #2) in the 'object-oriented' coding standards.
I have no idea why the decision was made to do this differently, except that it provides some hint in object-oriented code as to whether a variable (at least a variable with more than one word in its name) is local to a function or not. The fact that this remains ambiguous for one-word variable names would seem to undermine that logic, but I digress. The coding standards say what the coding standards say. :)
@alexpott's point about $adminUser was that it was only ever used within
testUserCancelUid1()
. As a result, there seemed to be no point in making it a class-level property, since no other function ever refers to $this->adminUser anyway, and as such it can simply remain a local variable ($admin_user).Comment #14
subhojit777@tibbsa thanks for the suggestions. I have changed the code as per your suggestion in #11
Comment #17
Mile23The patch doesn't apply, according to the testbot.
Comment #18
hussainwebIt was a straight reroll.
Comment #19
hussainwebComment #20
subhojit777Patch does not covers the changes asked by alexpott in #8.2
Comment #21
tadityar CreditAttribution: tadityar commentedNew patch based on all suggestions.
Comment #23
tadityar CreditAttribution: tadityar commentedhmm, trying once more.
Comment #25
subhojit777@tadityar test them locally at first. #21, #23 have same failing tests.
Comment #26
tadityar CreditAttribution: tadityar commentedFixed for sure now.. at least in my localhost
the mistake was from drupalLogin($this->admin_user)
Comment #27
Mile23Needs reroll.
Sigh.
Comment #28
Mile23Comment #29
cilefen CreditAttribution: cilefen commentedComment #30
Mile23Great, thanks @cilefen.
A couple things I suppose are artifacts of reroll after reroll...
Unused property (as far as I can tell).
This one is declared and then replaced with a local variable:
Comment #31
cilefen CreditAttribution: cilefen commented@Mile23 - thanks, I also found anonymous in UserEntityCallbacksTest.
Comment #32
subhojit777egrep -rl '\$this\->[a-z]+_[a-z_]+' core/modules/user/src/Tests/
is clean, and code looks alright. I was about to RTBC and then I saw some faults in coding standards (comment less than 80 characters).Comment #33
Mile23I'll set it to RTBC. :-)
Patch applies, fixes camel case/underscore problem for the tests. Explicitly adds undeclared properties. Also fixes some documentation errors.
Comment #34
alexpottCommitted 353a160 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to issue summary.