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
| 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 |
|---|---|---|---|
| #8 | clean_up_telephone-2387965-8.patch | 1011 bytes | tadityar |
| #2 | clean_up_telephone-2387965-2.patch | 1009 bytes | hussainweb |
Comments
Comment #1
hussainwebComment #2
hussainwebInitial attempt. I have removed properties that are not used anywhere.
Comment #3
hussainwebMissed something in IS after cloning issue. :)
Comment #4
tibbsa commentedMy only nit-picky comment is about the wording of the variable description:
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:
Subject to that minor tweak, this should be ready to go.
Comment #5
mile23I verified that
$fieldisn't used anywhere else.Changing
article_creatortowebUsermakes as much sense as just changing the property toarticleCreator, since that property isn't used outside ofsetUp(). 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. :-)
Comment #6
tibbsa commentedThere 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.
Comment #7
mile23The only change I'm looking for is the privileges/permissions change and I'll RTBC. :-)
Comment #8
tadityar commentedUpdated the permission part.
Comment #9
mile23phpcs 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.
Comment #10
alexpottCommitted a8e5ecc and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation for to the issue summary.