Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The config check in install_begin_request() always fails for me during Drush site reinstall. The bug is not specific to Drush though. The contents of $config are all the yml files in module owned directories and config.storage gets back an InstallStorage object. This is a bug - we want it to check that the active storage is empty.
In addition, the code comment is wrong, since the default storage is database now.
// Ensure that the active configuration directory is empty before installation
// starts.
if ($install_state['config_verified'] && empty($task)) {
$config = \Drupal::service('config.storage')->listAll();
if (!empty($config)) {
$task = NULL;
throw new AlreadyInstalledException($container->get('string_translation'));
}
}
Comment | File | Size | Author |
---|---|---|---|
#19 | 2247281-19.patch | 5.85 KB | pwolanin |
#14 | interdiff.txt | 779 bytes | sun |
#14 | install.settings.14.patch | 5.79 KB | sun |
#11 | interdiff.txt | 1014 bytes | sun |
#11 | install.settings.10.patch | 5.78 KB | sun |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedComment #2
pwolanin CreditAttribution: pwolanin commentedThis might behave better.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedThis fixes the exception for me.
Comment #4
sunOne problem with this is essentially the concern that I had mentioned in #2161591-85: Change default active config from file storage to DB storage already: The new config storage attempts to automatically create the storage, no?
It's not clear to me why this error is triggered via Drush, but not in the regular interactive installer, nor in tests (which are using the non-interactive installer)...?
How can this error be reproduced?
Comment #5
pwolanin CreditAttribution: pwolanin commented@sun - I was getting this intermittently in the ui too. Not sure how to test though.
the listAll() method does NOT create the storage (by design). Here's the code:
Comment #6
tstoecklerI'm consistently when trying to reinstall as well. I just commented out the throw call in the OP and Drupal installed fine.
Comment #7
sun@pwolanin: Ah, sorry, I wrongly remembered that part.
It is still not clear to me why I'm not able to reproduce this issue locally, and why our existing interactive installer tests are not encountering it either.
So either something is wrong with our existing tests, or we're missing test coverage for the scenario here. — What is the scenario here?
Some of you referred to "reinstall", but how exactly? With or without existing settings.php? Does settings.php contain a db connection? $config_directories? Is the database empty?
Separate from that, the non-interactive installer can only reach this code path, if the config storage exists ahead of time already. Since #2161591 that no longer means the active config directory, but the {config} database table.
This means that not only
$install_state['config_verified']
must be TRUE, but$install_state['database_verified']
must also be TRUE. Both must be TRUE already when the installer is initially invoked, becauseinstall_begin_request()
is only entered once in the non-interactive installer.It's not clear why
$install_state['config_verified']
is TRUE to begin with?Comment #8
tstoecklerRight by reinstall I mean I have a settings.php with $databases and $config_directories (that exist). Previously that prompted the database settings form, which I just had to re-submit and Drupal would install. Now I get the "Drupal already installed." I'll try to re-produce again later.
Comment #9
sun@moshe weitzman clarified in IRC:
That scenario is not covered by tests yet. We have an
InstallerExistingSettingsTest
, but it is a misnomer, since the preconfigured settings.php only contains database settings (but nothing else).Attached patch renames the existing test and adds a new one to test a fully populated settings.php. It fails for me locally, and is expected to fail.
Comment #11
sunIncorporating #2.
Comment #12
pwolanin CreditAttribution: pwolanin commentedThanks for adding the test.
Comment #14
sunOops.
Comment #15
pwolanin CreditAttribution: pwolanin commentedThis test looks basically good, but I think it loses test coverage for one path to installation? why don't we add this new one instead of replacing the existing one?
Comment #16
sunSorry for the confusion, I have renames=copies enabled. Both tests are retained; the diff file headers clarify:
Comment #17
moshe weitzman CreditAttribution: moshe weitzman commentedoh, i missed those headers too. rtbc then.
Comment #19
pwolanin CreditAttribution: pwolanin commentedHere'a re-roll.
Comment #20
tstoecklerYup.
Comment #21
catchCommitted/pushed to 8.x, thanks!