Installer starts with fatal error/exception "table 'semaphore' not found" if settings.php contains $databases already

Hm. Digging further into this ultimately brings me back to my original patch in #1798732-137: Convert install_task, install_time and install_current_batch to use the state system

Installation fails, because the 'keyvalue.memory' service is unknown.

If you refresh the error page (new HTTP request), then you get the error message "Table 'variable' already exists."

The DI container (or several services on it) has to be swapped out mid-request, so as to make services us the freshly created database tables - or alternatively, the installer has to redirect after installing base system database tables, so that the new HTTP page request boots the regular runtime service container (using the database).

The previous suspicion was only partially correct. drupal_install_system() swaps out the minimal service container with a regular DrupalKernel. Albeit not as cleanly separated as in my original patch, that existing code appears to work as intended.

The problem is caused much earlier in the request: install_begin_request(), one of the very first functions being invoked to set up the base environment before the actual main installer logic is executed, checks whether settings.php exists, config directories exist, and a database connection can be established. If all conditions are met, then the regular DrupalKernel is initialized.

However, those three conditions are not sufficient; the services in the regular container additionally need a persistent storage; i.e., the database tables of the base system schema. Due to that, the installer breaks with "table not found" errors.

The proposed fix is to add a new verification "base_system_verified" to the early environment setup, which essentially tests whether the last defined table in the System module schema exists in the database. Only if that is the case, a regular DrupalKernel is used.

The patch also replaces a Settings overloading/reversion hack for the minimal service container, which exists to make the installer use memory implementations for all key/value stores (instead of database implementations). It is sufficient to register the key/value memory factory services in the minimal container. The only reason for why that was not possible before is that various other services are type-hinting the KeyValueFactory class as opposed to a KeyValueFactoryInterface.

Lastly, this patch also fixes the error message "table 'variable' already exists", which occasionally appeared under certain edge-case conditions (e.g., using the Back button of your browser, or by reloading an early installer page), and which was caused by the same condition that also caused the regular DrupalKernel to be used; i.e., the installer does not know whether and when the base system has been installed.

Sorry, the KeyValueFactory changes turned out to be not really relevant here, so I've split that out into #2156265: KeyValueFactory is swappable, add an interface and fix type-hints

All of the test failures appear to be DUTB tests. The DUTB base class implements a similar key/value memory storage hack like the installer (but differently).

By splitting that fix into #2156265: KeyValueFactory is swappable, add an interface and fix type-hints, it appears we introduced a hard dependency on that interface to get committed, in order to move forward here... :-/

I was confused in #9 — the failing tests are web tests, not DUTB tests.

Meanwhile I was able to confirm that merging the KeyValueFactoryInterface patch back into this patch here fixes the fatal errors in web tests.

Hence, waiting for that patch to get committed.

Until then, reviews and feedback is welcome :)

It's green :)

I'm not sure whether any further explanations for the proposed changes are missing (besides those I gave in comments above already). Please let me know if anything is unclear.

re: @larowlan: Blatant removal of lengthy comments

I added those lines myself when the configuration system and the Config\InstallStorage was introduced originally.

Core has changed and advanced a lot since then (primarily: Settings), and so the details covered in that comment are mostly obsolete now.

The main point the comment tries to explain is that the installer essentially boils down to two discrete phases:

1. Pre-persistent storage (before System module schema has been setup)
2. Post-persistent storage (→ regular DrupalKernel)

The very first installer screens are operating in (1), which means that they cannot use a regular DrupalKernel, because plenty of services would need to be overridden in order to work in an "empty" environment (no settings, no config, no database, no storage). → Every HTTP page request rebuilds all necessary bootstrap information from scratch.

But as soon as (2) is reached and the base system database tables exist (persistent storage for base system services à la cache, k/v, lock, config, etc), we are, in fact, operating in a regular Drupal environment.

As a testament to that, some time ago, I was able to install Drupal 8 with System module only. Obviously resulted in pretty much blank pages only, but that's to be expected, isn't it? :)

However, if you complete the train of thought, the much bigger effect of (2) is that the entire remaining operations of the installer are happening in a regular Drupal environment. As a consequence, you can simply install/enable modules as in a regular environment, because you're operating in a normal Drupal system → no more special-casing for the installer.

We could even turn the remainder of the installer into a module of its own (which could then be swapped out by distros/install-profiles), but that's a debate of its own and not to be discussed here.

That said, the majority of the above explanation is irrelevant for this issue and patch here. The only parts that matter are:

1. Introduce a dedicated $install_state flag to signify that the base system is operable.
2. As long as that is not the case, the installer has to use the minimal installer container and rebuild everything from scratch on every request.
3. As soon as the base system is operable, we can boot a regular DrupalKernel.

Doing so fixes the reported bug:

