drupal_get_bootstrap_phase() is broken

OK to reproduce this, truncate {cache} then visit any number of node/n pages but not the front page. This will also be the same for any 'entity' page like user/n, taxonomy/term/n etc.

Here's what happens:

menu_custom_theme() calls menu_get_item(), menu_get_item() loads the objects for that page, this calls node_load(), which calls drupal_schema_fields_sql()

This eventually gets down to drupal_get_schema(). On a cache miss, drupal_get_schema() caches on these conditions:

 if (!empty($schema) && (drupal_get_bootstrap_phase() == DRUPAL_BOOTSTRAP_FULL)) {
        cache_set('schema', $schema);
      }

menu_custom_theme() is called while we're still officially at DRUPAL_BOOTSTRAP_LANGUAGE, because it actually happens inside _drupal_bootstrap_full(), and the phase is set after that's completed.

This means that the schema will never be cached until you hit a page which both requires a schema hit, and calls it after _drupal_bootstrap_full() has completed, which includes the default front page in HEAD, but not many others I can think of.

We probably wouldn't have noticed this unless field_sql_storage_schema() was doing a bad SQL query, but it also means we load all .install files which is a big memory hog.

This patch is a bit ugly but fixes it, along with a bad drupal_function_exists() revert found at the same time - we just set $completed_phase immediately before running the bootstrap phase, instead of afterwards, so that operations which occur during the full bootstrap, know that they're in that rather than the previous one. This is how things happened in D6.

Note this also means that any function called during hook_init() currently gets DRUPAL_BOOTSTRAP_LANGUAGE if they consult drupal_get_bootstrap_phase(). That's probably a good way to test this thinking about it.

Tracked down the change to #370454: Simplify page caching - up until then, we set the phase prior to executing it, not afterwards - that looks to be a side-effect of the code there, not its intention.

So while we probably want to do it differently to what's in the current patch, that's the main bug here.

Re-titling, uploading proper patch with a test, and also the test separately - should have one failure.

chx reviewed in irc, changing the ternary back to an if.

It shouldn't be wrong, but I can reproduce the test failures locally with the patch, and not without it.

drupal_set_message() suppresses page caching, the page cache tests depend on system_test.module.

Since it was broken for a good 2-3 months, and caused a serious regression, without anyone noticing apart from via repeated and diverse benchmarking on a completely unrelated patch by more than 2-3 people, I thought it was worth having a test. However I agree the test is a bit weird / hacky and wouldn't be sad if it's removed - the proper test to catch things like this is #638078: Automated performance testing for core.

chx asked for a change from $completed_phase to $stored_phase - since setting that before we actually execute the phase is a bit strange. internal variable name only. I've also attached a version with and without the test.