Posted by dww on October 4, 2010 at 4:45pm
4 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | update.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | dww |
| Status: | closed (fixed) |
Issue Summary
Splitting this off from #918808: Standardize block cache as a drupal_render() #cache ...
Thanks to #240873: Move custom help settings to blocks hook_help() is now treated as a block. Given block caching, blocks shouldn't produce side effects like drupal_set_message() (regardless of the specific outcome of the debate at #918808). So, the way update.module injects its drupal_set_message() calls into various admin pages is fragile and broken.
Comments
#1
#2
That's a simple fix.
#3
Committed to CVS HEAD. Thanks.
#4
Bah. ;) That's what I get for trying to a) deal with this stuff late at night after having a flu for a week, and b) not providing tests. We're now getting duplicate messages on all the Update status and Update manager UI pages, which is pointless, since you're already looking at pages that are either warning you or you're already in the middle of trying to fix it.
Here's a test. This should fail 9 assertions.
Next comment will be the same test with a patch that fixes it.
#5
This should be all green.
#6
Actually, here's a slightly better version of update_init() so we don't incur the cost of calling update_requirements() if we're not actually going to use them. Not sure it's feasible to really test this aspect, so I'm just leaving the same test from #4 to catch the UI bug.
#7
<?phpif (arg(1) == 'appearance' || arg(1) == 'modules') {
- foreach (array('core', 'contrib') as $report_type) {
- $type = 'update_' . $report_type;
- if (isset($status[$type]['severity'])) {
- if ($status[$type]['severity'] == REQUIREMENT_ERROR) {
- drupal_set_message($status[$type]['description'], 'error');
- }
- elseif ($status[$type]['severity'] == REQUIREMENT_WARNING) {
- drupal_set_message($status[$type]['description'], 'warning');
+ if (arg(2) == NULL) {
?>
You can fold the two conditions together, and the second one should be arg(2) === NULL.
#8
Good catch, thanks! You should have set to CNW. ;)
#9
Yay.
#10
Actually, I think this is a much clearer way to write update_init() that will be less error-prone and easier to maintain in the future.
#11
Committed to CVS HEAD. Thanks.
#12
Thanks Dries. Sorry about the extra work with the follow-up patch... Yay for tests. ;)
#13
Automatically closed -- issue fixed for 2 weeks with no activity.