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

This has caused me to lose more hours of my life than I care to admit. So I'm more than fine fixing this, however I'd love for someone like chx or dmitri who have done development on this form to take a look at this patch.

Could we also get a screenshot of what this looks like when there's a conflict?

Huh. Must've cross-posted. Sorry!

And while we're at it...

+  $result = db_query("SELECT name FROM {system} WHERE type='module' AND status=1");
+  while ($module = db_fetch_object($result)) {
+    $enabled_modules[] = $module->name;
+  }

Not sure if these get_enabled functions are needed at all, but they should use the new DBNG syntax..

foreach ($result as $module) {

And for these simple loops, you can use fetchAll() instead.
return db_query("SELECT name FROM {system} WHERE type='module' AND status=1")->fetchAll();

But again, I'm not sure if they are needed at all.

See http://drupal.org/node/310072

Well, I'm really sorry for the inconvenience.

Wait. Just to be really clear, I meant that the fact that you can't name a module and theme the same has caused me to lose many hours of my life, NOT you and not this patch. :D That was code for "Thanks a lot for looking at this, it's a really confusing and hard to track down problem when it bites you!" :)

Screenshots look awesome. That's exactly how I was hoping this would be fixed.

This is IMO way way too much in regards of code vs benefit. Why not unique the name in system and in module_rebuild_cache and in _system_theme_data simply catch the exception and put up an error message? This would be about three lines of coded added at each code place... (try, catch, drupal_set_message).

Rohin Knight, I hope you are not offended. I have asked other community members to chime in here because my solution would be a lot less pretty than yours but I do not feel the need for that lot of code. Let's see what others say

Well, this has happened to me too. I guess chx's simple approach would be *good enough* since it would at least stop people like me from doing the wrong thing. A unique DB key is a simple and full proof solution. Coupled with an error message it could even be user friendly, although maybe not as much so as the submitted patch.

Let me see if I got this right:

- add "name" to the "unique keys" in the {system} schema in system.install
- try-catch the insert queries on that table
- warn about duplicate system names in the catch block

Is this all that needs to be done for chx's approach?

Yes.

While experimenting with this, I have found several caveats that may make this non-trivial:

1.) The upgrade function will need to find duplicates, and follow a certain rule to remove them, because the constraint can otherwise fail to be added to an existing table. The easiest resolution is deleting the theme, since things are less likely to break.
2.) Themes are currently refreshed by deleting the records and reinserting the newly discovered ones, two queries in total. If the insertion fails because a new theme shares the name of a module, then we are left without a single theme in the database. (That is not good.) The catcher will need to switch to the maintenance theme or try to reinstall Garland at least.

subscribing

Since I was the one who wrote a similar handbook snippet elsewhere...

Seems to me the issues identified in #23 violate the 'we can break you code, but not your data' rule. I don't think we should be deleting things from people's databases under any circumstances, unless we're sure we put it there.

Also, since having the duplicate name in the database is only a potential for problems, and not an absolute bug, it's better to err on the side of not deleting things.

Finally, if someone who knew what they were doing really needed to go against the principle, they could manually hack their database values, without having to hack the database structure, to get a around a restriction in the UI.

So, I think the UI restriction is the better approach than the DB key. Surely we can trim down Rohin Knight's code a bit for some middle ground. Given #23, we'll need more than 3 lines of code for the DB key approach anyway.

What would be the problem in allowing to have a theme and a module with the same name? Why for D7 the preferred approach is to not allow people doing that instead of adding "module_" or "theme_" to the value in the database?

It has nothing to do with the database. It's collision in the PHP function namespace.

When Drupal called foo_block, it has no idea if foo_block is in foo.module or themes/foo/template.php .

That's why the only real solution is to change the function naming convention, e.g, foo_define_blocks for modules and foo_render_block for themes.

I think the possibility of name collision is a bug to be fixed. If nothing else, it's a key DX feature. Drupal core should not define theme_foo and foo_hook.

