Problem/Motivation
When I set up a custom class for PHPStorage by setting, for example, by setting a custom directory for compiled twig like:
$settings['php_storage']['twig'] = array(
'directory' => '/var/tmp/mysite/php_storage/twig',
);
I get an undefined index (and really should be an exception):
Notice: Undefined index: secret in Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage->__construct() (line 62 of core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php).
Proposed resolution
Merge in default settings instead of making site owners set the secret and class
Remaining tasks
review patch
decide if we need to throw an exception if the secret is empty
User interface changes
n/a
API changes
n/a
Data model changes
n/a
original report:
I have a fresh install of Drupal 8.0.3 that is reporting the following:
Notice: Undefined index: secret in Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage->__construct() (line 62 of {path}/docroot/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php).
Is this an error on my end? Or just a silent error in drupal?
Comment | File | Size | Author |
---|---|---|---|
#16 | increment.txt | 1010 bytes | pwolanin |
#16 | 2669418-16.patch | 3.86 KB | pwolanin |
#13 | increment.txt | 3.64 KB | pwolanin |
#13 | 2669418-13.patch | 3.85 KB | pwolanin |
#10 | 2669418-10.patch | 1.35 KB | pwolanin |
Comments
Comment #2
yobottehg CreditAttribution: yobottehg commentedWe also have this message on all Acquia Cloud hostings. We reported that, but they could not find the error.
Comment #3
dawehnerThat is a clear sign of the sadness of Acquia Cloud to not write the hash salt into settings.php on install, see
core/lib/Drupal/Core/PhpStorage/PhpStorageFactory.php:47
Afaik they don't allow settings.php to be writeable at all, so you better do it on your own :)
Comment #4
michaellander CreditAttribution: michaellander commentedI might be misunderstanding, but we setup our site outside of Acquia, and have a hash_salt defined, and are getting the same error only on our Acquia environment. I've also tried defining the hash_salt again after including the Acquia stuff, but still the same issue. I do think it's an Acquia thing, but am not entirely clear what I'm missing.
Comment #5
kurund CreditAttribution: kurund as a volunteer and at Web Access India Pvt. Ltd. commentedComment #6
kurund CreditAttribution: kurund as a volunteer and at Web Access India Pvt. Ltd. commentedI have tested this issue but I am not able to replicate this on the vanilla install of Drupal running on 8.0.x branch. This might be Acquia environment related issue hence, can be closed.
Comment #7
kurund CreditAttribution: kurund as a volunteer and at Web Access India Pvt. Ltd. commentedComment #8
chrisyawman CreditAttribution: chrisyawman commentedI came across this issue only after I modified my $settings['php_storage'] array in the settings.php. I resolved it by adding the following at the end of the settings.php file:
$settings['php_storage']['twig']['secret'] = $settings['hash_salt'];
Comment #9
bneva CreditAttribution: bneva as a volunteer commentedI feel like manually setting the Twig secret in settings.php is only treating the symptom.
I think there are three solutions, but I don't know the Core well enough to know about any side effects.
1. Modify \Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage such that the constructor either ignores or prepends the hash_salt when it pulls the secret from the $configuration variable. (e.g., $this->secret = $configuration['secret']; should be $this->secret = isset($configuration['secret']) ? : Settings::getHashSalt() -or- '';)
2. Modify \Drupal\Core\PhpStorage\PhpStorageFactory to automatically add the hash_salt to the $configuration array if one is not set. Perhaps log this as an error if it's not set.
3. Modify \Drupal\Core\Template\TwigPhpStorageCache (or wherever else Twig's Storage Cache sets its configuration) to include the provided hash_salt.
Comment #10
pwolanin CreditAttribution: pwolanin as a volunteer commentedComments #8 and #9 seem basically correct to me.
We already have a check for:
isset($configuration['class'])
This patch just merges in the 2 required keys instead of having an else clause.
Comment #11
pwolanin CreditAttribution: pwolanin as a volunteer commentedComment #13
pwolanin CreditAttribution: pwolanin as a volunteer commentedmm, how about this. Doesn't break the test, and adds a little more coverage (and converts 2 deprecated method calls).
Comment #14
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI also encountered this problem recently on a site hosted on Acquia Cloud, and the patch makes sense :)
Comment #15
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedJust one nit though:
This message is copied from below, so it's not really accurate for this assertion.
Comment #16
pwolanin CreditAttribution: pwolanin as a volunteer commentedThanks - here's the patch with a fix for the test text.
Comment #18
alexpottCommitted ed78a4c and pushed to 8.0.x, 8.1.x and 8.2.x. Thanks!
Comment #21
alexpott