Posted by Yoran Brault on December 13, 2007 at 5:29pm
12 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | system.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | redhatmatt |
| Status: | needs review |
| Issue tags: | Needs Documentation |
Issue Summary
when you have mixed case module name and so mixed case functions identifiers, drupal_get_schema_versions fails to find update hooks in .install files. This because php get_defined_fuctions() returns a lower case list of functions. Solution is just to lower case the module name before processing it.
--- drupal-5.5/includes/install.inc 2007-10-07 00:38:28.000000000 +0200
+++ drupal/includes/install.inc 2007-12-13 18:22:53.000000000 +0100
@@ -37,6 +37,7 @@
* FALSE.
*/
function drupal_get_schema_versions($module) {
+ $module=strtolower($module); // protection against mixed case module names
$updates = array();
$functions = get_defined_functions();
foreach ($functions['user'] as $function) {
Comments
#1
And as for Drupal 6.0:
Maybe a better solution is to ban MiXeDcaSeMoDules altogether?
There's another use of get_defined_functions(), in D6's theme.inc::drupal_find_theme_functions(). Is it OK?
And in the past the Actions module used get_defined_functions() to find action hooks. It's no longer does this, but I mention this to demonstrate that namespace introspection is a pattern which isn't alien to Drupalish code. Maybe we should simplify things and force modules to have names in all-lowercase?
#2
Oops, sorry, I accidentally changed the status.
I agree that it's a RTBC for D5, in order to support existing modules.
#3
Fixes are submitted for the branch head, which is 5.x-dev. Also, please don't set your own fixes to RTBC without getting reviewed. And finally, please upload an actual patch.
#4
OK this would be dealt with first in the current development branch.
The coding standards http://drupal.org/coding-standards state that function names should be lowercase, which implies module names should be also. What is not clear is that failure to adhere to this convention will cause the problem described in this issue. I have added a comment to that effect.
I can't see much appetite for Drupal enforcing the entirety of those coding standards for custom modules, but even so maybe the modules listing page could disable the "Enabled" checkbox for modules with invalid names, and print a suitable message. I can't see support being provided for a naming convention that deviates from Drupal's own standards.
#5
+1
For D5 and D6: I wonder if it's possible to configure the CVS server to reject folder names, in the top-level, having a capital letter.
#6
It's not possible to create rules in cvs to prevent importing new directories (which is how most people add a module).
We can create rules to block it at the node/add/project-project level.
#8
The following patch prevents mixed-case modules from being enabled, and displays a warning on the modules page.
Screenshot attached where I changed the filenames of the Devel module:
- devel_node_access to Devel_Node_Access en devel to Devel.
#9
If this gets in we will want to create a handbook page to explain how to resolve the problem so that people who see the error message can easily see how to fix it.
#10
In review of patch #8 with D7 CVS-head:
On the case of mixed upper and lower case named module, testModule, and at the admin/module page, the module enabling checkbox was grey'd out and the module required message read, "TestModule files has a mixed-case filename (testModule), but should be all lower-case" - note the absence of a period and the capitalization of TestModule, both of which appear to be consistent with core module behavior. On the case of all uppercase named module, it was not listed at admin/module. Patch does not appear to effect lowercase named modules in anyway.
#11
$v very likely means that it's not used, at all. It's just more expensive to run array_keys needlessly. Might be needless micro-optimization. As for $requires, $this->requires lists the modules this module requires. I believe it's fairly understandable.
#12
ok, i wont bother with such var names in the future. Thank you chx for the feedback. I see that they are even outside the scope of the patch too (thanks Dmitri). I also did not update the status but have now done so.
That leaves:
The function reference function_name() rather than function_name.
Requires is for a module dependency, but this message is not such a dependency. Perhaps there is a better/different way of providing the feedback. Or have the messages say testModule requires testmodule?
If $extra['requires'] is the place to set such a message, then I think the capitalization of the first char in the message is a diffident matter. What about a period at the end of the statement?
#13
Fixed comment in this patch. Regarding the other points:
Requires is currently also used for requiring a specific version of Drupal, so I think it can be used for requiring lower case names as well?
The rationale I used for the casing of the module name is that the first letter of the module should be uppercase when displayed: http://www.google.be/search?q=drupal+capitalization+standard
The period at the end of the filename is deliberately missing because this message becomes part of a list of messages seperated by commas (otherwise you could get '.,' at the end of the sentence).
#14
Patch #13 (and the explanation - thanks Stevel) has been re-reviewed and resolves all concerns and with the assumption that the 2nd reviewer will be more experienced than I on the appropriate use of the Requires message - marked as reviewed & tested.
#15
#13: 200628-fix-comment.patch queued for re-testing.
#16
#13: 200628-fix-comment.patch queued for re-testing.
#17
I don't think the solution in #13 is tenable. There are modules in contrib such as http://drupal.org/project/AudioRecordingField that have non-zero usage statistics.
#18
Since AudioRecordingField is 6.x only I don't see a problem with this. In fact, #13 will help ensure that developers don't fall down the MixeDcaSe trap in the future. However the message could be improved, maybe change
@module filesto@module module fileand also add a full stop at the end.#19
Here's a patch that tries to make the updates work for mixed-case module names. Let's do this instead and have #6 prevent new modules with mixed-case names instead.
#20
Will review due to needing this for D.O. upgrade. estimating 2hr.
#21
This isn't a bug in Drupal.org itself, untagging.