Manual upgrade path from 7.x to 8.x loses fixed module_list() inexplicably.

See #1292470-177: Convert user pictures to Image Field (177, really?). I think the most problematic point is that we're trying to access config() before it's set up because of config() calls *very* early in the bootstrap process.

So for a start, we need to move the order of things in update_prepare_d8_bootstrap() and move drupal_install_config_directories() and the necessary code above the database bootstrap. That did get me a few steps further until it died while running updates.

I guess that is also the main reason the tests are passing, because there we're setting up these directory ourself. So maybe we need to stop doing that and instead make sure that they're created in the correct directory for the test to reproduce this issue. No idea if that's possible.

I can't reproduce this locally. I also can't see the call to config() in _drupal_bootstrap_database() (and if there is one triggered somewhere that's a critical bug in itself). Managed to get all the way through the upgrade path with no fatal errors at least.

Here's what currently happens... (note, to do the update, I installed a 7.x site from a 7.x checkout, drush genc/t/m/u/v'ed a bunch of stuff, then git checkout 8.x).

When you go to index.php you get:

Fatal error: Uncaught exception 'Symfony\Component\DependencyInjection\Exception\InvalidArgumentException' with message 'The service definition "config.factory" does not exist.' in /Users/abyron/Sites/7.x/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php on line 690
( ! ) Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "config.factory" does not exist. in /Users/abyron/Sites/7.x/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php on line 690
Call Stack
#	Time	Memory	Function	Location
1	0.1090	4618296	_drupal_exception_handler( )	../bootstrap.inc:0
2	0.2015	9398296	error_displayable( )	../bootstrap.inc:2225
3	0.2015	9398424	config( )	../errors.inc:155
4	0.2016	9398552	Symfony\Component\DependencyInjection\ContainerBuilder->get( )	../config.inc:67

We should fail more gracefully there. Not a critical though, I don't think.

Go to update.php, WSOD.

Shift+reload update.php, still WSOD.

Now that's a critical. ;)

Nothing in php_error.log (apart from the error I got on index.php). I'm a little stuck.

catch, what did I do wrong, and/or how did it not break for you?

@webchick: The problem in the first error is that is happening *while* we are trying to fail more gracefully. The problem is that while catching an exception, we check a CMI setting and surprise, CMI can not be loaded, so we die with a completely wrong exception message. That's IMHO a major bug on it's own and also happens on normal page requests when you're messing with low-level Container stuff (like I'm doing for the cache backend and database in DIC isses).

I can't test it right now, but I did something like the attached patch to make sure that CMI directories are set up as the very first thing. Did then also get as far as BTMash.

For upgrading I did this:

- dumped the db that was in my 7.x checkout, not a lot in it probably.
- restored that to a new database.
- deleted the files/config directory
- upgraded 8.x direct from update.php

Not sure why it's not failing for me. I did try to get it to fail a few times.

