Updated: Comment #1

This issue has novice tasks. If you are an experienced core developer and have multiple commit mentions, please review novices' work on these tasks rather than doing them yourself. Feedback from experienced contributors is valued.

Problem/Motivation

Two word module names are a bit tricky in terms of plural, case, and machine name.

Some places when renaming picture to responsive image can be improved.

Kind of related background:

Proposed resolution

Update the inconsistencies.

Remaining tasks

User interface changes

API changes

Follow-up from #2124377: Rename "Picture" module to "Responsive Image" module.

CommentFileSizeAuthor
#13 drupal_project_name_clean_up_after_picture_module_renamed_responsive_image-2222431-08.patch6.35 KBtechnikh
#4 drupal.project-name-clean-up-after-picture-module-renamed-responsive-image.2222431-04.patch6.35 KBvisabhishek

Comments

yesct’s picture

yesct commented
  1. Regarding the permission, see #57c and #79 #80 in the original issue. The change shown in the interdiff in #82, is in terms of the human name used in t(). Perhaps for DX the permission machine name can be consistent with permissions in other modules also which seem to commonly use a plural? (ag "'administer [a-z ]*s' => array[(]" core/modules) forum module has permission 'administer forums', language module has permission 'administer languages', etc. 
    +++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationListUiTest.php
@@ -73,7 +73,7 @@ public function setUp() {
       'administer languages',
       'administer image styles',
-      'administer pictures',
+      'administer responsive image',

+++ b/core/modules/responsive_image/lib/Drupal/responsive_image/Entity/ResponsiveImageMapping.php
@@ -2,67 +2,67 @@
+ *   admin_permission = "administer responsive image",

+++ b/core/modules/responsive_image/lib/Drupal/responsive_image/Tests/ResponsiveImageAdminUITest.php
@@ -41,7 +41,7 @@ public function setUp() {
-      'administer pictures',
+      'administer responsive image',

+++ b/core/modules/responsive_image/lib/Drupal/responsive_image/Tests/ResponsiveImageFieldDisplayTest.php
@@ -42,7 +42,7 @@ public function setUp() {
-      'administer pictures',
+      'administer responsive image',

+++ b/core/modules/responsive_image/responsive_image.module
@@ -36,24 +36,47 @@ function picture_help($path, $arg) {
+    'administer responsive image' => array(
+      'title' => t('Administer responsive images'),

+++ b/core/modules/responsive_image/responsive_image.routing.yml
@@ -1,39 +1,39 @@
-    _permission: 'administer pictures'
+    _permission: 'administer responsive image'
...
etc etc

    similar to the language module having the permission: administer languages (not administer language, or administer language module), this should be: administer responsive images.

    should look for all of them. 

  2. +++ b/core/modules/responsive_image/lib/Drupal/responsive_image/Tests/ResponsiveImageAdminUITest.php
@@ -13,23 +13,23 @@
+      'description' => 'Thoroughly test the administrative interface of the responsive image module.',

    almost everywhere where the name of the module is followed by the word module, this should most likely be the proper name with capitalization, like:
    Responsive Image module. 

  3. +++ b/core/modules/responsive_image/lib/Drupal/responsive_image/Tests/ResponsiveImageAdminUITest.php
@@ -74,15 +74,15 @@ public function setUp() {
+    // Add a new responsive_image mapping, our breakpoint set should be selected.

    I think in most comments this was just a general description of the kind of mapping: responsive image mapping. And the machine name (with underscore) not used.

yesct’s picture

yesct commented
Issue summary: View changes

ug. just html fix in the summary.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I would say not to bother with issues like this in Test files. The tests already have a huge mess of non-grammatical messages, bad capitalization, etc. The important thing for tests is that they are clear enough to understand, and that they ensure we don't have regressions (i.e., that they continue to pass), and I'm inclined to just leave the tests as "good enough" and not worry about them, as the issues pointed out here are no worse than anything else in other Core tests. Really, the tests are kind of a mess... let's not bother with them, unless they're wrong or really unclear -- there are a lot more important things that we could be spending our time on.

Outside of files in namespace ...\Tests, though, I agree with YesCT's assessment:
- If text is referring to the name of the module, it should always be "the Responsive Image module", with that exact phrase and capitalization.
- The permission machine name should indeed probably be "administer responsive images" not "administer responsive image".
- We should not be using "responsive_image" ever in comments and other text. Either it should be "the Responsive Image module", or a specific function name like responsive_image_foo() ending in parens, or it should be "responsive image" or "responsive images" in plain text.

visabhishek’s picture

visabhishek commented
Status: Active » Needs review
StatusFileSize
new drupal.project-name-clean-up-after-picture-module-renamed-responsive-image.2222431-04.patch6.35 KB

I am uploading a patch based on #1. Please review.

Status: Needs review » Needs work

The last submitted patch, 4: drupal.project-name-clean-up-after-picture-module-renamed-responsive-image.2222431-04.patch, failed testing.

visabhishek’s picture

visabhishek commented

The failed test case in comment #4 is not related to this issue.

Any suggestions ?

visabhishek’s picture

visabhishek commented
Status: Needs work » Needs review

4: drupal.project-name-clean-up-after-picture-module-renamed-responsive-image.2222431-04.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

This looks mostly good, thanks!

One spot is a bit of a problem:

core/modules/responsive_image/lib/Drupal/responsive_image/Tests/ResponsiveImageAdminUITest.php

-    // Add a new responsive_image mapping, our breakpoint set should be selected.
+    // Add a new Responsive Image mapping, our breakpoint set should be selected.

Responsive Image should not be capitalized here. *Only* capitalize it if it is used like this "the Responsive Image module". All other uses are not proper nouns and should not be capitalized.

And... Is this really the only problem? So all we are doing in this patch is actually renaming the machine name of the administer permission? Because the issue implies that there is a lot more cleanup needed...

lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented
Assigned: Unassigned » lokapujya
lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented

The 3rd point in comment #1 was fixed for the specific line of code identified, but many other comments that need the same cleanup.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented
Assigned: lokapujya » Unassigned
technikh’s picture

technikh commented
Assigned: Unassigned » technikh
technikh’s picture

technikh commented
Assigned: technikh » Unassigned
Status: Needs work » Needs review
StatusFileSize
new drupal_project_name_clean_up_after_picture_module_renamed_responsive_image-2222431-08.patch6.35 KB

fixed the Capitalization issue mentioned in comment #8
attached patch.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Looks fine to me now. Leaving at Needs Review for the maintainers of the module to review, since it impacts the permission name.

yesct’s picture

yesct commented

13: drupal_project_name_clean_up_after_picture_module_renamed_responsive_image-2222431-08.patch queued for re-testing.

yesct’s picture

yesct commented
Status: Needs review » Reviewed & tested by the community

This looks fine to me. pinged a couple people. I'm ok with waiting for +1 from someone else, but marking it rtbc to get it on their radar also.

attiks’s picture

attiks commented

Looks good

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: drupal_project_name_clean_up_after_picture_module_renamed_responsive_image-2222431-08.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Needs review

13: drupal_project_name_clean_up_after_picture_module_renamed_responsive_image-2222431-08.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Unrelated test failure, back to previous status.

By the way we're in "criticals and majors only" commit period pre-alpha until Wednesday.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit f18d62b on 8.x by webchick: 
    Issue #2222431 by TechNikh, visabhishek, YesCT, jhodgdon: Project name...

Status: Fixed » Closed (fixed)

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