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.

Files: 
CommentFileSizeAuthor
#26 287647_0.patch1020 bytesDéja
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 287647_0_0.patch.
[ View ]
#25 287647.patch1020 bytescafuego
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 287647_0.patch.
[ View ]
#21 287647.patch1.05 KBcafuego
Passed: 14496 passes, 0 fails, 0 exceptions
[ View ]
#17 null_schema_cast.patch932 bytesmikejoconnor
Failed: Failed to apply patch.
[ View ]
#10 null_schema.patch1.08 KBmikejoconnor
Failed: Failed to apply patch.
[ View ]
#7 null_schema.patch1.06 KBmikejoconnor
Failed: Failed to apply patch.
[ View ]
#4 issue-287647-4.patch972 byteslilou
Failed: Failed to apply patch.
[ View ]
get-schema-null.patch847 bytesbjaspan
Failed: Failed to apply patch.
[ View ]

Comments

bjaspan’s picture

Status:Active» Needs review
moshe weitzman’s picture

This is true of theme registry as well. I think a little defensiveness like this is a good thing.

lilou’s picture

Status:Needs review» Needs work

CNW because drupal_get_schema() move into boostrap.inc.

lilou’s picture

Status:Needs work» Needs review
StatusFileSize
new972 bytes
Failed: Failed to apply patch.
[ View ]

Status:Needs review» Needs work

The last submitted patch failed testing.

lilou’s picture

mikejoconnor’s picture

StatusFileSize
new1.06 KB
Failed: Failed to apply patch.
[ View ]

If the schema is NULL, I don't think we need to do the require. How about this?

Crell’s picture

We can probably go as far as confirming that $current has a count() >= 1, just to be extra safe.

Damien Tournoud’s picture

mikejoconnor’s picture

StatusFileSize
new1.08 KB
Failed: Failed to apply patch.
[ View ]

Here's a new patch, including count()

chx’s picture

Status:Needs review» Closed (won't fix)

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".

robertDouglass’s picture

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.

robertDouglass’s picture

Status:Closed (won't fix)» Needs work
Damien Tournoud’s picture

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").

chx’s picture

casting is fine. it's a bit of ugly but yeah ok.

Crell’s picture

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. :-)

mikejoconnor’s picture

Status:Needs work» Needs review
StatusFileSize
new932 bytes
Failed: Failed to apply patch.
[ View ]

New patch, casting to an array.

rszrama’s picture

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.

Crell’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+Quick fix

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.

Dries’s picture

Status:Reviewed & tested by the community» Needs work

Sorry but that comment could be a bit more explanatory. Setting back to 'code needs work'.

cafuego’s picture

Status:Needs work» Needs review
StatusFileSize
new1.05 KB
Passed: 14496 passes, 0 fails, 0 exceptions
[ View ]

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.

Crell’s picture

Status:Needs review» Reviewed & tested by the community

Safety from bad hooks++

bjaspan’s picture

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>

webchick’s picture

Version:7.x-dev» 6.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

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?

cafuego’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new1020 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 287647_0.patch.
[ View ]

I can't express in words how much I concur that it should be backported to D6. Patch attached.

Déja’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new1020 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 287647_0_0.patch.
[ View ]

As this issue continues to plague Drupal 6.x nearly a year after this patch was submitted, I'm resubmitting cafuego's patch so that it can go through the automated testing.

I also discuss the issue and its solution in greater detail in my post at http://echodittolabs.org/blog/2010/09/butterfly-effect

grendzy’s picture

+1
Confirmed this is a straight backport of the D7 commit.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 287647_0.patch, failed testing.

grendzy’s picture

Status:Needs work» Reviewed & tested by the community

bad bot! This is a D6 patch.

Gábor Hojtsy’s picture

Status:Reviewed & tested by the community» Fixed

I'm not fun of babysitting code either, but there is just too much support here. Committed to Drupal 6.

Status:Fixed» Closed (fixed)
Issue tags:-Quick fix

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