Override hash_salt when empty vs not set.

[adamzimmermann]

I recently encountered an issue where my Platform environment was throwing an error 500 after updating to the latest suggested code from Drupal's default.settings.php in my settings.php file.

The issue is from this line:

$settings['hash_salt'] = '';

• https://github.com/drupal/drupal/blob/9.4.x/core/assets/scaffold/files/default.settings.php#L252

---

This causes the settings.platformsh.php hash salt logic to think a value is already set due to the way the ?? operator works.

• https://github.com/platformsh-templates/drupal9/blob/master/web/sites/default/settings.platformsh.php#L111

It returns its first operand if it exists and is not null; otherwise it returns its second operand.

• https://www.php.net/manual/en/migration70.new-features.php#migration70.new-features.null-coalesce-op

I can see the case for why you don't want to override any value, even if that value is an empty string. However, if this was just an unwanted/unnoticed side-effect of how ?? works, then I think this fix would help others.

[apotek]

👍

Drupal ships with the hash_salt set to an empty string.

$settings['hash_salt'] = '';

Which means drupal breaks on platform.sh if the code is relying on NULL in order to get the salt from platform sh.

In the end, it seems platform.sh would want the code to grab the salt for any empty or non-truthy value it has been set to. This code change adds robustness.

[gilzow]

@adamzimmermann :
In this situation, if I am understanding correctly, Drupal expects something for $settings['hash_salt'] so it needs to be set and not be an empty string, correct? If it isn’t set, or it is an empty string, we need to set $settings['hash_salt'] to the value of the $paltformsh->projectEntropy, correct?

In your suggested change, won't an existing $settings['hash_salt'] be overridden by $platformsh->projectEntropy and an empty $settings['hash_salt'] be set to ''?

If we know Drupal will for sure set $settings['hash_salt'] to a default of '', it looks like instead we could make it
$settings['hash_salt'] = $settings['hash_salt'] ?: $platformsh->projectEntropy;
so that if it contains a non-empty existing value, we leave it alone, otherwise set it to $platformsh->projectEntropy?

[adamzimmermann]

In this situation, if I am understanding correctly, Drupal expects something for $settings['hash_salt'] so it needs to be set and not be an empty string, correct?

Correct. ✅

---

If it isn’t set, or it is an empty string, we need to set $settings['hash_salt'] to the value of the $paltformsh->projectEntropy, correct?

Correct. ✅

---

In your suggested change, won't an existing $settings['hash_salt'] be overridden by $platformsh->projectEntropy and an empty $settings['hash_salt'] be set to ''?

You are correct. I mixed up the logic (I'm updating my commit now). 🙈

It should be:

$settings['hash_salt'] = empty($settings['hash_salt']) ? $platformsh->projectEntropy : '';

---

If we know Drupal will for sure set $settings['hash_salt'] to a default of '',

We don't know that for sure. That is what is in the latest suggested code, but it is on the user to update their code and this is a file tracked in Git, not something included via Composer, so $settings['hash_salt'] might not be set.

---

it looks like instead we could make it $settings['hash_salt'] = $settings['hash_salt'] ?: $platformsh->projectEntropy; so that if it contains a non-empty existing value, we leave it alone, otherwise set it to $platformsh->projectEntropy?

This is definitely more clean, and I almost suggested this too, but then was concerned it might throw an error if $settings['hash_salt'] was not defined. If we can confirm this isn't an issue, then I would be all for it!

The PHP docs aren't super conclusive.

• https://www.php.net/manual/en/language.operators.comparison.php

But from some quick googling, I think my concern is valid.

• https://stackoverflow.com/questions/6052763/php-undefined-index-using-ternary-operator

[apotek]

it looks like instead we could make it $settings['hash_salt'] = $settings['hash_salt'] ?: $platformsh->projectEntropy; so that if it contains a non-empty existing value, we leave it alone, otherwise set it to $platformsh->projectEntropy?

This is definitely more clean, and I almost suggested this too, but then was concerned it might throw an error if $settings['hash_salt'] was not defined. If we can confirm this isn't an issue, then I would be all for it!

Can I throw a curveball here? It seems to me that if a client is using platformsh, then why would they not want platformsh to clobber whatever hash they may or may not have set up? Is there any valid use-case where someone might need to have or manage their own salt? I think we can cut through a lot of ambiguity if this code simply does this:

$settings['hash_salt'] = $platformsh->projectEntropy

Boom. Done.

Now, if there is a use case for wanting to let a user manage their own salt, then yes, we basically want to say "if you have a salt, and it is not empty, then we'll leave it alone but if you don't have a salt or you have an empty salt (default drupal install), we'll make sure you have one".

ie

$settings['hash_salt'] = (array_key_exists('hash_salt', $settings) && !empty($settings['hash_salt'])) ? $settings['hash_salt'] : $platformsh->projectEntropy;

or, more readably (letting the defined and non-empty condition simply sail on by unmodified):

if (!array_key_exists('hash_salt', $settings) || empty($settings['hash_salt'])) {
  $settings['hash_salt'] = $platformsh->projectEntropy;
}

But this is where things get really silly. Because, if we allow for an end user to manage their own salt, then it seems to me we should allow them to have an empty salt, if that's what they want, and how are we going to know whether the salt is empty because they they left it that way, or because they actually want it to be empty?

So, in the end, I feel like platformsh should assume that a user wants their salt to be $platformsh->projectEntropy, and if they really really don't want that, aren't there plenty of means for them to override, in a subsequent settings file, what platformsh set it to? Ie, let's just set it, and if a user really wants to override it, they have the ability to do so by setting it later on in the cacophony of settings files that nowadays exist in drupal :)

[adamzimmermann]

👏🏼 That thinking makes sense to me, but I was trying to respect the logic that was there, but perhaps it should be rethought.

[gilzow]

But this is where things get really silly. Because, if we allow for an end user to manage their own salt, then it seems to me we should allow them to have an empty salt, if that's what they want, and how are we going to know whether the salt is empty because they they left it that way, or because they actually want it to be empty?

I don't think we can. We can only tell if it is empty, not why or how it was set to empty.

@apotek my initial suggestion was going to be

$settings['hash_salt'] = empty($settings['hash_salt']) ? $platformsh->projectEntropy : $settings['hash_salt'];

which is essentially what you proposed, but changed it since I wasn't anticipating someone removing the value completely from settings.php. The above suggestion will at least catch situations where $settings['hash_salt'] is not set at all (which would throw an error like in the initial screenshot), or when it's an empty string. We can then add documentation that if the user really wants it to be an empty string, they can either comment out the line above, or reset it to empty at line 39 in settings.php but that they should consult Enhance hash_salt documentation in default.settings.php before doing so.

If this makes sense and is acceptable, either @adamzimmermann can update this PR or I can create a new PR with the above change. Either way we'll update the docs to reflect the change.

[adamzimmermann]

PR updated!

Thanks for the feedback and the thoughtful discussion. I think we landed in a good place.

[chadwcarlson]

Thanks @adamzimmermann !

[apotek]

Thanks @adamzimmermann !