As I said in the other issue, there is no hook_block anymore in D7. But this can also occur if one module provides theme_something and another module provides hook_something, this is not something that is easy to control.

But I'm also not sure what to do with sites that already have that situation. IMHO, we wouldn't delete data, because you have to disable all themes except garland when upgrading to D7 anyway. Whatever we do, we have to inform the user/admin why his theme/module doesn't show up in the list.

Sure, hook_block is just an easy example to illustrate what we should continue to try to avoid.

We have a large number of sites that this would break if we applied the patch (theme name and module name = 'custom').

In our case, this is an intentional split between changes we think should be in a module (or have to be in a module) vs ones that would go in a theme. There are never any hook collisions, because we only ever use one or the other for a given hook.

Can we just make this a warning rather than actually not letting you do it? Or better yet, throw an error only if there is an actual namespace issue. The check only needs to happen on the theme / module list pages.

Marked #511874: New "Video Upload" Module's Name conflicts with one of this Module's components as a duplicate of this issue. What was raised there was a namespace collision between a module and another sub-module, rather than module and theme. However the same fix should work for both...

Or better yet, throw an error only if there is an actual namespace issue.

These conflicts can cause problems that are not necessarily namespace conflicts, but are much harder to discover. For instance, if I have example module and example theme, when i define a preprocess function in my theme like example_preprocess_node(), this is interpreted by the theme registry as a module preprocess function, since it has no way to determine where this function comes from. Since the theme registry thinks this is a module preprocess function, it gets called in the middle of the stack, rather than as the last function in the stack.

This example won't cause a PHP error like theme_block(), but does make bugs really hard to find. I think that with issues like this, which have the potential to trip people up and tie up support channels, it is better to keep them from happening at all.

That's a good point about the order of execution, etc.

However, I still think this should warn you on the module / theme screen via drupal_set_message but still allow you to enable it. Otherwise, this will be a headache in upgrading from D6 to D7, and I don't see any additional benefit going from "Hey your stuff might not work, we recommend changing it" to "You have to change it if you want to enable it."

AFAIK, we've always more-or-less "supported" allowing a module and a theme to have the same name. At the DB level, we have an index (which is currently not unique, but which possibly should be unique) for the composite key "type, name" (where type is always 'module' or 'theme'). At the PHP level, we have API functions like drupal_get_path($type, $name). And at the file level, we have ./modules and ./themes, ./sites/foo/modules and ./sites/foo/themes, etc.

So, my point of view is that right now, it's ostensibly "allowed" to give a module and a theme the same name (at least, Drupal certainly gives you that impression); and if that causes issues, then that is a bug. In other words, I'm saying: the bug is not that you're allowed to give them the same name, the bug is that giving them the same name can cause side-effects. And anyway, let's face it, being able to give modules and themes the same name is pretty useful in many cases. So let's work on continuing to support it, and on eliminating the side-effects.

The three main causes of namespace collisions are:

1. hook_foo() implementations in a module and in a template.php (e.g. hook_theme() defined in both)
2. modulethemename_preprocess_baz() implementations in a module and in a template.php
3. a theme_bar() implementation in template.php that collides with a function name in a module (e.g. modulethemename_frontpage() being a themable function in template.php, and a menu callback in a module)

All of these collisions can be fixed by introducing additional new prefixing. My vote is to have the extra prefixing in template.php files, since this would require far less work and extra characters than would be involved in adding the prefixes to every function in every module. Just three are needed:

1. themename_hookname() becomes themename_hook_hookname()
2. themename_preprocess_themablefunction() becomes themename_template_preprocess_themablefunction()
3. themename_themablefunction() becomes themename_theme_themablefunction()

Jaza, my preference would still be to warn the user and make it discouraged.

