The statistics 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.
CommentFileSizeAuthor
#1 clean_up_statistics-2387981-1.patch9.09 KBhussainweb
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Status: Active » Needs review
FileSize
clean_up_statistics-2387981-1.patch9.09 KB
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented 
+++ b/core/modules/statistics/src/Tests/StatisticsLoggingTest.php
@@ -27,6 +27,13 @@ class StatisticsLoggingTest extends WebTestBase {
+   * User with permissions to create and edit pages.
+   *
+   * @var \Drupal\user\UserInterface
+   */
+  protected $authUser;

Not sure where $this->authUser comes from. Clearly there's a $this->auth_user, but that should be from a superclass if it's not declared in the original. But it's not in the superclass either.

As it is, the value is only ever referenced in setUp(), and creates the auth user as a side-effect which persists through the test method. I turned it into a local variable just now, and the test passes.

Other than that, looks good.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs review » Needs work
hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented

@Mile23: I couldn't quite understand what you say in #2. I added the property because it was used in the setUp method. You are quite right in saying that it is only used there and we can just change it to a local variable, but this is also true in countless other places in other modules. Should we change to local variables all over?

Actually, I think I would agree to changing such instances to local variables all over the code base (to keep things cleaner and predictable) but I didn't think for them to be in the scope of this issue.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs work » Reviewed & tested by the community

Yes, changing all of them would be outside the scope of this issue, but it's also true that the reference to $this->auth_user in setUp() is weird and maybe buggy. Changing $this->auth_user might *also* be out of scope...

I'd say your change is probably the best of these options though. I'll RTBC and let's find out what a maintainer thinks....

The patch in #1 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.

There are some ambiguities in regard to StatisticsLoggingTest->adminUser and StatisticsTestBase->blockingUser, which exist as their underscore versions in the setUp() method, but aren't defined in the class.

@hussainweb added these as properties, which might be out of scope, overkill, or just the right thing. I vote for it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 2775f31 and pushed to 8.0.x. Thanks!

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

  • alexpott committed 2775f31 on 8.0.x 
    Issue #2387981 by hussainweb: Clean-up statistics module test members -...

Status: Fixed » Closed (fixed)

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