The responsive_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.
CommentFileSizeAuthor
#7 clean_up_responsive_image-2396715-7.patch716 byteshussainweb
#7 interdiff-2-7.txt667 byteshussainweb
#2 clean_up_responsive_image-2396715-2.patch1.02 KBhussainweb
#2 interdiff-1-2.txt1.39 KBhussainweb
#1 clean_up_responsive_image-2396715-1.patch2.41 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
Related issues: +#2396687: Clean-up image module test members - ensure property definition and use of camelCase naming convention
FileSize
clean_up_responsive_image-2396715-1.patch2.41 KB

ResponsiveImageFieldDisplayTest::admin_user actually inherits from ImageFieldTestBase::admin_user and hence I'm just renaming it. The change for the base class is in #2396687: Clean-up image module test members - ensure property definition and use of camelCase naming convention.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
FileSize
interdiff-1-2.txt1.39 KB
clean_up_responsive_image-2396715-2.patch1.02 KB

I modified the patch in #2396687-2: Clean-up image module test members - ensure property definition and use of camelCase naming convention to rename ResponsiveImageFieldDisplayTest::admin_user property and hence removed the change here.

Mile23’s picture

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

Yes, we want the changes to be in the same patch as the changed property declarations.

Patch #2 gets RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/responsive_image/src/Tests/ResponsiveImageAdminUITest.php
@@ -21,7 +21,7 @@ class ResponsiveImageAdminUITest extends WebTestBase {
-  public static $modules = array('responsive_image', 'responsive_image_test_module');
+  public static $modules = ['responsive_image', 'responsive_image_test_module'];

This is out-of-scope

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented

I am happy to revert that line, but I just want to know if this line causes any problem. I know this is out of scope but the change does not affect any functionality at all. It just makes things slightly easier to read.

I know there are concerns for changes during beta stages, but again, since this is not functional and it slowly moves all the code towards the new array syntax, it might be acceptable. Thoughts, please?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

@hussainweb that line obviously does not cause problems but it is out of scope so why change it? Every additional line of out-of-scope change someone has to review takes time away from reviewing other patches.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Status: Needs work » Needs review
FileSize
interdiff-2-7.txt667 bytes
clean_up_responsive_image-2396715-7.patch716 bytes

Reverting that line.

@alexpott: The reason I changed it is that it is slightly more readable and looks better (especially among nested parenthesis). I was not sure to create another issue for this change and thought it would be better to make this change in a small way through existing patches. Do you think this should be a subject of another issue?

Mile23’s picture

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

Thanks, @hussainweb.

This patch removes an extraneous undefined class member, and also doesn't have out of scope array definitions.

alexpott’s picture

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

Committed cd880e6 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed cd880e6 on 8.0.x 
    Issue #2396715 by hussainweb: Clean-up responsive_image module test...

Status: Fixed » Closed (fixed)

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