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.

CommentFileSizeAuthor
#10 931284-10.update-msg-no-duplicate-nagging.patch6.94 KBdww
#8 931284-8.update-msg-no-duplicate-nagging.patch6.27 KBdww
#6 931284-6.update-msg-no-duplicate-nagging.patch6.98 KBdww
#5 931284-5.update-msg-no-duplicate-nagging.patch6.53 KBdww
#4 931284-4.update-msg-no-duplicate-nagging.test-only.patch3.65 KBdww
#1 931284-1.update-msg-in-init-not-help.patch3.53 KBdww

Comments

dww’s picture

dww
we/he/they
commented
Status: Active » Needs review
StatusFileSize
new 931284-1.update-msg-in-init-not-help.patch3.53 KB
damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

That's a simple fix.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

dww’s picture

dww
we/he/they
commented
Status: Fixed » Needs review
StatusFileSize
new 931284-4.update-msg-no-duplicate-nagging.test-only.patch3.65 KB

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.

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 931284-5.update-msg-no-duplicate-nagging.patch6.53 KB

This should be all green.

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 931284-6.update-msg-no-duplicate-nagging.patch6.98 KB

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.

damien tournoud’s picture

damien tournoud commented 
     if (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.

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 931284-8.update-msg-no-duplicate-nagging.patch6.27 KB

Good catch, thanks! You should have set to CNW. ;)

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

Yay.

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 931284-10.update-msg-no-duplicate-nagging.patch6.94 KB

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.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

dww’s picture

dww
we/he/they
commented

Thanks Dries. Sorry about the extra work with the follow-up patch... Yay for tests. ;)

Status: Fixed » Closed (fixed)

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