Often there are contrib or user errors that break Drupal administration functionality. Once the underlying error is fixed, a cache flush is needed to clear these errors.

One example of this is: #997716: Upgrading D6 to D7: leftover D6 Garland block.tpl.php breaks site completely

It use to be possible to use update.php to flush the cache, but due to UI enhancements, this is no longer possible. I have personally used this maybe 100 times in D6 to flush the caches.

This leaves no options other than creating a custom update script or to manually flush the caches via sql. Not something that most users would be able to do.

So maybe there needs to be an option to continue even when there are no updates pending or an alternative mechanism for doing this.

Marking as a bug as it is a regression from D6 (sorry this is brief, typing one handed with a broken humerus)

Files: 
CommentFileSizeAuthor
#21 update-cache-flush-d7-1049284-20.patch4.03 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 36,983 pass(es).
[ View ]
#14 update-cache-flush-combined-1049284-14.patch4.09 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,804 pass(es).
[ View ]
#12 update-cache-flush-test-only1049284-12.patch3.63 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 33,804 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#6 update-cache-flush-1049284-6.patch490 bytesxjm
FAILED: [[SimpleTest]]: [MySQL] 33,738 pass(es), 24 fail(s), and 46 exception(es).
[ View ]
#5 update-cache-flush-1049284-4.patch477 bytesxjm
PASSED: [[SimpleTest]]: [MySQL] 33,803 pass(es).
[ View ]

Comments

Dave Reid’s picture

That's because the D6 update.php allowed you to run updates even when you shouldn't have been able to. Regardless, it might be wise of us to add a call to drupal_flush_all_caches() on the 'No updates available' screen as I've often recommended 'make sure you run update.php to clear your caches' before.

Alan D.’s picture

Version:7.0» 7.x-dev

Fixing assignment to dev.

catch’s picture

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

This is very closely related to #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap. Leaving as major bug report since it's a regression from D6.

xjm’s picture

Status:Active» Needs review
StatusFileSize
new477 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,803 pass(es).
[ View ]

So currently update_finished() calls drupal_flush_all_caches(). All we want to do here is add it in the case where there are no pending updates?

This doesn't actually seem like a bug to me, but I guess if running update.php is expected to clear caches no matter what, it could be considered a regression.

Is the attached all that's needed?

xjm’s picture

StatusFileSize
new490 bytes
FAILED: [[SimpleTest]]: [MySQL] 33,738 pass(es), 24 fail(s), and 46 exception(es).
[ View ]

Or, do we want to move this to the first screen of update.php as in #534594-80: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap? That would mean caches get cleared automatically whenever someone with access visits update.php, and they'd get cleared a second time if updates were run. This would be part of a solution for #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap; the above patch would not be because it runs later.

Status:Needs review» Needs work

The last submitted patch, update-cache-flush-1049284-6.patch, failed testing.

xjm’s picture

Status:Needs work» Needs review

Okay, that's a little too early. The 500 errors happen locally as well. Similar breakage also happens if I move a full cache clear to update_info_page() (where some cache clearing is happening already). So, based on that, I think #5 is the correct solution and this does not actually overlap #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap.

sun’s picture

Issue tags:+Needs tests

#5 looks acceptable to me.

Since this is a non-obvious expectation, we need to add a test for this. The test needs to assert that a cache entry no longer exist after running update.php (without updates). Alternatively, implement hook_flush_caches() in a (possibly existing) testing module and set a message in there, which in turn should be displayed on the update.php finished page.

xjm’s picture

Note that if #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap does add a full bootstrap to registry_rebuild(), we can probably consolidate the cache clears a little so that pieces of them don't run multiple times.

Edit: I take this back; flushing the menu and bootstrap caches in the middle of running update.php is not a good idea.

xjm’s picture

Assigned:Unassigned» xjm

Looking into a test. UpdateScriptFunctionalTest::testRequirements() seems like good place to add this, since it already tests the various paths the upgrade script can take. Edit: and update_script_test.module is the obvious place to add the hook_flush_caches().

xjm’s picture

StatusFileSize
new3.63 KB
FAILED: [[SimpleTest]]: [MySQL] 33,804 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Attached test addition fails locally without #5, and passes with that fix applied.

If this were a real module I'd use placeholders in that message t(), but it's not worth the decrease in readability for a fake module.

Aside: Is there a reason for UpdateScriptFunctionalTest to be in system.test instead of update.test?

Status:Needs review» Needs work

The last submitted patch, update-cache-flush-test-only1049284-12.patch, failed testing.

xjm’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new4.09 KB
PASSED: [[SimpleTest]]: [MySQL] 33,804 pass(es).
[ View ]

Failed as expected. Now the combined patch, which should pass.

sun’s picture

Status:Needs review» Reviewed & tested by the community

Awesome!

webchick’s picture

Assigned:xjm» catch

I would like to leave this one to catch to commit, since he spent so much time in the upgrade system during D7.

(Yoinking. Sorry, xjm!)

Dave Reid’s picture

This looks great!

catch’s picture

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

This happens after a full bootstrap has already run, so it should be fine (last time I said this ctools got broken of course).

Committed/pushed to 8.x, this is the sort of patch that would make sense to have in 7.x at least a clear 10 days before releasing a point release I think, so better to get it into 8.x earlier rather than later in the month.

Patch does not affect the status of #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap which is when we can't run a full bootstrap at all.

xjm’s picture

Backporting this to D7 is nontrivial because UpdateScriptFunctionalTest::testRequirements() does not exist in D7, nor does update_script_test.module.

Edit: We just need to wait on #951644: Requirement warnings (e.g. for PHP memory limit) are not shown on install or update unless there is a requirement error also.

xjm’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new4.03 KB
PASSED: [[SimpleTest]]: [MySQL] 36,983 pass(es).
[ View ]

Alright, 7.x is back in synch now. :) Here's a D7 patch which needs to visit testbot.

xjm’s picture

Status:Needs review» Reviewed & tested by the community

Just to clarify, the first paragraph in #19 was incorrect--this patch extends tests that at that time were committed to D8 and RTBC for D7. They've been committed to D7 now as well, so the patch is a straight backport and I think it is still RTBC.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

I'm happy to see this functionality restored. Test coverage is great, too.

I guess the only reason not to do it would be if it could be a potential DDOS attack vector, but this is only happening on the second page of the updater, after access to the script has been determined already.

Committed and pushed to 7.x. Thanks!

xjm’s picture

I guess the only reason not to do it would be if it could be a potential DDOS attack vector, but this is only happening on the second page of the updater, after access to the script has been determined already.

Yeah, I double-checked that before I made the patch. It's part of the test coverage in a different test case, as well.

Status:Fixed» Closed (fixed)
Issue tags:-needs backport to D7

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