Invalid hook_schema() result can trash entire schema
bjaspan - July 27, 2008 - 02:44
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | database system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | Quick fix |
Description
If any module does not return a value from hook_schema(), the entire schema structure ends up being set to NULL because array_merge(array(), NULL) returns NULL (apparently). The attached patch prevents a single buggy module from taking down the system in this way.
IMHO, it should be applied to 6.x as well.
| Attachment | Size |
|---|---|
| get-schema-null.patch | 847 bytes |
#1
#2
This is true of theme registry as well. I think a little defensiveness like this is a good thing.
#3
CNW because
drupal_get_schema()move into boostrap.inc.#4
#5
The last submitted patch failed testing.
#6
See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
#7
If the schema is NULL, I don't think we need to do the require. How about this?
#8
We can probably go as far as confirming that $current has a count() >= 1, just to be extra safe.
#9
#305430: Prevent schema corruption was a duplicate.
#10
Here's a new patch, including count()
#11
I believe this to be a very dangerous practice. Where will this stop? How many lines of code will we add which shields us from bad modules? There are many, many ways for a broken module to mess up your Drupal. This is summed up as "we do not babysit broken code".
#12
Can you cast it to an array?
<?php$current = (array) module_invoke($module, 'schema');
?>
That's not even 1 line of babysitting code - it's 7 characters.
#13
#14
Agreed with Robert. Casting to an array is the way to go.
Plus I don't see how the patch in #10 solves the issue described in the original post ("passing NULL to array_merge() results in NULL").
#15
casting is fine. it's a bit of ugly but yeah ok.
#16
Casting sounds fine to me if that's easier. Probably needs a comment line to clarify it then, too.
I don't see that as babysitting broken code as much as keeping a typo from totally hosing an entire devel site. :-)
#17
New patch, casting to an array.
#18
So, I intentionally hacked poll_schema(). It threw a couple of warnings but felt good about itself and said it was installed. I installed profile at the same time as my hacked poll, and profile installed just fine. In that regards, I think the patch works fine.
fwiw, I'm with chx on this one, b/c people really just shouldn't be implementing hook_schema() unless they intend it to return a valid schema array. And if for some reason a schema fails to install, I don't think the module should even be marked as installed.
#19
This is not along a critical path, and while one module dying because its author messed up is one thing, screwing up other modules is simply not acceptable. #17 looks good to me.
#20
Sorry but that comment could be a bit more explanatory. Setting back to 'code needs work'.
#21
Argh, this bit me when I stuck a placeholder hook_schema() in a module. Such an easy fix.
Attached original patch with expanded comment. Also see #617546: Invalid hook_schema() result can trash entire schema for the D6 patch.
#22
Safety from bad hooks++
#23
I can't believe this little issue has required 6 patches (all basically identical) and 23 comments. Thank you, crell, for the RTBC.
<chant>commit, commit, commit</chant>
#24
Comment looks good.
In general, I am against babysitting broken code as well, but Crell in #19 makes a good point that this doesn't just screw up your module (as when you forget to return $items from hook_menu()), this screws up everything.
Committed to HEAD.
Looks like this could be backported to D6 as well?
#25
I can't express in words how much I concur that it should be backported to D6. Patch attached.