Do not allow a module and theme to use the same name.

Our company just stumbled upon this issue. Took us an hour or two of valuable time to understand this, and we almost broke a customer website over this. :-(

I want to help get this patch merged. I haven't contributed to Drupal so far, so I need some advice. In which branch should this be fixed? "10.1.x" because this is the currently developed version? Or "9.4.x" because this is the last suppported version? Then I'll try to get the patch from #88 working with the comments from #94.

In general work is always done against the current development branch (10.1 now) and then backported to older versions. In this case, I suspect that the impact will be considered too big to backport to previous versions and it will remain in 10.1 only.

This would be a huge improvement, removing a potential major stumbling block, which many (myself included) has already or will run into inevitably.

Naming a custom theme and a corresponding custom module the same name after the project makes total sense.

So I hope a test expert will have time to take a look at @alexpott's comment from June 2022 at some point, so we can move this forward, and get it committed in Drupal 10 before too long.

This issue was created February 2009, 14 years ago.

@Ammaletu: In general work is always done against the current development branch (10.1 now) and then backported to older versions. In this case, I suspect that the impact will be considered too big to backport to previous versions and it will remain in 10.1 only.

I finally found the time to update the patch from #88 to work with the current 10.1.x branch. It mainly involved adding the test methods to the correct test classes. The two new test methods fail without the modifications and pass with them.

Ok, let's try this again. This time including the newly added files. :-)

This time with correct indentation. Sorry for the confusion. It's my first time working with patch files.

New exception will need a change record to announce it.

The docs need updating for new exception.

All patches should be attached with an interdiff so it's easy to tell differences between patches.

Would help to have a red/green tests-only too

Thanks for your comment, smustgrave! I added the new exception to the two installer interfaces, created an interdiff to patch #88 (which I based this on, as I understand that #89 went into the wrong direction?!) and added a second patch with only the changes for the new test cases.

As requested, I wrote a change record for this. Also my first time, so somebody please have a look if this is appropriate in tone and length: https://www.drupal.org/node/3353397 →

The last submitted patch, 105: tests-only-371375-105.patch, failed testing. View results →

Created a test module and theme with the same name.
Installed the module
Tried to install theme
Got fatal error Drupal\Core\Extension\ExtensionNameReservedException: Theme name test_name is already in use by an enabled module. in Drupal\Core\Extension\ThemeInstaller->install() (line 219 of core/lib/Drupal/Core/Extension/ThemeInstaller.php).

Think this is ready for committer review

The issue summary is not complete, there is no indication of what the fix is for the problem. Since there is a CR I started there. The CR uses the term 'technical name' for the machine name. I have not seen that before so I think it should be changed. But now I see what this is doing.

The I found a couple of things in the patch.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
   @@ -38,9 +38,12 @@ interface ModuleInstallerInterface {
   -   *   Thrown when the extension's name is longer than
   +   *   Thrown when a module's name is longer than
   

   This change is out of scope. This should be in it's own issue.

2. +++ b/core/modules/system/tests/src/Kernel/Module/InstallTest.php
   @@ -119,4 +120,22 @@ public function testModuleNameLengthWithoutDependencyCheck(): void {
   +    $this->container->get('module_installer')->install([$name]);
   

   The setup method for this test creates the module installer. There are several instances that can be changed to use $this->moduleInstaller.

3. +++ b/core/modules/system/tests/modules/theme_module_name_collision_test/theme_module_name_collision_test.info.yml
   @@ -0,0 +1,5 @@
   +description: 'Test module with same name as a theme.'
   

   s/with/with the/

4. +++ b/core/modules/system/tests/themes/theme_module_name_collision_test/theme_module_name_collision_test.info.yml
   @@ -0,0 +1,5 @@
   +description: 'Test theme with same name as a module.'
   

   s/with/with the/

5. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -182,6 +182,12 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +          throw new ExtensionNameReservedException("Module name $module is already in use by an enabled theme.");
   

   s/enabled/installed/

   Install/Uninstall is used for modules and themes. See 🐛 Do not use verb "Install" for things other than turning on modules/themes Fixed .

6. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
   @@ -213,6 +213,12 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
   +        throw new ExtensionNameReservedException("Theme name $key is already in use by an enabled module.");
   

   s/enabled/installed/

I then applied the patch and tested on a fresh install of Drupal 10.1.x. The patch works as expected. I had trouble finding the test module and test themeto install them. I did not expect the name to be completely different from the filename. I had to refer to the patch to find out is was 'Same name'. In my experience the filename for the test modules and theme modules are used as the basis for the name. Maybe the filenames and the name can be changed to something simpler like 'Name collision test'?. It may help others working on this in the future.

I reviewed the comments and didn't find any discussion of the text for the new exception message. In #67 the text was changed from the original without comment. It has been rerolled since then. Perhaps the message should like this, "The extension $module is already in use by an installed theme and cannot be reused." However, that discussion can be moved to a followup.

created MR from the latest patch +
Address the suggestions from #110 review ... including renaming test module and theme folders/filenames/names.

Retested following my steps in #109 and still seems good and changes appear to have done.

Thanks, quietone, for the review! And thanks vasica for doing the recommended changes!

"The CR uses the term 'technical name' for the machine name. I have not seen that before so I think it should be changed." -> Done.

Committed 72e7c01 and pushed to 11.x. Thanks!

lauriii → closed merge request !4180

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