Create $config_directory_name during a D7 => D8 upgrade

Looking at the code and I agree this is an issue. Moving to a task because this is an actionable item.

Initial patch version to create confi folder on update.

Failures are related to the lack of config folder being created for simple test.

#2: config_update.patch queued for re-testing.

The more I think about this the more I wonder how we are going to test it
* Issue is that we need to file the writable to update
* if we are running simple test we don't want to update settings.php because of sandboxing issues
* The code also needs to ensure that settings is writtable
* We can't rely on variable_get as we should assume it is going away.

We could wrap the update code in

if ($test_prefix = drupal_valid_test_ua()) {

but then of course this code will never be tested.

Wrapping update for simpletest runs.

What is the issue with sandboxing and writing settings.php? There must be other places that test writing settings.php during the upgrade process, what do those do? I'd like to get this resolved so we can move forward with #1464554: Upgrade path for image style configuration

In drupal 7, the rewrite of settings.php is controlled by the $update_rewrite_settings global, which is controlled by update_prepare_d7_bootstrap in the case of simple test

 if (empty($databases) && !empty($db_url)) {

Will be false and as a result update_fix_d7_requirements will not update settings.php unless I am missing something as my understanding is that simpletest uses the exist settings.php for an install and then does this

$this->databasePrefix = Database::getConnection()->prefixTables('{simpletest' . mt_rand(1000, 1000000) . '}');

to prefix tables. It almost seems like in case of CMI that simpletest iteslf would need to enable the ability to write permissions on settings.php and then add a config entry ($conf['simpletest_config'] to leave the master site settings in place.

After reading some other issues I wonder if we could rename settings.php in setUp() and then make a copy of it, set the file to be 777 updated the config variable, set it to read only and then delete at the end of the test and restore the 'master' file when processing is finished. Ideally we should be able to place the config file in simpletest dir and do some sort of overriding but not sure if that will work, as that would give us clean in terms of file deletion for free. I'll try and play around with concept over the next couple of days.

No, simpletest does not touch settings.php. (It's not even able to.)

#1364798: Impossible to generate meaningful diffs of upgrade db dumps will improve the upgrade path testing, but equally won't be able to touch settings.php of the D8 site.

config_get_config_directory() already contains a tweak for test requests, so the config system has a config directory to write to (also during upgrades).

#8 looks acceptable to me, once the other gaps have been filled in.

Changing status off the back of #1585844: Add config system settings to settings.php on upgrade from D7

Bumping to critical since this feels like genuine release blocker, unless we do #1052692: New import API for major version upgrades (was migrate module).

#1576322: page_cache_without_database doesn't return cached pages without the database proposes a way to allow SimpleTest to affect settings.php

I think considering we have a number of test cases that need to do this we should do something about it.

1. +      $settings['config_signature_key'] = array(
   +        'value'    => drupal_hash_base64(drupal_random_bytes(55)),
   +        'required' => TRUE,
   +      );
   

   The signature key no longer exists due to other issues, so it can be removed.

2. +      $settings['config_directory_name'] = array(
   +        'value'     => 'config_' . drupal_hmac_base64('', session_id() . $settings['config_signature_key']['value'] . variable_get('drupal_hash_salt', FALSE)),
   

   This should now be 'config_' . drupal_hash_base64(drupal_random_bytes(55)) (per #1464944: Installation of configuration system fails in some cases which was recently committed). If we're using it more than one place now, it might make sense to have a small helper function.

   Also, I think it should take into account the possibility that someone has added $config_directory_name to settings.php themselves already.

3. +      drupal_rewrite_settings($settings);
   

   We can't use this since it will overwrite whatever customizations the D7 site had in its settings.php file. (Or at any rate, we can't unless we fix #445012: Prevent drupal_rewrite_settings() from overwriting customizations made to settings.php first.)

4. Don't we need some code to take into account the fact that settings.php might not be writeable (in fact, almost certainly is not writeable, assuming the D7 site was configured correctly)? See update_prepare_d7_bootstrap() for how that was handled in D6->D7.
5. I'm also wondering if some of the above can be simplified by putting this in system_requirements() rather than update_fix_d8_requirements(). The latter is only needed for things that will cause Drupal to fail early in the bootstrap (before DRUPAL_BOOTSTRAP_SESSION) if they aren't updated for D8 first.

   Currently, at least, the config system doesn't fall into this category; if you try to do a D6->D7 update you make it to the requirements page just fine (and it warns you that you can't proceed without defining $config_directory_name in settings.php by hand). Although maybe as time goes on that's expected to change?

...if you try to do a D6->D7 update you make it to the requirements page just fine (and it warns you that you can't proceed without defining $config_directory_name in settings.php by hand)

This also makes me wonder if (now that #1464944: Installation of configuration system fails in some cases has been committed) the current issue is actually critical anymore.

There is no longer anything actually broken, I think; the D7->D8 update will not let you proceed until you edit settings.php by hand and make it work correctly.

On the other hand, requiring all sites upgrading from D7 to figure this out (and add a proper config directory name which is sufficiently-obscured to be secure) is not that exciting either, so maybe we should leave it.

...if you try to do a D6->D7 update you make it to the requirements page just fine...

I was on a time warp while writing the above comments; that's supposed to say D7->D8, of course.

I was trying the upgrade path and I needed to add a directory by hand. I just used 'temp' because I hadn't a clue what I should add.

Add the end of the upgrade process I received these warnings:

Update #8009
Failed: Drupal\Core\Config\FileStorageException: Failed to write configuration file: sites/default/files/temp/system.cron.yml in Drupal\Core\Config\FileStorage->write() (line 97 of C:\xampp\htdocs\upgrade\test3\core\lib\Drupal\Core\Config\FileStorage.php).

Still critical IMO

I'm on a windows machine btw

I made a patch, took some time to figure this stuff out... Hopefully this one is better than the previous one. It also includes the fact that settings.php needs to be writable.

This one is cleaner, cleaned the globals.
Didn't test this one.

Btw there was a bug with the current function

update_extra_requirements($requirements);

never got called, so I fixed it, don't think we should open a new issue just for that...

Hmmm the directory still isn't created...
I hoped the system would do it once I defined the name.

This one created the damn direcotry and no more warnings when upgrading!

I used the D7 settings.php check less this code

<?php
if (empty($databases) && !empty($db_url)) {
?>

and a database function call. I thought it was something we didn't need someone needs to verify it..

+++ b/core/includes/update.incundefined
@@ -113,6 +129,21 @@ function update_prepare_d8_bootstrap() {
+        $config_directory = config_get_config_directory();
+        $result = file_prepare_directory($config_directory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
+      }

Do I need to check if $config_direcotry_name exists now? And if it doesn't what do we have to do?

What if file_preare_directory fails? Should we do anything with the result.

-3 days to next Drupal core point release.

Here is a different approach that reuses some code from install.core.inc (untested)

Ok this time I tried the upgrade ;)

I can add a test that verifies the config directory is created.

#29: 1500312-fixed-config-upgrade-29.patch queued for re-testing.

Talked with sun and chx on irc and apparantly a test is blocked on #1576322: page_cache_without_database doesn't return cached pages without the database.
They agreed this could go in without a real test for now after some good group manual testing.

So start testing!

1) Install a drupal 7 site,
2) Upgrade with a patched drupal 8
3) Check after upgrade if a config directory is created in sites/default/files

I tested it and there were no errors what so ever.

My setup is a Mac with MAMP.

Can you verify that a folder prefixed with "config_" is created in the sites/default/files directory?

Yes it is, and has the same name as $config_directory_name in settings.php

This has been tested by a few people also in combination with #1464554: Upgrade path for image style configuration
The only problem that we noticed was caused by a non writable files directory but that will be handled in #1674052: update.php shows WSOD if files/ is not writable.

So as far as manual testing goes this is good to go. Can I have a technical review (and a possble rtbc).
Thnx :)

I had a longer look at this, and wasn't really happy with the idea that the new helper function rewrites settings.php not only for the config directory, but also for all other settings in the Drupal installer. That mix made the logic really hard to follow. Instead of that, we should rewrite settings.php twice, if needed.

Which brings me to my next point: The code logic tried to rewrite settings.php, even if the $config_directory_name is defined already. Untangling the logic as mentioned above prevents us from doing that.

I relocated the two general helper functions in install.inc to come right after drupal_rewrite_settings(), in order to achieve at least some sane function grouping - thus, the interdiff is a bit larger than the actual changes are.

I also adjusted update_prepare_d8_bootstrap(), since it only attempted to check/create the config directory in case settings.php is writable. I don't think that binding/dependency is true - especially, if your settings.php contains a $config_directory_name already. In that case, update_prepare_d8_bootstrap() does not require a writable settings.php, but still needs to ensure that the config directory exists and is writable.

Lastly, there is one thing I did not adjust, but which irks me a bit -- the new update_prepare_d8_bootstrap() requires settings.php to be writable, which was not the case before. It might be fine to require that. I'm merely thinking of alternative upgrade paths; e.g., using Drush or whatever, in which settings.php might be fully prepared for a D8 bootstrap already and does not actually have to be writable.

+++ b/core/includes/update.inc
@@ -88,6 +88,18 @@ function update_prepare_d8_bootstrap() {
+  $settings_file = conf_path() . '/settings.php';
+  $writable = drupal_verify_install_file($settings_file, FILE_EXIST | FILE_READABLE | FILE_WRITABLE);
+  $requirements = array(
+    'settings file' => array(
+      'title' => 'Settings file',
+      'value' => $writable ? 'The settings file is writable.' : 'The settings file is not writable.',
+      'severity' => $writable ? NULL : REQUIREMENT_ERROR,

Essentially, I think we need to make this additional requirement conditional - based on a (potentially increasing) list of settings, which either have to exist in settings.php already, or if they do not, then settings.php needs to be writable.

Thoughts?

Essentially, I mean this.

$settings_exist = TRUE;
$settings_exist = $settings_exist && !empty($GLOBALS['config_directory_name']);

Why not just

$settings_exist = !empty($GLOBALS['config_directory_name']);

In general I agree with everything said above. And yes we should make the additional requirement conditional.

Lastly, there is one thing I did not adjust, but which irks me a bit -- the new update_prepare_d8_bootstrap() requires settings.php to be writable, which was not the case before.

I just copied the code form the D6 to D7 upgrade. SO in fact we are just doing the same as in previous upgrades, but yes they have a conditional test for this to there. So with the additional test we should be ok :).

THANK YOU for the review and the fixes.

@aspilicious: Perhaps it is premature, but I wanted to write it in a way that makes it clear for everyone else that other new/changed settings can/should be added to this condition.

@all: I'm afraid the recent changes are larger and we need to manually test again. :-/ Perhaps it makes sense to wait for another review of the latest patch (to make sure we don't have to test a third time), dunno...

I tested both of the patches posted by Sun. Neither of them produced any errors, and both of them created /sites/default/files/config_random_stuff/

Alright, I tested the patch from #40 and received no errors. I checked and there was a config directory created in /sites/default/files.

I should add, I followed all of the steps for manual testing from

1) Install a drupal 7 site,
2) Upgrade with a patched drupal 8
3) Check after upgrade if a config directory is created in sites/default/files

And received the results I wrote about in #44.

Ok so we just need to fix the first point of me in #41. I can do this tomorow.

It's an ugly interdiff but should point out the additonal changes are safe. I think this is rtbc...

Thanks!

Sorry, forgot to post on this issue yesterday. I tested this and can confirm it's good.

Thanks, office hours testers! :) @damiankloip, @hosef, @ryanissamson

Thanks for the manual testing. Committed/pushed to 8.x.