The telephone 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.

Comments

hussainweb’s picture

Status: Active » Needs review
StatusFileSize
new1009 bytes

Initial attempt. I have removed properties that are not used anywhere.

hussainweb’s picture

Issue summary: View changes

Missed something in IS after cloning issue. :)

tibbsa’s picture

Status: Needs review » Needs work

My only nit-picky comment is about the wording of the variable description:

 /**
   * User with privileges to create articles.
   *
   * @var \Drupal\user\UserInterface
   */

First, Drupal generally refers to "permission" or "permissions" (though typically "permission" will do unless you have a user with permission to do a whole host of unrelated things), rather than "privileges". Second, Drupal's documentation standards call for "complete sentences". While the below isn't precisely a complete sentence either, it works if you think of it as "This is...._____" and seems to be consistent with what is done elsewhere:

 /**
   * A user with permission to create articles.
   *
   * @var \Drupal\user\UserInterface
   */

Subject to that minor tweak, this should be ready to go.

mile23’s picture

I verified that $field isn't used anywhere else.

Changing article_creator to webUser makes as much sense as just changing the property to articleCreator, since that property isn't used outside of setUp(). Six of one, half a dozen of another.

The @var comment block is outside of scope, but since it's there, let's change 'priviledge' to 'permission,' get a green test pass, and then we can RTBC. :-)

tibbsa’s picture

There are many, many tests that make these $webUser / $adminUser / $otherVar "class properties" in setUp() (for example) and then never refer to them again. I've posed the question of, 'Why?', and so far nobody has given me any indication that there is a reason. I've hypothesized that it might help in the event of a catastrophic test failure. For example, if that failure might be related to the user accounts, having $this->webUser to inspect might be helpful in some way. But it seems to me that this would be an edge case that could be temporarily implemented if there were some reason to do so, and not a convention that ought to be utilized "just because". That said, it is likely out of scope to get into removing class properties that are pointlessly made class properties rather than ordinary local variables -- if they ought to even be that. I think $this->drupalLogin($this->drupalCreateUser(array('create article content', 'edit own article content'))); would work just as well?

As to the question of the docblock being out of scope, I think not. @webchick seemed to be of the view that if we're touching these things we ought to try to fix them as much as we can, so as not to introduce or leave code there that we'll have to revisit in the future anyway. That doesn't mean adding docblocks to everything that is missing a docblock, but I think it can mean adding docblocks to anything that is otherwise "touched".

I based this view on the fact that the "contributor task" documentation for improving patch standards indicates that "Every function and class added or changed by the patch should have a documentation header (docblock)," which would seem to suggest that if you alter a function that is missing a docblock altogether it ought to get one at the same time. Applying that logic to the class properties seems sensible as a result.

mile23’s picture

The only change I'm looking for is the privileges/permissions change and I'll RTBC. :-)

tadityar’s picture

Status: Needs work » Needs review
StatusFileSize
new1011 bytes

Updated the permission part.

mile23’s picture

Status: Needs review » Reviewed & tested by the community

phpcs says the patch in #8 changes underscore to camel case. It also removes unused variables and keeps the test from creating an extraneous user. And then it adds some happy docblock documentation.

So therefore: RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a8e5ecc and pushed to 8.0.x. Thanks!

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

  • alexpott committed a8e5ecc on 8.0.x
    Issue #2387965 by hussainweb, tadityar: Clean-up telephone module test...

Status: Fixed » Closed (fixed)

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