Module packages using different letter-casing appear multiple times

Looks good to me.

@sun, would you please explain "the rules" as you mentioned above. I thought that bugs were fixed in the current release, but new features were added into the next release. Please correct me. :)

Shouldn't this be handled in the projects themselves, i.e. shouldn't the two maintainers agree that e.g. in this case "User interface" is correct?

The portion of the patch that changes color.info is no longer needed, and is in fact wrong. All core modules in 8.x and 7.x have consistent usage of capitalization. They are all "package = Core" or "package = Testing".

Changing status to "needs work" because of the above.

This is still an issue, but the code has been moved to ModuleListForm.

Reviewed and it seems that patch in #10 is not working. Package names should be capitalised before building the form element. I provided a couple of screenshots to prove that now it's working.

@joshi.rohit100 It's a good practice to test and provide screenshots to help reviewers to check that your patch works. Anyway thanks for your contribution ;)

Hi

My name is Jonathan. I'm at DrupalconLA and I've verified that the patch applies.

Machine names are lower case - this patch is forcing the first character of the machine name to upper case and therefore failing in tests - for example:

$this->drupalPostForm('admin/modules', array('modules[Field types][image][enable]' => "1"), t('Save configuration'));

fails because it is should now be 'modules[Field Types][image][enable]'

The solution is to fix all the tests that refer to packages with improper nouns

(w/ Thanks to cosmicdreams!)

cosmicdreams and I think that by using proper nouns for package names in the tests this will pass.

Found one more spot that needs ucwords in InstallUninstallTest.php

Patch is green, reviewing...

This will ultimately need to be reviewed by a core committer to see if it's the change we want to make. It technically does not alter any APIs, just some strings that shouldn't have been relied upon explicitly.

Added beta evaluation. Screen shots to follow

Before the module is applied we can have package names that only vary by the way they are cased. Like this:

After the patch those casing difference are now denormalized and grouped. Like this:

I meant this

#18 seems to solve the issue here

Needed a re-roll due to text changes in button text.

I have also changed the case to ucwords(strtolower($package)) as strtolower($package) was producing incorrect case in the package summary output.

I think you should be using Unicode::ucwords() rather than the PHP functions.
https://api.drupal.org/api/drupal/core!lib!Drupal!Component!Utility!Unic...

Replaced PHP functions with Unicode methods.

I have tested patch #34 with the following dummy modules:

• name: Module A
  package: Test Package
  

• name: Module B
  package: test package
  

• name: Module C
  package: TeSt pAcKaGe
  

Before the patch:

After the patch:

Also, the package label is sensibly capitalised:
<summary role="button" aria-controls="edit-modules-test-package" aria-expanded="false" aria-pressed="false">Test Package</summary>

Hi,
I have tested patch 34 with three test modules. See attached Extend before and after to see the results.

Modules are correctly grouped after the patch was applied and label is correctly capitalized

Hide Belgium Cities

Patch is ready to be RTBC.

