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

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

StatusFileSize
new2.79 KB

Renaming admin_user to adminUser and removed a property ImageFieldDisplayTest::dumpHeaders as it was not used anywhere.

hussainweb’s picture

StatusFileSize
new1.39 KB
new4.18 KB

Changing property name in derived class in responsive_image.

mile23’s picture

Status: Needs review » Reviewed & tested by the community

Using phpcs I determined that this patch changes all under_score class property names to camelCase for image module's simpletests.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: clean_up_image_module-2396687-2.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review

It seems like this could be a random failure.

mile23’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
@@ -17,8 +17,6 @@
-  protected $dumpHeaders = TRUE;

This 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.

hussainweb’s picture

Status: Needs work » Needs review
StatusFileSize
new659 bytes
new4.36 KB

Reverting 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?

mile23’s picture

Status: Needs review » Needs work

Also, I am just thinking: Isn't it better to set this value in code in setUp() and avoid all confusion?

Yes, it's overloading the existing property, so it should be an assignment to TRUE in setUp() 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.

  /**
   * {@inheritdoc}
   */
  public function setUp() {
    // Use WebTestBase::dumpHeaders to help us identify bugs while writing tests.
    $this->dumpHeaders = TRUE;
  }

The rest is RTBC.

hussainweb’s picture

@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?

hussainweb’s picture

Status: Needs work » Needs review

Created 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.

mile23’s picture

Status: Needs review » Needs work

You'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 dumpHeaders is out of scope here. :-) One more go-round and it's RTBC time.

hussainweb’s picture

Status: Needs work » Needs review
StatusFileSize
new616 bytes
new4.03 KB

Fair enough. I am reverting that change.

mile23’s picture

Status: Needs review » Reviewed & tested by the community

phpcs 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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 9fd145f and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 9fd145f on 8.0.x
    Issue #2396687 by hussainweb: Clean-up image module test members -...

Status: Fixed » Closed (fixed)

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