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 node 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 |
---|---|---|---|
#21 | clean_up_node-2380773-21.patch | 68.05 KB | hussainweb |
Comments
Comment #1
tibbsa CreditAttribution: tibbsa commentedChanges to the node module.
I also noted a few instances where docblocks were not included for the $webUser variables, and a few other where the $webUser variable was not even declared as a protected member variable prior to use, and included insertions for those. While arguably out of scope for this change, this does not affect functionality in any way, and opening a separate issue for this change seemed overkill.
Comment #2
tibbsa CreditAttribution: tibbsa commentedSending to the Testbot
Comment #3
tibbsa CreditAttribution: tibbsa commentedComment #4
cilefen CreditAttribution: cilefen commentedComment #5
tibbsa CreditAttribution: tibbsa commentedComment #6
cilefen CreditAttribution: cilefen commentedThere are more instances to fix:
Comment #7
tibbsa CreditAttribution: tibbsa commentedViews/FrontPageTest.php includes a reference to $this->root_user, correction of which will have to take place as part of the 'simpletest' module cleanup.
Comment #8
Mile23Just a few things:
NodeAccessFieldTest
, line 80: Missed a camelCase.NodeRevisionsAllTest
: No @var docblocks on the properties.NodeTranslationUITest
:testDisabledBundle()
, line 208: Missed a camelCase.Comment #9
tibbsa CreditAttribution: tibbsa commentedSomehow your line numbers aren't making sense with what I have in the code. Can you clarify what you're looking at that appears to be missing?
NodeRevisionsAllTest: No @var docblocks on the properties.
No properties were otherwise changed or added in this class as part of this issue. We did not necessarily go and document every missing docblock -- but if we were adding new properties, those got documented as we went.
Note the comment in the parent issue: "Do not, as part of this work, add documentation, variables, or otherwise refactor any code that you are otherwise not changing at all to accomplish the above three tasks. If you spot other problems, it is better to create a new issue for these."
NodeAccessFieldTest, line 80: Missed a camelCase.
NodeTranslationUITest: testDisabledBundle(), line 208: Missed a camelCase.
Comment #11
Mile23OK then. Let's re-test since the patch is stale, and then it's off to RTBC land if it's green.
Comment #12
Mile23The patch in #7 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. I found a few that were mentioned in #9, but those are apparently out of scope for this issue.
Comment #13
alexpottThis does not need to be class properties - it is only used with the scope of a single method.
Comment #14
tibbsa CreditAttribution: tibbsa commentedComment #15
Mile23Patch in #14 applies, passes the testbot, changes property names from underscore to camel case, and refactors
$this->nidsVisible
to a local variable as per #13.Comment #17
Mile23Previous fail says: "Setup environment The testbot client is probably malfunctioning."
Running again.
Comment #19
Mile23Comment #21
hussainwebA straight reroll did the trick.
Comment #23
subhojit777Well I am not sure about this:
In
FrontPageTest.php
:In
NodeAccessViewGrantsCacheContextTest.php
:Find for << See here in above code snippets.
I have tried changing
root_user
torootUser
, but the test is failing. Am I missing something obvious?Comment #24
hussainweb@subhojit777: The root_user is setup in WebTestBase and will have to be handled by #2382195: Clean-up simpletest module test members - ensure property definition and use of camelCase naming convention. You can't change it in this patch since it requires changing WebTestBase.php.
Comment #25
subhojit7771. Did
egrep -rl '\$this\->[a-z]+_[a-z_]+' core/modules/node/src/Tests/
, and got this as output:From @hussainweb's comment in #24, I guess there is no problem with that.
2. Test passed in #22
3. Did manual review of the patch in #21, it looks good.
Hence RTBC :)
Comment #27
webchickLooks like Alex has been committing others of these, so joining the club. :P
Committed and pushed to 8.0.x. Thanks!