Even if settings.php, $databases, and $config_directories exist, that does not mean that the base system is operable (and persistent storage is available).

install_begin_request() is enhanced with a new environment condition that checks whether the base system tables have been installed already.

Since this check is required upfront to make the decision of whether to boot a regular DrupalKernel or build a minimal installer service container, it has to happen very early in the request. For that reason, we try to query the database, and we simply wrap that check into a try/catch block.

I noticed that we have a $install_state['database_tables_exist'] key already that had similar in purpose, but is unused.

As a final clean-up step, attached patch removes the unused and obsolete 'database_tables_exist' flag, because the unique condition we're looking for really is that the base system is operable. We do not want to know whether individual pieces like database tables and whatnot have been set up, but instead, we need to know whether the base system is fully operational or not.

Any further issues with this patch? Or can we move forward here? :)

The existing patch is back to green - testbot fluke.

Why not check all tables?

Checking all tables would needlessly slow down the installer — if the last table defined in system_schema() has been created, then we can be reasonably sure that all other (previous) tables have been installed as well.

That is, because hook_schema() definitions are processed in the order in which they are defined. In other words, if any of the tables could not be created, then the last table was not created either.

This argumentation is not meant to say that the overall base_system_verified check could not be improved and made more solid in the future — I could actually see plenty of room for doing so, but for now, I'd like to focus on (1) fixing this concrete bug and (2) introducing/hardening the underlying concept of two discrete phases in the installer: pre-base-system and post-base-system.

Any other questions? :)

Re-rolled against latest HEAD, and:

+++ b/core/includes/install.inc
@@ -622,8 +622,16 @@ function drupal_verify_profile($install_state) {
 function drupal_install_system() {
-  // Create tables.
-  drupal_install_schema('system');
+  // Create base system tables (persistent storage).
+  // Since this task is executed in the early installer environment,
+  // install_run_tasks() cannot record that this task has been completed. Thus,
+  // the interactive installer invokes this task a second time and the fact that it
+  // completed can only be recorded after the second invocation.
+  $schema = drupal_get_schema_unprocessed('system');
+  $table = key($schema);
+  if (!db_table_exists($table)) {
+    drupal_install_schema('system');
+  }

Removed this change to drupal_install_system(), as I'm no longer sure whether it still applies (the base_system_verified condition should kick in already), and also, because it's technically outside of the scope of this issue here.

26: drupal8.reinstall.26.patch queued for re-testing.

I think I'd prefer a specific check for a given table than just arbitrarily taking the last one from the list, because that one could change from time to time, and might conceivably conflict with an existing table name if Drupal's being installed to another database w/ prefixes or whatever.

(I'm worried specifically about seemingly random testbot fails when testbot1 is testing against one version of a schema and testbot2 is testing against another changed by the patch it's working on, because they're testing two different table names for this check.)

It took me a few minutes to digest and understand this comment ;) I hope I understood it, and I don't think any of the concerns apply, and here's why:

1. Tests are operating in cleanly separated environments.

   They are executed in parallel on the same machine/testbot (the example of testbot1 vs. testbot2 does not apply), but our existing database table prefixing is robust enough to not result in table name clashes. If that would be an issue, then we would see random test failures today already (which we don't).

2. To my knowledge, we do not support changes to settings.php after starting the installer.

   That would be the only way to achieve the conflict in expectations being made; i.e., changing the 'prefix' in settings.php after hitting second or third installer page, after settings.php has been (re)written.

   By now, I have a relatively excellent understanding of Drupal's installer, and I'm fairly confident that none of the other code in the installer accounts for that case either.

   I would not recommend to support that edge-case in the first place.

   Just as of last week or so, someone filed a bug report against Drupal core, complaining that the installer blew up, after manually changing essential values (IIRC, $databases), and hitting the reload/back button in a browser was initiated a day before. Do we want to support that?

   I think the clear answer is No. → settings.php declares how Drupal will be installed. It either exists already or will be created automatically. But changing the declared settings in between installer steps is not something we can or want to support at any point in time. The entire installer would have to be rewritten to account for that, because e.g. the database (or config directories) could be manipulated (literally) at any point in time.

3. Based on that reasoning (which hopefully makes sense), checking the last defined table in system_schema() is absolutely sufficient.

   We're already accepting the extra LoC of checking the last defined table instead of the first, so as to ensure that all defined base system tables have been created (as they're created sequentially in the order they're defined). Sans the unsupported edge-cases outlined above, that is a simple, but yet, 100% complete "test" coverage that is possible within the early installer environment.

   Checking a specific table of the defined ones would have no benefit to the taken approach. If at all, it would only introduce bugs later on, in case there is a new table definition appended to system_schema() and if we'd forget update the installer accordingly. :)

Added docs to the case of catching the exception.