Standardize the return values of updates
dww - August 15, 2007 - 16:11
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | update system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
It's silly that update.php assumes each update *must* return an array. Many DB updates don't actually run queries (at least not directly, stuff like variable_del() or menu_rebuild(), etc) and forcing everyone to return an empty array() is rather silly. This has caused numerous issues to get filed against various projects that happened to forget the (IMHO pointless) step of "return array()" at the end of an update that doesn't do any log-able queries:
http://drupal.org/node/159757
http://drupal.org/node/167549
This is crazy. update.php should just be patched so that when an update doesn't return array, it doesn't blindly try to array_merge() it.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| update_php_no_blind_array_merge.patch.txt | 644 bytes | Ignored | None | None |
#1
Looks good, and fixes the issue (that seems to only appear on PHP5).
#2
Here's a version for HEAD/D6.
#3
And here's another version for HEAD that fixes a bogus double ;; in nearby, related code (totally unrelated to the bugfix, but it's so minor that a separate issue/patch seems silly).
#4
The 6.x version works fine as well.
#5
IMO it's very sloppy for us to have functions that return one data type under certain conditions and another data type on others, and we do this entirely too often. The best practice is for this to be consistent and predictable, always, so there's never any question. The docs state that this hook returns an array value. And in fact, even when the module doesn't do any queries at all, a $ret[] = t('Did such and such thing to this other deal here.') to communicate what happened is definitely not the worst thing in the world.
On the other hand, I agree that developers are constantly getting bitten by this, so I guess I can see the argument, although I'd prefer it if we didn't commit this. However, if this patch is committed, the hook_update_N() docs need to be updated to reflect the change.
#6
@webchick: unfortunately, the array you return can't just include a message like that. it has to be like so:
$ret[] = array('success' => TRUE, 'query' => "variable_del($variable)");I do that all the time on queries that don't directly use update_sql()... But, this seems like a kludgy hack, using 'query' for this human-readable message.
Anyway, point being, sometimes the updates don't return arrays, and it's annoying that core generates PHP errors in this case. It's not like this code happens on every page load, so the usual "we shall not bloat core and slow everyone down to deal with stupid contrib maintainers" does not apply. This seems like a trivial case of defensive programming for core...
But, luckily, it's not up to either webchick or myself to decide. ;)
#7
Ok. Then what about something like this?
Let's rename that index to 'message' with the aim to make it mandatory that update functions self-document what they're doing, rather than smudging up our API to cater to developers who don't read docs. (and fwiw, I say this as someone who's forgotten that little return array() before a couple times, myself :P)
#8
Ahem. ;) How about one that doesn't cause a parse error? ;)
This is for 6.x, btw since it's an API change (ish).
#9
The original patch was RTBC-ed, but possibly better patches were submitted later. It would be better to discuss this IMHO. The original quickfix is quite trivial, but surely does not allow for clean status messages for updates where update_sql is not used.
#10
I think we can do better than "The following status messages were returned," which is a bit technical.
#11
I still think #2 and/or #3 are the way to go. webchick's idea is nice and all, but it really *is* an API change (s/query/message/) and it seems like it's probably too late in D6 to be making changes like that to the DB update system. My patches above are tiny, solve the reported problem, and still apply cleanly... The bigger problem of making the update system more developer friendly for updates that don't use update_sql() (and for that matter, fixing update_sql() so it supports query placeholders, etc) are tasks that should probably be postponed until 7.x. But fixing the bug I reported is still valid for 6.x and 5.x (as per the version of this issue).
#12
Sorry, I should have been more specific: #2 or #3 is for HEAD/6.x, the patch from the original issue post is still valid for 5.x. All of which apply cleanly (and are RTBC if you ask me, but they're my patches, so I'll let someone else pull that trigger). Thanks.
#13
#14
updates can now return false. no idea if that helps here
#15
@moshe: do you have an issue nid about that change? thanks.
#16
http://drupal.org/node/208602. seems unrelated now that i look closer.
#17
Finally done in another issue. updates don't return an array anymore.
#18
Automatically closed -- issue fixed for 2 weeks with no activity.