@catch: Did you use your existing 8.x settings.php (which has config path's defined in already) or did you also copy your settings.php from 7.x over?

I used my existing 8.x settings PHP, perhaps that's it!

The patch above is nonsense, this one at least gets me through until the batch starts and then throws up.

Somehow system_list() returns object instead of strings, need to figure out why. It's possible that my changes are causing this, not sure.

Ok, so the reason for that is that the 7.x bootstrap cache contains stuff in a different format than we do.

The attached patch makes sure the bootstrap cache is flushed. There might be a better place for this, not sure.

@BTMash that might be possible, but would also completely disable the cache and require us to rebuild everything on every request and update.php can take a while to run, so that might be quite the slowdown.

IMHO, clearing the cache is the cleanest solution, I'm just not sure where exactly we should be doing it.

Ok, the notice happens because we somehow loose the static module list information again, possibly due to some of the bootstrap stuff happening in between the now moved initial set and and the call that causes it to throw that notice.

Still wondering if we can add test coverage for this. Will have a peek at the upgrade path test. I assume that BTMash's fancy git based upgrade path tests would be able to do that because we actually create a settings.php there? (do we?)

+++ b/core/includes/update.inc
@@ -87,10 +87,9 @@ function update_check_incompatibility($name, $type = 'module') {
+  drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION);

@@ -116,6 +115,23 @@ function update_prepare_d8_bootstrap() {
+    drupal_classloader();

DRUPAL_BOOTSTRAP_CONFIGURATION() sets up the class loader already, no?

+++ b/core/includes/update.inc
@@ -116,6 +115,23 @@ function update_prepare_d8_bootstrap() {
+  if (!$settings_exist) {
...
+    drupal_install_config_directories();

@@ -225,6 +231,11 @@ function update_prepare_d8_bootstrap() {
+      // Populate a fixed module list (again, why did it get lost?) to avoid
+      // errors due to the drupal_alter() in _system_rebuild_module_data().

I bet the reason for why it gets lost is the initial !$settings_exist condition. update_prepare_d8_bootstrap() is invoked for every page of update.php, so the module list override no longer happens after it was accessed once.

Overall, this change seems to be yet another "duct-tape" (no pun intended) for the actual problem:
#922996: Upgrade path is broken since update.php does not enforce empty module list

+++ b/core/includes/update.inc
@@ -193,20 +209,6 @@ function update_prepare_d8_bootstrap() {
-      // @todo Race-condition: drupal_install_config_directories() calls into
-      //   install_ensure_config_directory(), which calls into
-      //   file_prepare_directory(), whichs calls into file_get_stream_wrappers(),
-      //   which attempts to invoke hooks with a non-existing module/hook system.

Even with these changes, this @todo and race condition still exists, so I'd appreciate if this comment would be retained.

+++ b/core/includes/update.inc
@@ -218,6 +220,10 @@ function update_prepare_d8_bootstrap() {
+      // Make sure that the bootstrap cache is cleared as that might contain
+      // incompatible data structures.
+      cache('bootstrap')->flush();

I wonder whether it is safe to call into the Cache API here -- aren't there schema changes for cache tables? Wouldn't it be more safe to do a direct db_truncate('cache_bootstrap')->execute()?

Thank for the review.

- No idea about classloader, will have to check. I had to add it or it broke with fatal error CacheFactory not found.

- about the $settings_exist condition. Possible. But this code should only be executed on the first request anyway? Will check. And yes, this is duct-tape material.

- The removal of the comment was accidental, will add them back.

- Calling cache() is "safe", because I have to load cache.inc anyway earlier for the module_list() call. We can also switch that to a direct db_delete(), that's fine with me. Will do a re-roll later.

Yes, classloader is called, maybe I did add that before I added the bootstrap call. Needs to be verified manually because it's not covered by the tests, but should be ok to remove that.

Re-added the comment.

Spent a lot of time testing this patch this evening. Here's what I've found...

Without this patch

with config directories in settings.php - manual upgrade works
without config directories in settings.php - manual upgrade fails

With this patch

with config directories in settings.php - manual upgrade fails
without config directories in settings.php - manual upgrade works

We need upgrade to work regardless of whether the user has manually added the configuration to seetings.php as perhaps someone will set config directories before running an upgrade so that they'll be outside of webroot for security purposes.

The error that occurs with the patch installed and config directories in settings is that they might not be created yet and due to the changes they never are. Patch attached fixes this.

And it's not currently possible to add a test for this patch since simpletest uses the D8 settings.php no matter what you do. In order to test this patch using simpletest we'd have to do work on the writing of a simpletest settings.php file similar to the approach found on #1576322: page_cache_without_database doesn't return cached pages without the database

Considering the changes in #20 are so minimal - just adding a check for the existence of directories and trying to create them if they don't... and I did extensively test berdir's patch and it fixes the reported problem and now doesn't break when config_directories is already defined...

I think this rtbc!

I can confirm that #20 is RTBC, those changes make sense.

OK. The two duct tape hacks already have issues open #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap and #1331486: Move module_invoke_*() and friends to an Extensions class, let's try to clean things up when those are in.

We definitely need to support having config directories in settings.php - that'll happen if you re-run an upgrade after restoring a database dump for example, or if you're using any old random existing 8.x install to test the upgrade path like I did..

Committed/pushed to 8.x.

+++ b/core/includes/update.inc
@@ -225,6 +235,11 @@ function update_prepare_d8_bootstrap() {
+      // Populate a fixed module list (again, why did it get lost?) to avoid
+      // errors due to the drupal_alter() in _system_rebuild_module_data().
+      $module_list['system']['filename'] = 'core/modules/system/system.module';
+      module_list(NULL, $module_list);

Erm, this question of "why did it get lost?" should really have been figured out before commit.

I didn't add that to my earlier review, since it appeared obvious to me that it is a remaining task for the patch.

Let's re-open this to keep working on that then.

Let's see whether this works.

I think both things happen on the same request anyway, so that doesn't fix it and isn't the explanation why it doesn't. Something seems to be resetting the list? Maybe add a debug to module_list() and check who's calling it with what...

Fixing tag.

Moving back to fixed - we added UpdateModuleHandler in the interim which kills all this code.