Posted by MartysMind on February 9, 2009 at 8:21am
25 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | system.module |
| Category: | task |
| Priority: | normal |
| Assigned: | matt2000 |
| Status: | needs work |
| Issue tags: | DrupalWTF, DX (Developer Experience) |
Issue Summary
It would be keen if Drupal core were nice enough to alert you if you tried to register a module and theme of the same name... but it doesn't.
If you name your theme and a module the same name you get name space collisions. E.G samename_block(..) will conflict if you have it declared in both module and theme.
It would save a lot of new Drupal developers frustration if Drupal didn't let you register a theme and active module by the same name.
Also noted here http://drupal.org/node/143020
Comments
#1
So we could prevent users from enabling the module if a theme with the same name is currently enabled and vice versa. Same way modules cannot be enabled if dependencies are missing with the tick box grayed out. Could also do a validation check when form is submitted, just in case a user has both module and theme forms open at the same time, which would also give them the option to try and enable a module and a theme of the same name.
#2
Okay, here's the patch for D6. I'll now work on the D7 one.
#3
#4
And here is the one for Drupal 7. I hope this gets accepted into core.
#5
#6
#7
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?
#8
Huh. Must've cross-posted. Sorry!
#9
And while we're at it...
#10
The last submitted patch failed testing.
#11
Well, I'm really sorry for the inconvenience. Here's what it looks like on the module and theme forms when there is a name conflict.
#12
Okay I've fixed the patch and this time it passes the module dependencies test.
#13
#14
+ $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..
<?phpforeach ($result as $module) {
?>
And for these simple loops, you can use fetchAll() instead.
<?phpreturn 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
#15
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.
#16
The error message seems a little confusing to me it needs to explain how it conflicts.
Example:
A theme with the name Garland already exists. (A theme and a module cannot have the same name).
#17
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).
#18
I think I'll leave this patch for someone else to polish up for Drupal 7 who is more familiar with the database abstraction layer and coding preferences.
#19
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
#20
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.
#21
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?
#22
Yes.
#23
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.
#24
subscribing
Since I was the one who wrote a similar handbook snippet elsewhere...
#25
The last submitted patch failed testing.
#26
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.
#27
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?
#28
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.
#29
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.
#30
Sure, hook_block is just an easy example to illustrate what we should continue to try to avoid.
#31
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.
#32
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...
#33
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.
#34
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."
#35
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" (wheretypeis always 'module' or 'theme'). At the PHP level, we have API functions likedrupal_get_path($type, $name). And at the file level, we have./modulesand./themes,./sites/foo/modulesand./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:
hook_foo()implementations in a module and in a template.php (e.g.hook_theme()defined in both)modulethemename_preprocess_baz()implementations in a module and in a template.phptheme_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:
themename_hookname()becomesthemename_hook_hookname()themename_preprocess_themablefunction()becomesthemename_template_preprocess_themablefunction()themename_themablefunction()becomesthemename_theme_themablefunction()#36
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.
#37
@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.
#38
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.
#39
@sun: Could you explain what you mean by that?
#40
@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.
#41
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?
#42
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.
#43
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.
#44
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? :)
#45
This is going to be especially critical if #591794: Allow themes to alter forms and page gets in.
#46
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...) :-)
#47
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.
#48
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?
#49
While it'd be nice to get this fixed, it's been like this since time immemorial and certainly doesn't qualify as critical.
#50
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.
#51
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.
#52
Is this really enough? How do we gracefully handle failure in system_update_7051()? There will be plenty of sites with dupes here.
#53
Because system_requirements() is checked before every run of update.php, system_update_7051() will never run until the user resolves any conflicts.
#54
#51: chx_hates_name_conflicts.patch queued for re-testing.
#55
The last submitted patch, chx_hates_name_conflicts.patch, failed testing.
#56
#57
subscribe
#58
Just wondering if this problem will still be present in Drupal 8, with all the namespacing/PSR-0 changes that are commited?