Problem/Motivation

A new top-level global for settings.php was added which moved a few things out of $conf.
None of the settings got an upgrade path. Not all of them need one, but a few do. This upgrade path needs to be added.

Proposed resolution

These values would need to be pulled from variables and written out to settings.php, so the best way to accomplish this is with #1921818: Modify drupal_rewrite_settings() to allow writing $settings values

Remaining tasks

#1921822-17: Take account of drupal_hash_salt during migration path from 7.x recommends

  1. Edit UPGRADE.TXT to instruct people to rename their settings.php to settings.d7.php
  2. Add code to upgrade process to pull drupal_hash_salt and $databases values and write them to new settings file.
  3. Detect if your conf_path() is writable and do it automagically for you if so.

User interface changes

None

API changes

None

#1833516: Add a new top-level global for settings.php - move things out of $conf
#1921818: Modify drupal_rewrite_settings() to allow writing $settings values

Original report by heyrocker

When #1833516: Add a new top-level global for settings.php - move things out of $conf was committed, none of the settings got an upgrade path. Not all of them need one, but a few do. This upgrade path needs to be added. These values would need to be pulled from variables and written out to settings.php, so the best way to accomplish this is with #1921818: Modify drupal_rewrite_settings() to allow writing $settings values. This issue is currently postponed on that one.

Files: 
CommentFileSizeAuthor
#41 take_account_of-1921822-41.patch1.09 KBdimaro
#11 1921822-settings-upgrade-path-11.patch1.04 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View
#9 1921822-settings-upgrade-path-9.patch242.09 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#7 1921822-settings-upgrade-path-7.patch1.05 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 53,126 pass(es). View
#2 1921822-settings-upgrade-path-2.patch1.02 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 52,380 pass(es), 26 fail(s), and 130 exception(s). View

Comments

heyrocker’s picture

Status: Postponed » Active

This is unblocked now.

heyrocker’s picture

Status: Active » Needs review
FileSize
1.02 KB
FAILED: [[SimpleTest]]: [MySQL] 52,380 pass(es), 26 fail(s), and 130 exception(s). View

I think this should work. Do we need upgrade path tests? I have yet to write those yet but I suppose I should. I did test it manually with both scalar and array values ('reverse_proxy_addresses' and 'proxy_exceptions' are arrays).

Status: Needs review » Needs work

The last submitted patch, 1921822-settings-upgrade-path-2.patch, failed testing.

webchick’s picture

Issue tags: +Needs tests

Yeah, I definitely think we do, unless it's absolutely impossible.

chx’s picture

Issue tags: -Needs tests

You can't write a test that changes settings.php.

chx’s picture

you wanted if (!empty($settings)) drupal_rewrite_settings($settings); to avoid a notice and needless work.

heyrocker’s picture

Status: Needs work » Needs review
FileSize
1.05 KB
PASSED: [[SimpleTest]]: [MySQL] 53,126 pass(es). View

Ah aye.

beejeebus’s picture

Status: Needs review » Needs work

this doesn't look right.

+  foreach ($upgrade as $variable_name => $setting_name) {
+    $variable = update_variable_get($variable_name, '');
+    if ($variable) {
+      $settings['settings'][$setting_name] = (object) array(
+        'value' => $variable,
+        'required' => TRUE,
+      );
+    }
+  }
+
+  if (!empty($settings)) {
+    drupal_rewrite_settings($settings);
+  }
+}

$upgrade's keys are all simple ints, so $variable_name will be 0, 1, 2, ....

i guess it should just be