While allowing you to name a module and theme the same name is a feature, it has limited utility (it doesn't really add any new things you can do) and what you are describing as a way to truly support it involves making the D6 -> D7 port process more painful. So, I'd really rather we add in a warning and leave it at that.

@kwinters: agreed, my proposal would mean a much more painful D7 upgrade path (and worse DX for D7 theme developers), and all to support an edge case that few themers will need. In retrospect, it's clearly ridiculous that such a proposal would actually get implemented and committed to core. Anyway, I think it was good to throw it out there, if only just to demonstrate what is required were we to "fully support" modules and themes with the same name.

Agreed that a strong warning about same-named modules and themes is needed, at the minimum.

FWIW, we could completely avoid such issues if themes would be treated as modules. That would not only solve this issue, but also the underlying issue why one would want to create a module for a theme in the first place. Additionally, we already have a lot of issues in the queue that would be auto-resolved if themes would be treated like modules. Not to mention the sheer amount of new possibilities this would open up for module developers.

@sun: Could you explain what you mean by that?

@webchick
In light of this issue it will mean you can't name two modules using the same name :). Other things are for another issue to discuss.

No, I know that. ;) I'm wondering what exactly "make themes act like modules" means. Getting rid of the distinction in the system table? Loading them along with module_load_all() or whatever it is? Giving them .install files?

I think redefining the relationship between themes and modules goes beyond the scope of this issue.

IMHO, this patch is critical because namespace collisions are fairly likely given what should be the proper use of custom theme functions in custom and contrib modules. There are about 130 hooks available to the developer in Drupal core, many of which have innocent sounding names that could double as theme function names to a theme/module developer not aware of such namespace restrictions. Eg: hook_field_info as theme_field_info, hook_comment_view as theme_comment_view, etc.

Re-rolled #12 patch to conform with DBTNG. Is this what we want, or do we just want a big nasty drupal_set_message and maybe a warning in the Status report?

---
This patch is contributed by TP1 Internet 360, a Montreal-based Drupal web studio.

Visual review of patch looks good, though I didn't actually apply it. But testbot did. :-)

How do we deal with this in update.php if a site already has a matching theme & module, but no obvious issues (yet) ?

As kwinters (#31) and I and others have seen, this is not uncommon. Some commercial themes even ship with support modules named the same.

I'd also like to hear sun expand on the idea of 'themes are modules', and while it is beyond (but not outside) the scope of this particular issue, and almost certainly too big to implement for D7 at this point, if it will solve many issues at once, it's worth considering.

Dealing with update.php -- that's an edge case I hadn't considered. What I am equally concerned about is how this would impact enabling a module via drush. Does anybody actually use the Modules page anymore anyway? :)

This is going to be especially critical if #591794: Allow themes to alter forms and page gets in.

