part of #1956180: [meta] Remove global variables

CommentFileSizeAuthor
#14 interdiff.txt4.98 KBsun
#14 global.hashsalt.14.patch6.94 KBsun
#12 drupal8.global-hash-salt.12.patch5.05 KBsun
#7 2036259-7.patch3.84 KBdamiankloip
#7 interdiff-2036259-7.txt536 bytesdamiankloip
#1 drupal-drupal_hash_salt-2036259-1.patch3.32 KBParisLiakos
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Active » Needs review
FileSize
drupal-drupal_hash_salt-2036259-1.patch3.32 KB
ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

not sure about upgrade path, should i defer to #1921822: Take account of drupal_hash_salt during migration path from 7.x ?

Edit: yeap its in the second point of the issue summary there

Status: Needs review » Needs work

The last submitted patch, drupal-drupal_hash_salt-2036259-1.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Related issue #1555862: DrupalWebTestCase::drupalGetToken() does not add hash salt

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

#1: drupal-drupal_hash_salt-2036259-1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-drupal_hash_salt-2036259-1.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
interdiff-2036259-7.txt536 bytes
2036259-7.patch3.84 KB

We also need to make the change in install.core.inc, then I think this should be ok.

Status: Needs review » Needs work

The last submitted patch, 2036259-7.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review

#7: 2036259-7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2036259-7.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Assigned: Unassigned » chx

Spoke to chx on IRC, we briefly looked into debugging this. There are no real errors in the logs, so at the moment we have no more info than the qa result page...

Assigning to chx at his request - he is going to take a look and see what's going down.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Assigned: chx » sun
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +API clean-up
FileSize
drupal8.global-hash-salt.12.patch5.05 KB

Due to #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead, this patch should pretty much work as-is now.

Re-rolled against HEAD and converted new code that accesses the global variable.

Status: Needs review » Needs work

The last submitted patch, 12: drupal8.global-hash-salt.12.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
Related issues: +#2176621: Remove global $databases
FileSize
global.hashsalt.14.patch6.94 KB
interdiff.txt4.98 KB
  1. Fixed drupal_rewrite_settings() does not import newly written settings into Settings singleton.
  2. Removed the now obsolete/unnecessary 'drupal_' prefix from setting name.
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Parent issue: » #1956180: [meta] Remove global variables

Status: Needs review » Needs work

The last submitted patch, 14: global.hashsalt.14.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

14: global.hashsalt.14.patch queued for re-testing.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

Good job, its green!:)

Very minor note:

  1. +++ b/core/includes/install.inc
@@ -199,8 +200,14 @@ function drupal_rewrite_settings($settings = array(), $settings_file = NULL) {
+  $settings_settings = array();

    maybe we could have a better name for this variable?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review

Heh, yeah, it was very hard to find an accurate variable name in there, because the entire function is about $settings. I considered $new_settings, $settings_variables, $settings_values, but all of those would be even more ambiguous within the scope of that function body. ;)

Thus, the $settings in settings.php are the $settings_settings :-)

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Related issues: +#1921822: Take account of drupal_hash_salt during migration path from 7.x

heh, yeah:)
Added a draft change notice here:
https://drupal.org/node/2197037

It is good to go imo, but dunno if i can rtbc it

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Any change to move forward here? :-)

This patch contains changes that conflict with #2176621: Remove global $databases, and in turn, the latter contains a fix that currently blocks #1351352: Distribution installation profiles are no longer able to override the early installer screens

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs review » Reviewed & tested by the community

Looks great - Drupal installs successful with this patch and generates a hash salt in seetings.php as expected.

+++ b/core/includes/bootstrap.inc
@@ -1662,10 +1662,13 @@ function drupal_get_user_timezone() {
+  // This should never happen, as it breaks user logins and many other services.
+  // Therefore, explicitly notify the user (developer) by throwing an exception.
+  if (empty($hash_salt)) {
+    throw new \RuntimeException('Missing $settings[\'hash_salt\'] in settings.php.');
+  }

Nice!

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Yes, this looks great. This was held up for ages. Amen to #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented

I have updated the CR to indicate that it is better to use drupal_get_hash_salt() in both D7 and D8, rather than accessing the variable/setting directly.