The image 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 |
|---|---|---|---|
| #14 | clean_up_image_module-2396687-14.patch | 4.03 KB | hussainweb |
| #14 | interdiff-9-14.txt | 616 bytes | hussainweb |
Comments
Comment #1
hussainwebRenaming admin_user to adminUser and removed a property ImageFieldDisplayTest::dumpHeaders as it was not used anywhere.
Comment #2
hussainwebChanging property name in derived class in responsive_image.
Comment #3
mile23Using phpcs I determined that this patch changes all under_score class property names to camelCase for image module's simpletests.
Comment #5
hussainwebIt seems like this could be a random failure.
Comment #7
mile23Back to RTBC.
Comment #8
alexpottThis is used... see WebTestBase::$dumpHeader. This was added by #2204159: (Responsive) Image styles do not add correct cache tags, nor do they invalidate cache tags upon flushing to help debug this test when things go wrong.
Comment #9
hussainwebReverting that change and adding the comment from
WebTestBase. (Maybe we can just put in an@inheritdoc)Also, I am just thinking: Isn't it better to set this value in code in
setUp()and avoid all confusion?Comment #10
mile23Yes, it's overloading the existing property, so it should be an assignment to
TRUEinsetUp()instead, without a declaration in this class.Here's something to copy paste and modify as you see fit. I'd make a patch but then I can't review.
The rest is RTBC.
Comment #11
hussainweb@Mile23: There are many other places this happens and I think we can create a follow up issue for making the change. We can't change all of them here as it would be out of scope, of course. What do you think?
Comment #12
hussainwebCreated a followup at #2406669: Set dumpHeaders property in setUp method rather than while overriding the property in class definition. Please set back to Needs work if you believe the change should happen here.
Comment #13
mile23You're right... There are 7 of them in different modules including this one. Let's handle it in your follow-up: #2406669: Set dumpHeaders property in setUp method rather than while overriding the property in class definition
In that case the docblock for
dumpHeadersis out of scope here. :-) One more go-round and it's RTBC time.Comment #14
hussainwebFair enough. I am reverting that change.
Comment #15
mile23phpcs says there aren't any class properties with underscore names.
It does complain about a bunch of local variables with camel case names, but that's out of scope of this issue.
And as per @alexpott in #8, we've addressed the issue of $dumpHeaders in #2406669: Set dumpHeaders property in setUp method rather than while overriding the property in class definition
Woot.
Comment #16
alexpottCommitted 9fd145f and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.