On further thought, the upgrade issue is not fatal, because the upgrade instructions inform the user to switch to a core theme and disable contrib modules before proceeding. ( I just don't always do that, at my own peril...) :-)

Namespace collision is a pretty big issue with Drupal these days, IMHO. With so many contrib modules implementing their own hooks, it's becoming more and more common to have unexplainable errors and/or performance related issues (see: http://acquia.com/node/1011148#comment-15012) related to namespace collision with hook implements.

I think this patch is a small step towards reducing these collisions. Maybe it's too small, but it's a step regardless.

0. admin.css: There really isn't an existing class we can reuse instead of introducing another? I can't imagine a situation where a designer would want to specifically style a 'conflicting theme' message different than any other such message.

1. The very last hunk has an incorrect message: 'Incompatible with this version of PHP'

2. If I'm reading this correct, system_enabled_themes() and system_enabled_modules should return arrays, not db objects?

3. Can we have the big red X on theme form, like for incompatible versions, instead of just removing the enabled checkbox?

While it'd be nice to get this fixed, it's been like this since time immemorial and certainly doesn't qualify as critical.

I think we can go with the chx approach. We just need the upgrader (fix_requirements() function) to check if there is a conflict with the new key and ask user to resolve it before proceeding.

Here's an attempt at the chx approach.

It took a little extra code because we currently assume that we succeed every time we try to enable a theme. So I had to make an effort at eliminating that assumption.

Is this really enough? How do we gracefully handle failure in system_update_7051()? There will be plenty of sites with dupes here.

Because system_requirements() is checked before every run of update.php, system_update_7051() will never run until the user resolves any conflicts.

#51: chx_hates_name_conflicts.patch queued for re-testing.

subscribe

Just wondering if this problem will still be present in Drupal 8, with all the namespacing/PSR-0 changes that are commited?

I've just encountered an issue with Drupal 8 where I had a profile (basically a module) and a theme named the same. Everything appeared to work fine except the libraries of the theme refused to load. Some hours of debugging later I realised that in the LibraryDiscoveryParser when it tries to resolve libraries in buildByExtension it decides whether the extension is a module or a theme. In my case because I had a profile named the same as the theme it decided it was a module and subsequently could not find the associated library files. It was a silent failure (as far as I know) so no real way of finding this out without some step debugging.

So this is one area where it is still a problem in Drupal 8.

Marking for potential Drupal 7 backport; see the linked issue for another "interesting" side effect that can occur here.

#2472119: Extension installers allow extensions with duplicate names to be enabled

I have just encountered this error on Drupal 8.3.4. I had a theme and a module with the same name, with the result that the theme libraries failed to load despite showing up as attached to the page (hook_page_attachments). The failure was silent, so it puzzled me for a while.

I took a stab at creating a new patch for fixing this in Drupal 8. Attached is the patch, which runs the check only when enabling a theme/module. The error isn't pretty, it works the same way that extension name length check does; basically it just fails. Is there need for a nicer UI notice for this?

Re-ran the tests against 8.5.x, still applies and all the tests pass. Hopefully someone finds time to review this.

Semi-related: I haven't seen any documentation anywhere about this but I had a problem when there was a number in the machine name of my child theme, preprocess hooks wouldn't fire. see here for more info: #2922599: Preprocess hooks fail if child theme contains numbers in machine name. I would recommend patching core to kick an error or status notification if a theme has a number in its machine name.

I am facing this problem with a site where there is a warning about a `mothership` module missing:

The following module is missing from the file system: <em class="placeholder">mothership</em>. For information about how to fix this, see <a href="https://www.drupal.org/node/2487215">the documentation page</a>. bootstrap.inc:1143

Obviously, Mothership is a theme and not a module (this base theme is actually being used on the site).

I have checked the database, and the module path in the system table seems to be pointing to the correct directory. It is also listed as a type of theme. So is this a core bug, reporting a theme as a missing module, or is this related to the issue we are trying to solve here?

PS: the theme is not under the usual /site/all/themes dir, but rather inside a custom profile. Also it is for some reason nested inside another subdirectory (for some reason the previous agency must have done this). So the path to the .info file is under /profiles/profile_name/themes/mothership/mothership/mothership.info

In my view, this could be broader - perhaps even as far as triggering an 'error' severity item on the Status report.

Currently the error is only thrown when manually uploading a module/theme or supplying a URL.

The situation that brought me here was:

• creating a theme directory and .yml files by hand (not uncommon), which happened to duplicate a module name
• enabling rather than installing it
• then wondering why the CSS library wasn't loading.

Could we at least do a check when choosing 'Set as default' on /admin/appearance too?

I don't think it should ever let you install the 2nd (module or theme) once one of them is installed. This happened to me a couple weeks ago. It was quite a pain to clear the errors that kept occurring even after un-installing the module that had the same name as the custom client theme.

> I don't think it should ever let you install the 2nd (module or theme) once one of them is installed.

+1

Though I think it's doing that at the moment. It's just that:

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -136,6 +136,12 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
+        // Throw an exception if a theme with the same name is enabled.

+++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
@@ -163,6 +163,12 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
+      // Throw an exception if a module with the same name is enabled.

System module's ModulesListForm needs to catch these exceptions and show the user a message.

Just wasted couple of hours trying to find out why my theme would not load any css styles..
And I used xdebug to dig deep into core ant stumbled upon this code: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Asset%21L...

if ($this->moduleHandler
      ->moduleExists($extension)) {
      $extension_type = 'module';
    }
    else {
      $extension_type = 'theme';
    }

So as far as I get.. a module would steal the libraries from a theme if they share the same name.

Rerolled #67 for D9.1.x, tested on D9.1.8. I think that this simple patch should be extended to be more user-friendly solution like #11.

This annoying 12-years bug is a sample of Drupal-hate, because I wasted several hours trying to understand why my CSS in theme is not executed.

I think that at least, we need to update modules/themes documentation to avoid creating new modules/themes with identical names.
From Theming Guide:
It must be unique. Your theme should not have the same short name as any other module, theme, theme engine, or installation profile you will be using on the site.

fixed 7 Custom Commands Failed.

Patch #85 applied cleanly and throw an exception if a theme with the same name is enabled .Adding Screenshot for the reference.

RTBC +1

Patch #85 applied successfully

This one seems well worth getting in to core, but it's not going to get there without tests!

Here's a patch with appropriate test coverage. Unfortunately it fails! There's some problem where if I install and then uninstall a module, the theme installer still things the module is installed. The module installer does not have the same confusion about themes being installed and uninstalled.

I'm still trying to figure out why. In the meantime the test might as well fail up here for all to see.

I found a fix for the bug I found during testing. All I did was change a line in the ThemeInstaller so that it's the same as the analogous line in the ModuleInstaller. I wonder if the bug is somehow related to this issue: #3063687: ConfigFactory static cache is not updated when cache tags are invalidated?

Anyway, Here's a patch with the fix. I know that this undoes some dependency injection, but at least it works and it's consistent with the ModuleInstaller, FWIW.

If you look at the interdiff you'll also see I added in a missing us statement for the test I added in #88. The missing use statement did not cause the test to fail because it failed before the test made it to the missing class. So nothing major to worry about there.

Removing credit for #87 which is a renamed copy of the screenshot provided in the prior comment #86

+++ b/core/modules/system/tests/src/Functional/Module/InstallTest.php
@@ -99,4 +100,22 @@ public function testModuleNameLength() {
+  /**
+   * Tests installing a module with same name as enabled theme.
+   */
+  public function testInstallModuleSameNameAsTheme() {
+    $name = 'test_theme_module_name_collision';
+
+    // Install and uninstall the module.
+    $this->container->get('module_installer')->install([$name]);
+    $this->container->get('module_installer')->uninstall([$name]);
+
+    // Install the theme then module.
+    $this->container->get('theme_installer')->install([$name]);
+    $message = "Module name $name is already in use by an enabled theme.";
+    $this->expectException(ExtensionNameReservedException::class);
+    $this->expectExceptionMessage($message);
+    $this->container->get('module_installer')->install([$name]);
+  }

This test is broken in #88 because it is in a BrowserTestBase test. These tests should never ever use $this->container - especially when testing module installs because $this->container gets messed up. This will pass if you move it to a kernel test.

The changes in #89 are unnecessary.

Thanks for the review @alexpott!

These tests should never ever use $this->container - especially when testing module installs

Good to know! I think I was probably imitating the code in the method testModuleNameLength which is right above this new one.

This test is broken in #88 because it is in a BrowserTestBase test

I'll have to find some time to do manual testing again, but I'm pretty sure the failing test matched my manual testing experience. I wish my comments from 7 months ago were a little more detailed.

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.

Sounds good @Ammaletu, thanks! Getting this in core 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 this can get committed in Drupal 10 before too long. The 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

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 #3182405: Do not use verb "Install" for things other than turning on modules/themes.

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 vasike 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!