foreach ($upgrade as $variable_name) {

or was the intention to provide meaninful keys?

heyrocker’s picture

Status: Needs work » Needs review
FileSize
242.09 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

When I first wrote this I had a keyed array with variable name/setting name, thinking there might be cases where the two differ. When there weren't I lopped off the keys in the array but didn't change the loop. Good catch!

heyrocker’s picture

Status: Needs review » Needs work

uh that patch is borked, will u/l a new one in a bit

heyrocker’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View

That's better

beejeebus’s picture

looks good to me. not going to RTBC it until we get another reviewer.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.installundefined
@@ -2195,6 +2195,38 @@ function system_update_8051() {
+    $variable = update_variable_get($variable_name, '');

update_variable_get() does not consider $conf overrides. So this doesn't work for things that were defined in $conf only. There will also be things that aren't $conf at all, e.g. $drupal_hash_salt.

We either need to change that or check it ourself. Also not sure what to do with the existing $conf snippets in settings.php that we're replacing with this.

catch’s picture

Hmm I actually think we don't need an upgrade path for this - if you're running a site which needs them, then you're capable of updating them yourself - they're code rather than user data.

Not changing status and others might feel differently, but fwiw.

Berdir’s picture

I'm not sure myself, but at least *some* of them need an upgrade path. For example, drupal_hash_salt and $databases, if we change them to $settings too.

catch’s picture

OK those two do indeed and it'd be great to move those two to $settings as well so well kill off globals altogether (except maybe $conf).

heyrocker’s picture

Just trying to brainstorm some ideas since I really think we want to automatically upgrade all these vars if possible.

In UPGRADE.txt we already have the following:

   Sometimes an update includes changes to default.settings.php (this will be
   noted in the release notes). If that's the case, follow these steps:

   - Make a backup copy of your settings.php file, with a different file name.

   - Make a copy of the new default.settings.php file, and name the copy
     settings.php (overwriting your previous settings.php file).

   - Copy the custom and site-specific entries from the backup you made into the
     new settings.php file. You will definitely need the lines giving the
     database information, and you will also want to copy in any other
     customizations you have added.

So we're already instructing people to overwrite their old settings file with the new one. What if we change the first step to say

   - Make a copy of your settings.php file and name it settings.d7 (note it must be exactly named settings.d7 for the upgrade to work)

and just remove step three. If we know what the source of the old settings file is, then we can grab the old entries, upgrade them, and write into the new file. We could even detect if your conf_path() is writable and do it automagically for you if so.

chx’s picture

settings.d7.php you mean, but yes, I might take this one if noone else does; the database upgrade will need special casing in the new supersimple drupal_rewrite_settings function ;)

YesCT’s picture

http://drupal.org/node/1427826 has instructions for updating issue summaries and using the issue summary template

using the template might be helpful, also looks like some good comments to incorporate there.

nategasser’s picture

Issue summary updated @ Drupalcon Portland

nategasser’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

removing needs tag

Shellingfox’s picture

Status: Needs work » Needs review
Issue tags: -D8 upgrade path, -Configuration system, -settings system

Status: Needs review » Needs work
Issue tags: +D8 upgrade path, +Configuration system, +settings system

The last submitted patch, 1921822-settings-upgrade-path-11.patch, failed testing.

webchick’s picture

Issue tags: +beta blocker

Tagging all critical D8 upgrade path issues as "beta blocker."

catch’s picture

Title: Add upgrade path for $settings » Take account of drupal_hash_salt during migration path from 7.x
Issue tags: -beta blocker

Since the migration based API will use a fresh 8.x install to migrate into, there's no need to migrate $databases, and anything else in settings.php can be configured by the person setting up the new site.

Except for drupal_hash_salt - it's absolutely necessary that old passwords continue to work etc. so that needs to be an explicit step. I think we can either require that it's copied across manually, or rewrite it if we can - but one or the other has to happen.

catch’s picture

Category: bug » task
catch’s picture

Status: Needs work » Postponed

Postponed on at least the initial framework from #1052692: New import API for major version upgrades (was migrate module).

xmacinfo’s picture

Why not keep automated upgrades as much as possible? Not all site should require manual settings and manual database migration.

I still believe that many Drupal sites upgrading to Drupal 8 would still benefits from using the regular upgrade steps and use update.php instead of doing everything manually.

catch’s picture

Component: other » migration system
catch’s picture

Issue summary: View changes

formatting

catch’s picture

Priority: Critical » Major
Issue summary: View changes

Still important to figure out, but no longer blocks release since migratation path doesn't.

mgifford’s picture

I think this has been fixed with #2125717: Migrate in core: patch #1

jhedstrom’s picture

Are folks agreed that this is fixed with #2125717: Migrate in core: patch #1?

mikeryan’s picture

Migration from Drupal 7 has not yet been committed to core - there's been some work on it in the IMP sandbox, but I don't see anything involving drupal_hash_salt. And I don't see how it could do anything, since drupal_hash_salt is defined in the legacy site's settings.php, not in the database we're migrating from - the D8 instance has no way to automatically obtain the value. There's going to need to be a manual step documented, methinks... At the very least, perhaps instructions to the upgrader to copy the D7 drupal_hash_salt and paste into the migrate_upgrade form.

mikeryan’s picture

See #2397849: Export migration configuration via services for a possible approach - a contrib-implemented service on D7 to provide drupal_hash_salt (and any other code-based configuration).

jhedstrom’s picture

Status: Postponed » Active

This is probably no longer postponed.

Berdir’s picture

Issue tags: -D8 upgrade path

No need for the upgrade path tag here.

alexpott’s picture

Issue tags: -Configuration system

This is not part of the configuration system.

benjy’s picture

quietone’s picture

Issue tags: +Needs reroll, +migrate-d7-d8
catch’s picture

Issue tags: +Migrate critical
dimaro’s picture

Status: Active » Needs review
Issue tags: -Needs reroll
FileSize
1.09 KB

Rerolled #11.

Status: Needs review » Needs work

The last submitted patch, 41: take_account_of-1921822-41.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

This issue needs a completely different direction from #41 - that was based on a time when we upgrade d7 to d8 in place.

Except for drupal_hash_salt - it's absolutely necessary that old passwords continue to work etc. so that needs to be an explicit step. I think we can either require that it's copied across manually, or rewrite it if we can - but one or the other has to happen.

xjm’s picture

Status: Needs work » Closed (duplicate)
Issue tags: -migrate-d7-d8, -Migrate critical

Discussed with @alexpott, @weal, @mikeryan, and @benjifisher. This is actually a duplicate of #2598038: Invalid passwords after D7 to D8 migration -- that issue cannot be resolved without this, and it doesn't make sense to discuss the solution separately.

The rerolled patch in #41 is not relevant as @alexpott says.

alexpott’s picture

Status: Closed (duplicate) » Needs work
Issue tags: +Needs issue summary update

We need to work out if we care if a Drupal 7 site that is being migrated to Drupal 8 has a different hash salt in settings.php. (Note that user passwords do not use this salt). This salt is used to generate things like CSRF tokens - or things like this:

function google_analytics_user_id_hash($uid) {
  return Crypt::hmacBase64($uid, \Drupal::service('private_key')->get() . Settings::getHashSalt());
}
function _update_manager_unique_identifier() {
  $id = &drupal_static(__FUNCTION__, '');
  if (empty($id)) {
    $id = substr(hash('sha256', Settings::getHashSalt()), 0, 8);
  }
  return $id;
}

If a site migrates it's hash salt then we can determine who is moving from 7 to 8 on d.o...

However given the difficulties of doing anything automatic - a migration cannot have access to both settings.php - I think this issue might be a docs issue.

quietone’s picture

Issue tags: +migrate-d7-d8

Adding tag for Drupal source and destination version.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

Priority: Major » Normal

@alexpott: Any updated thoughts on this? Just docs, or is there anything else to do here?

Thanks.