Running update.php to flush caches no longer works unless there are updates pending

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.

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.

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?

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.

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.

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

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.

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

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?

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

Awesome!

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!)

This looks great!

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.

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.

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

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.

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!

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.