+++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
@@ -75,7 +76,7 @@ public function testInstallUninstall() {
+      $package = Unicode::ucwords(Unicode::strtolower($module->info['package']));

@@ -145,7 +146,7 @@ public function testInstallUninstall() {
+      $edit['modules[' . Unicode::ucwords(Unicode::strtolower($module->info['package'])) . '][' . $name . '][enable]'] = TRUE;

Why didn't we go for Unicode::ucfirst? There'd be way less change in HEAD then right since we would not need to change the tests?

I have changed Unicode::ucword to Unicode::ucfirst as suggested in #42.

I chose Unicode::ucword so to have the labels in title case, which has the first letter in each word capitalized.

Moving it to needs review.

I helped out in the original development of this patch, but this approach is more complete and ucfirst is what I was going for so I agree this is a good change.

Updated IS to use the template and identify novice tasks.

I'm not convinced this is change is correct. At the moment we don't translate package names - if we do then this change is incorrect. If this occurs shouldn't we just raise an issue against the module to fix it?

I've added some related issues from the module perspective.

The closest I can find for package name specification is Writing module .info files (Drupal 7.x). It is in the package section. Although this is for Drupal 7.x, I believe D8 follows the same guidelines.

The relevant part about case is repeated below:

If present, the package string groups modules together on the module administration page (admin/modules); the string should therefore be the heading you would like your modules to appear under, and it needs to be consistent (in spelling and capitalization) in all .info files in which it appears. It should not use punctuation and it should follow the Drupal capitalization standard as noted above.

It mentions "Drupal capitalization standard" but that is not described explicitly, it is "it should be capitalized as a proper name" as mentioned in the name section. From this the propper usage is package = User Interface

There is also a lot of "should"s in the documentation. These would need to be changed to "must" or they can not be really classified as bugs, and it also identifies which modules are using the incorrect capitalization.

Tested #43 path.
Created one module with package name Issues and other module with package name ISSues.

Both modules were shown in same Package, so it means that patch #43 works.

Marked the task embed before and after screenshots as done because they appear to be in comment #35. Don't see a reason why new screenshots are needed.

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -202,7 +202,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-        $package = $module->info['package'];
+        $package = Unicode::ucfirst(Unicode::strtolower($module->info['package']));

I don't think this fix is right. If the packase is 'Commerce Tools' then it'll make it 'Commerce tools' where we do '#title' => $this->t($package). We should only be normalising the package key.

The patch in #43 is the most current patch under consideration.
That patch needs to be re-rolled to address the concerns raised by #63 and to account for changes in core made over the past 3.5 years.

There are three hunks in the patch. One hunk applies to ModulesListForm.php and the other two apply to InstallUninstallTest.php. But because the changes made in #2665152: Simplify module form structure and fix bugs when Suhosin is used removed the package name from the form element markup on the install page, we no longer need to make package-name-related changes in InstallUninstallTest.php.

What remains is a patch that changes just ONE LINE in ModulesListForm.php. This line changes form keys to use the package name standardized by Unicode::ucfirst(strtolower("package name from .info.yml")). This also changes the package name used as the #title in the details form element that wraps the modules in that package.

@alexpott's concern in #63 is that these package titles will now be listed in sentence case. He said:

I don't think this fix is right. If the package is 'Commerce Tools' then it'll make it 'Commerce tools' where we do '#title' => $this->t($package). We should only be normalising the package key.

But back in #42 he also said:

Why didn't we go for Unicode::ucfirst? There'd be way less change in HEAD then right since we would not need to change the tests?

You can't really do both (see below). I agree with the second quote - all of core uses sentence case (ucfirst) so ucfirst is far less likely to be disruptive. The exception to sentence case is the new "Core (Experimental)" package, which was added long after this issue was last reviewed. But this patch only changes how that string is displayed on the modules list page, it doesn't break any test or necessitate changes to core .info.yml files. (And the Seven theme displays these titles in smallcaps, so it's not going to be capitalized like in your .info.yml file anyway ...)

There are reasons I disagree with the objection in #63:

1. There are some modules in the package "Commerce (contrib)" and some in the package "Commerce (Contrib)". Right now those are listed in two different details elements with two different titles. If you standardize the form element keys, then they will at least be grouped together, but which of the two titles is correct, and which should be used as the title? The first module encountered? That will change depending on your installed modules, and the title is set before the modules are sorted by name so the "first encountered" will also depend on the order the modules were added to the file system and discovered. We HAVE to pick a case for the package title, so it might as well be the same as for the form keys. We have no other way of deciding which is "correct".
2. It has been a Drupal coding standard for a very long time that form element titles should be sentence case. In fact, sentence case is pretty much standard for all parts of Drupal. Although package names have never been standardized, it's already a de facto standard, as evidenced by Drupal core.
3. Standardizing the details element titles to title case causes its own problems. We can't just ucwords(strtolower("Core (Experimental)"))because that won't work - it results in "Core (experimental)". And we have to transform it to a standardized version or else we leave ourselves vulnerable to this grouping issue all over again.

So I propose the following patch, which again is just a one-line change which will fix this persistent and still-occurring problem.

New patch. Should be using mb_strtolower() instead of Unicode::strtolower().

Optimistically setting this RTBC without tests as a litmus test for the discussion in #2972776: [policy, no patch] Better scoping for bug fix test coverage

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -157,7 +157,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-        $package = $module->info['package'];
+        $package = Unicode::ucfirst(mb_strtolower($module->info['package']));

Looking at the code - we end up doing '#title' => $this->t($package),

I think this means that we should not pass soemthing that has been put through Unicode::ucfirst(mb_strtolower()) to it.

I think we should create a normalised key and then pass the package from the first module in the list. This means we won't break translations or end up oddly capitalising something that was intended to be capitals. For example if your are building a site for the World Wildlife Fund and you use that or WWF as the package name for the custom modules.

Something like this...

Just fixing the so-called typo meh, US spelling--

Marking as RTBC, because all I'm changing is a borderline typo

Again, this is a test case for #2972776: [policy, no patch] Better scoping for bug fix test coverage so there's no tests

Tested patch #77 by using Action and Automated Cron.
Changed Action to User interface
Changed Automated Cron to User Interface

Verified they appear as separate "packages" on the modules patch
Applied the patch and they are grouped together.

Seems like something a simple test could be added for but this is a minor issue so maybe it is overkill.

Checking RTBC issues. There are no unanswered questions here. I see that there are no tests but that is explained in #73. This is making a change for the UI but there are no before and after screenshots to demonstrate that this is working as intended. This is needed in all cases where the UI is change, even more so here when there are no tests.

Tagging for screenshots which should be in or linked to from the Issue Summary so they are easy to find. I went to add this to the remaining tasks but it is already listed.

Holy moly, it has a D8 beta evaluation. Old issue alert.

1. This should have a change record and a release note snippet, since it's both changing the behavior for these keys and also altering how core (and potentially Project Browser in a followup?) sort and list data from Drupal.org's project info.

   We might also want to check with the infra team about this -- I don't know if d.o currently does anything with this data or not, but if we're changing the way core lists modules and d.o is able to extract the package and use it somehow, d.o would probably also need a parallel change.

2. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -190,9 +190,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        // capitalization are grouped.
   

   Nit: Capitalizations.

3. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -204,15 +206,17 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      // Use the first module's package label as $package_key is normalized.
   

   This is a little confusing. I think it is trying to say:

   Use the first module's package label, because the $package_key value has already been normalized.

4. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -204,15 +206,17 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      $package = $modules[key($form['modules'][$package_key])]->info['package'];
   

   A question about the actual functionality here. If there are multiple labels for the same package name, especially due to casing issues, presumably some are canonical (Sentence case labels) whereas some might be malformatted (Title Case Labels or lowercase labels). So, by defaulting to whatever is first in the array, are we perhaps overwriting a good label format with a bad one?

   Should we have additional functionality to Title case the label if there are multiple versions that differ only by casing?

   And what if the labels differ by more than their casing? What if they legitimately belong in different groups and it's the package key that's completely wrong?

Asked in slack for the two groups in 1.

This needs an mr with the concerns 2, 3 , and 4 addressed.

So, by defaulting to whatever is first in the array, are we perhaps overwriting a good label format with a bad one?

Possibly, but it's better than multiple groups.
Simple title or sentence carrying was ruled out.

Should we have additional functionality to Title case the label if there are multiple versions that differ only by casing?

I feel strongly we should not, that would potentially be a lot of shuffling and guessing.

And what if the labels differ by more than their casing? What if they legitimately belong in different groups and it's the package key that's completely wrong?

I'm not sure how that could arise or if it's a concern to worry about. If you have an examples that would help.

Asked in slack, @dww and @drumm confirmed d.o does not use the package key.

I don't see any evidence of project browser using this, but pinged again.

@chrisfromredfin also confirmed.