Child ticket of #1775842: [meta] Convert all variables to state and/or config systems.

Original Goal

  • Convert the 'install_task' variables to the State system.

Problem

  • There is no state storage, unless the installer created the state storage.
  • The installer needs to use state(), but the state storage has to be in memory and swapped out with the regular database backend as soon as it is available.
  • The installer already contains a range of flags and measures to detect which storages/services are available, but they happen too late and are not sufficient to determine availability of the state database backend.
  • Another consequence of the current installer process is that System module is installed in a completely different way than any other module.

Proposed solution

  1. Split the installation process into two phases:
    1. Before {system} schema.
    2. After {system} schema.
  2. Before {system} schema, the entire installer operates in a mocked environment that is very similar to the one of DrupalUnitTestBase. The only "storage" that exists are HTTP request parameters.
  3. As soon as sufficient data has been collected through the initial installer UI, install the {system} schema.
  4. After {system} schema, the entire remaining installer process operates in a full environment without any mocked services and also a full, regular DrupalKernel.
  5. Use the only available storage, HTTP request parameters, to switch from the mocked/crippled environment to the full environment; i.e.:
    install.php?langcode=en&profile=standard&bootstrap=6.
  6. Install System module like any other module, so its bundle, services, everything else works like for any other module.
  7. (original goal) Replace the install_task & Co variables with state().

Notes

  • The revised interactive installer process needs to do a bootstrap step in between to properly set up session handling. We will eliminate this additional step when #1841198: Clean-up the session patch is ready, by treating and mocking the session service like other services.
CommentFileSizeAuthor
#180 interdiff.txt858 bytescatch
#180 install.patch4.21 KBcatch
#177 install_task_time.patch1.66 KBcatch
#177 combined.patch3.37 KBcatch
#174 drupal-split-installer-into-1798732-174.patch33.58 KBmarkhalliwell
#174 interdiff.txt2.61 KBmarkhalliwell
#173 drupal-split-installer-into-1798732-173.patch33.37 KBmarkhalliwell
#173 interdiff.txt32.44 KBmarkhalliwell
#165 drupal-split-installer-and-convert-install_task-1798732-165.patch32.68 KBjuanolalla
#161 drupal-split-installer-and-convert-install_task-1798732-161.patch27.46 KBjuanolalla
#157 druapl-installer_services-1798732-157.patch34.57 KBmbrett5062
#154 drupal-installer_services-1798732-154.patch34.29 KBmbrett5062
#152 druapl-installer_services-1798732-152.patch34.22 KBmbrett5062
#150 drupal-installer_services-1798732-150.patch34.19 KBmbrett5062
#137 installer.services.137.patch35.31 KBsun
#133 installer.services.133.patch35.31 KBsun
#133 interdiff.txt3.13 KBsun
#130 installer.services.130.patch36.36 KBsun
#130 interdiff.txt1.34 KBsun
#126 installer.services.126.patch36.55 KBsun
#123 installer.services.123.patch36.59 KBsun
#120 installer.services.120.patch36.6 KBsun
#120 interdiff.txt4.55 KBsun
#119 installer.services.119.patch37.69 KBsun
#119 interdiff.txt5.3 KBsun
#116 installer.services.116.patch36.49 KBsun
#116 interdiff.txt6.36 KBsun
#103 1798732_real_103.patch15.88 KBchx
#103 interdiff.txt1.22 KBchx
#102 1798732_real_102.patch14.88 KBchx
#102 interdiff.txt897 byteschx
#101 1798732_real.patch14.08 KBchx
#99 installer.services.99.patch39.22 KBsun
#99 interdiff.txt2.6 KBsun
#84 installer.services.84.patch37.39 KBsun
#74 installer.services.74.patch37.39 KBsun
#74 interdiff.txt1.52 KBsun
#72 installer.services-1798732-72.patch36.81 KBBerdir
#72 installer.services-1798732-72-interdiff.txt911 bytesBerdir
#70 installer.services.70.patch36.78 KBCameron Tod
#64 installer.services.64.patch37.29 KBsun
#64 interdiff.txt1.25 KBsun
#61 installer.services.60.patch37.07 KBjthorson
#54 installer.services.54.patch37.46 KBsun
#54 interdiff.txt5.45 KBsun
#52 installer.services.51.patch35.15 KBsun
#52 interdiff.txt1019 bytessun
#50 installer.services.50.patch34.66 KBsun
#50 interdiff.txt5.88 KBsun
#48 installer.services.48.patch32.61 KBsun
#48 interdiff.txt5.78 KBsun
#47 installer.services.47.patch32.05 KBsun
#43 installer.services.43.patch32.09 KBsun
#43 interdiff.txt26.36 KBsun
#42 drupal8.state-install-task.42.patch6.37 KBsun
#37 state.install.revert.20.patch5.41 KBsun
#28 1798732.drupal8.system-install-state-update.28.tests_only.patch4.23 KBalexpott
#28 1798732.drupal8.system-install-state-update.28.patch5.09 KBalexpott
#28 23-28-interdiff.txt2.62 KBalexpott
#23 drupal8.system-install-state-update.23.patch2.1 KBsun
#20 convert-install_task-variable-to-state-api-1798732.20.patch4.82 KBalexpott
#15 convert-install_task-variable-to-state-api-1798732.patch4.82 KBwestie
#15 interdiff.txt4.97 KBwestie
#11 drupal-convert-install_task-variable-to-state-api-1798732-10.patch4.75 KBDean Reilly
#11 interdiff.txt1.8 KBDean Reilly
#10 drupal-convert-install_task-variable-to-state-api-1798732-9.patch4.05 KBDean Reilly
#10 interdiff.txt764 bytesDean Reilly
#8 drupal-convert-install_task-variable-to-state-api-1798732-7.patch4.03 KBDean Reilly
#8 interdiff.txt762 bytesDean Reilly
#7 drupal-convert_install_current_batch-1798860.patch3.43 KBwestie
#7 interdiff.txt1.38 KBwestie
#1 drupal-convert-install_task-variable-to-state-1798732-1.patch2.64 KBDean Reilly
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dean Reilly’s picture

Title: Convert install_task and install_time variables to use state system » Convert install_task variable to use state system
FileSize
2.64 KB

First attempt. I replaced a query to the variables table with a state()->get() too and it seems to work ok but wouldn't mind someone taking a second look.

Dean Reilly’s picture

Status: Active » Needs review
alexpott’s picture

Status: Needs review » Needs work

We need to provide a system_update_N to migrate the install_time variable to state.

alexpott’s picture

And install_task :)

alexpott’s picture

And install_task :)

alexpott’s picture

Issue tags: +State system

Tagging

westie’s picture

Patch to also upgrade variables to the state system for install_current_batch

Dean Reilly’s picture

Status: Needs work » Needs review
FileSize
762 bytes
4.03 KB

Added update hook.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.installundefined
@@ -1976,6 +1976,16 @@ function system_update_8021() {
+function system_update_8022() {
+  if ($install_task = variable_get('install_task', FALSE)) {
+    state()->set('install_task', $install_task);
+  }
+  variable_del('install_task');

Should be update_variable_get() and update_variable_del()

Dean Reilly’s picture

Status: Needs work » Needs review
FileSize
764 bytes
4.05 KB

Updated function calls.

Dean Reilly’s picture

Added in install_time conversions from #1798840: Convert install_time to use state system and updated the update hook to convert that variable too.

Lars Toomre’s picture

Hopefully, this is the first of the patches that touch the system module which is committed. There a lot of variables that should be added to this patches system_update_8022().

If another system conversion patch lands first (and touches system module), check for right update_N function.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Tested... both a fresh install and an update from Drupal 7 to Drupal 8

Thanks for the work!

sun’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/includes/install.core.inc
@@ -439,7 +439,7 @@ function install_run_tasks(&$install_state) {
+        state()->set('install_task', $install_state['installation_finished'] ? 'done' : $task_name);

@@ -525,7 +525,7 @@ function install_run_task($task, &$install_state) {
+        state()->set('install_current_batch', $function);

@@ -2002,5 +1994,5 @@ function install_configure_form_submit($form, &$form_state) {
+  state()->set('install_time', $_SERVER['REQUEST_TIME']);

We should namespace these with the owning module name, in this case System:

system.install_task
system.install_current_batch
system.install_time

westie’s picture

As per #14 added system namespace

Status: Needs review » Needs work

The last submitted patch, convert-install_task-variable-to-state-api-1798732.patch, failed testing.

Dean Reilly’s picture

+++ b/core/modules/system/system.install
@@ -1976,6 +1976,20 @@ function system_update_8021() {
+function system_update_8022() {

Looks like the update hooks were renumbered yesterday:

http://drupalcode.org/project/drupal.git/blobdiff/bf586d4f3337dcb427ed4b...

This will need to become system_update_8023() now.

LinL’s picture

Currently the latest patch for #1798796: Convert javascript_parsed variable to use state system includes system_update_8023 and there are other patches that include a system_update_N function, so not really sure how we co-ordinate this?

Dean Reilly’s picture

I'd be in favour of proceeding as we are now and just re-rolling the patches as necessary after each of the issues gets committed. It will be a little more work for the people writing the patches but probably easier than one person trying to coordinate between all of the issues now. It also keeps the patches nice and bite-sized. :)

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.82 KB

Rerolled to bump to system_update_8026() otherwise looks good. No commit credit necessary.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks great, committed and pushed to 8.x. Thanks a lot!

sun’s picture

Status: Fixed » Needs work
+++ b/core/includes/install.core.inc
@@ -436,7 +436,7 @@ function install_run_tasks(&$install_state) {
-        variable_set('install_task', $install_state['installation_finished'] ? 'done' : $task_name);
+        state()->set('system.install_task', $install_state['installation_finished'] ? 'done' : $task_name);

@@ -2006,5 +1998,5 @@ function install_configure_form_submit($form, &$form_state) {
-  variable_set('install_time', $_SERVER['REQUEST_TIME']);
+  state()->set('system.install_time', $_SERVER['REQUEST_TIME']);

+++ b/core/modules/system/system.install
@@ -2132,6 +2132,20 @@ function system_update_8025() {
 /**
+ * Convert install_task and install_time variables to state api values.
+ */
+function system_update_8026() {
+  $variables = array('system.install_task', 'system.install_time');
+
+  foreach ($variables as $variable) {
+    if ($value = update_variable_get($variable, FALSE)) {
+      state()->set($variable, $value);
+    }
+    update_variable_del($variable);
+  }
+}

The old variables are not upgraded, since the update searches for the new state names.

sun’s picture

Status: Needs work » Needs review
FileSize
2.1 KB

Attached patch fixes the upgrade path and introduces a helper function update_variables_to_state() to simplify these migrations.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Looks good and yes we need that method :)

Lars Toomre’s picture

Unless I am reading the patch in #23 incorrectly, I think that there is a potential problem for those D7 variables that have a NULL value. Such variables would be skipped in the conversion to state process, yet they would be deleted at the end of the loop.

I think the update function needs to build an array of those variable names that have been converted rather than using the passed in list. Am I wrong?

webchick’s picture

Status: Reviewed & tested by the community » Needs review

That sounds like a needs review.

alexpott’s picture

Assigned: Dean Reilly » Unassigned

I'm going to add upgrade path tests to this in the same vein as the tests for the config upgrade path tests.

In answer to #25, I believe this is intentional. The point is in D7 if the variable is not in the variable table then the value should not stored in the state system bu the upgrade path. See http://api.drupal.org/api/drupal/core!includes!update.inc/function/updat...

Now the interesting thing is when the value of the variable is set to NULL in the D7 db and this is not the default value assumed... I think if this would affect things then the upgrade should deal with this on a case by case basis.

For the variables migrated by this patch that is not an issue as null would be an invalid value.

alexpott’s picture

Added tests for state upgrade. Attaching a patch without the fix to show that test failing :)

sun’s picture

Status: Needs review » Reviewed & tested by the community

Works for me.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

I reviewed this as well and it looks good to me too. I'm still not 100% that 'State API' is the best terminology for this, but (1) I can't think of a better name and (2) we should debate that in another issue. Committed to 8.x. Thanks!

Damien Tournoud’s picture

Priority: Normal » Critical
Status: Fixed » Active

As a consequence of this, you cannot install Drupal anymore with both:

  1. A valid settings.php file, and
  2. An empty files/ directory, or no files/ directory at all

What happens is that the installer creates a fake container when the config system is not in a valid state (in install_begin_request()). This container doesn't have the keyvalue service registered to it, so calling state()->get('system.install_task') in this case fails miserably:

exception 'Symfony\Component\DependencyInjection\Exception\InvalidArgumentException' with message 'The service definition "keyvalue" does not exist.' in /srv/commerce2.lab/www/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php:690

Stack trace:
#0 /srv/commerce2.lab/www/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php(338): Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('keyvalue')
#1 /srv/commerce2.lab/www/core/includes/bootstrap.inc(2487): Symfony\Component\DependencyInjection\ContainerBuilder->get('keyvalue')
#2 /srv/commerce2.lab/www/core/includes/install.core.inc(921): state()
#3 /srv/commerce2.lab/www/core/includes/install.core.inc(377): install_verify_completed_task()
#4 /srv/commerce2.lab/www/core/includes/install.core.inc(82): install_begin_request(Array)
#5 /srv/commerce2.lab/www/core/install.php(36): install_drupal()
#6 {main}
alexpott’s picture

I think this will be fixed when #1809206: KeyValueFactory hard-codes DatabaseStorage lands...

webchick’s picture

Priority: Critical » Normal
Status: Active » Postponed

K, well in the meantime, reverted this commit from 8.x because we don't need two new criticals gumming up all the features.

Marking postponed on #1809206: KeyValueFactory hard-codes DatabaseStorage.

Cameron Tod’s picture

Just as an FYI, I'm getting the same error as #31 with this patch reverted.

Starting Drupal installation. This takes a few seconds ...                                                                          [ok]
Undefined index: backtrace errors.inc:188                                                                                              [notice]
WD php: Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "keyvalue" does not exist. [error]
in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of
/Users/cam8001/Sites/drupal/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "keyvalue" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of /Users/cam8001/Sites/drupal/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).
Drush command terminated abnormally due to an unrecoverable error.                                                              
cburschka’s picture

Status: Postponed » Needs work

Patch #20 never got reverted.

Check history of install.core.inc, which includes patch #20 but no revert: http://drupalcode.org/project/drupal.git/history/refs/heads/8.x:/core/in...
Patch #20 got committed: http://drupalcode.org/project/drupal.git/commit/91fec4d2d77948cc3ff64782...
But the revert only targets patch #28: http://drupalcode.org/project/drupal.git/commit/41814bb38c67f86c7ad285eb...

Reverting it manually seems to allow installation to proceed.

sun’s picture

Category: task » bug
Priority: Normal » Critical
Status: Needs work » Reviewed & tested by the community

#28 has to be reverted and presents a critical bug in the installer right now.

sun’s picture

Sorry, #28 has been reverted already, but not #20.

alexpott’s picture

The alternative is to commit #1798738: Move css_js_query_string to state system which fixes this.

sun’s picture

That issue only touches the surface of the actual problems.

The story begins in drupal_install_system() already, which installs System module's tables and directly afterwards calls into the keyvalue service in order to record System module's schema version. But, at that point, there is no keyvalue service to begin with.

#1798738: Move css_js_query_string to state system further hides that problem by actually introducing a keyvalue service for the installer, but that's a memory backend, so System module's schema version is not stored in the database.

Fixing that (like here) opens yet another can of worms: install_begin_request() unconditionally records $install_state['database_tables_exist'] even before drupal_install_system() has been called, so in fact, no database tables exist yet, but the installer assumes they would.

In short: It's a horrible mess.

catch’s picture

Status: Reviewed & tested by the community » Needs work

If the issue is that the table isn't created yet, then as long as there's a database connection
we can use the same pattern as in #1167144: Make cache backends responsible for their own storage for the key/value store and it should work automatically - that's one of the main motivators for that patch.

If it doesn't, then we'd need a persistent storage engine, not in the db, which could copy over to Drupal install proper at the end or s

alexpott’s picture

I've tested it and #1798738: Move css_js_query_string to state system does not prevent System module's schema version from being stored in the database.

The keyvalue storage override is removed in install_settings_form_submit() which is fired on the step before drupal_install_system(). I think it would have fixed the specific issue we are facing here. However, I totally agree it's a horrible mess :)

sun’s picture

Status: Needs work » Needs review
FileSize
6.37 KB

First of all, attached is a cumulative revert-revert of #20 and #28 against latest HEAD.

To combat the install phases fiasco, I played with the idea of simply adding a new installer phase/step that - when reached - switches from the installer container to the regular container. Thus, eliminating the entire horrible mess of variables that attempt to track the availability of various services. Instead, let's have a single, defined step and entry point in the installer that performs the switch — everything before that step operates on the empty environment, everything after can rely on settings.php, configuration, database, and keyvalue to exist. How does that sound?

sun’s picture

FileSize
26.36 KB
32.09 KB

What. A. Ride. — That was fun. :)

  1. Rewrote the installer.
  2. Fixed missing kernel in full bootstrap, and missing global $user.
  3. Removed debugging code.
  4. Added more documentation.

Summary: This simplifies the installation process by separating it into two phases:

1) Before system schema.

2) After system schema.

Before means that there is nothing yet, and the entire installer has to work off runtime/memory information. Contrary to that, After means that we're in a fully operational Drupal environment, and have all regular services at hand.

Thus, as soon as the system schema is installed, all functionality in the installer works as usual, and thus, we're eliminating the entirety of special-casing that previously existed for the installer. This includes the installation of all modules and the install profile itself.

Quite extensively tested — after all that DDL, I should defragment my disk now... ;)

Status: Needs review » Needs work

The last submitted patch, installer.services.43.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

This seems to make #1792536: Remove the install backend and stop catching exceptions in the default database cache backend obsolete.

+++ b/core/includes/install.core.inc
@@ -876,15 +838,94 @@ function install_verify_requirements(&$install_state) {
+function install_system_rebuild(&$install_state) {

I need to correct the high-level process flow comments for install_system_rebuild(), since this install task callback is actually re-invoked on every request. The code logic is correct, only the comments are a bit misleading.

Crell’s picture

*raises hand* This conversion could happen much later. It's going to conflict with #1530756: [meta] Use a proper kernel for the installer, which has to happen sooner if it's going to happen. Could we postpone this issue on whether or not the installer-kernel happens, since that would completely obsolete this issue anyway?

Comments like "I rewrote the installer" tossed into an issue about removing a variable scare the dickens out of me, especially when there's a separate "rewrite the installer" thread already, one that the aforementioned poster knows about because he's commented in it.

sun’s picture

FileSize
32.05 KB

Oh, sorry, the "Rewrote the installer" commit message isn't really true — putting that message in was only necessary from a personal mood after spending almost a day in this code. ;) When my cowboy debugging code still existed, that message actually should've been "Rewrote core" :P

The only thing that is changed here is the order of installation tasks being executed, and the environment in which these tasks are executed. The switch to a full, regular DrupalKernel as soon as it is possible helps a lot to simplify the installer environment and make it understandable for developers.

Additionally, the pre-system-schema environment also becomes a clearly defined in-memory/runtime environment (no substantial change there, so this is still a custom ContainerBuilder), which makes it crystal clear what services are registered and how, and also how long that environment exists.

In fact, this patch helps to move forward in a dozen of different areas currently:

  1. Quite a couple of other issues keep on running into the always-identical problem space of the undefined mish-mash environment that is the installer, which currently lacks the clear environment separation that is introduced by this patch.

    Those are not only config/state conversions. Without deeper investigation, the following issues immediately cross my mind:
    #1764474: Make Cache interface and backends use the DIC
    #1811730: Refactor Database to rely on the DIC solely
    #1067408: Themes do not have an installation status
    #1356276: Allow profiles to define a base/parent profile
    #1792536: Remove the install backend and stop catching exceptions in the default database cache backend
    ...

  2. I worked on this as a preparation for #1530756: [meta] Use a proper kernel for the installer to begin with. This patch simplifies the kernel work.

    I also need to amend that aforementioned issue is very far way from being in a remotely ready state, and I suspect that its state and progress is essentially blocked exactly by the environment mess I'm resolving here.

So, I really don't think that there's anything to worry about. I will ping @Letharion though.

Meanwhile, merged HEAD. No changes, just a diff context conflict.

sun’s picture

FileSize
5.78 KB
32.61 KB
  1. Fixed trailing white-space.
  2. Fixed root user is not logged in after installation.

This patch should come back green, and I consider it ready.

Status: Needs review » Needs work

The last submitted patch, installer.services.48.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
5.88 KB
34.66 KB
  1. Removed unnecessary loading of include files and clarified the remaining.
  2. Fixed ConfigStorageTestBase expectations are incompatible with empty config files.

Hm. #47 almost passed, but #48 did not — let's see how this one goes.

Status: Needs review » Needs work

The last submitted patch, installer.services.50.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1019 bytes
35.15 KB

Ah, found the culprit.

Fixed install task completion of install_system_schema() cannot be recorded.

Status: Needs review » Needs work

The last submitted patch, installer.services.51.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
5.45 KB
37.46 KB

Was finally able to reproduce the testbot failures.

  1. Fixed readable+writeable settings.php is wrongly detected as unreadable.
  2. Fixed installer batch is created with wrong session ID.
  3. Optimized base_system_verified status handling.
  4. Removed unnecessary re-execution of language and profile selection tasks.

This one should come back green for real now. :)

Status: Needs review » Needs work

The last submitted patch, installer.services.54.patch, failed testing.

sun’s picture

sun’s picture

Status: Needs work » Needs review

Further debugging of the test failures in #1067408: Themes do not have an installation status revealed that the installation failures over there require the identical fixes as here, and manual testing of both patches combined positively confirms that this patch here unblocks that issue.

Thus, we're mainly blocked on the "upstream" testbot fix in #1835144: pifr_drupal.client.inc makes too many assumptions about Drupal installer internals now.

Also, changing status to needs review, since the testbot failure is a fake.

jthorson’s picture

Drop me an IRC tell if you get blocked like this on testbot ... I'm more likely to get the message there than from within the issue queue; especially since we're heads-down on the PIFT port for D7 drupal.org right now. :)

jthorson’s picture

Issue tags: +Needs reroll

Tagging for re-roll.

jthorson’s picture

Attempted re-roll. Patch is successfully past install and running assertions on drupaltestbot-dev, with the patch from #1835144: pifr_drupal.client.inc makes too many assumptions about Drupal installer internals applied.

jthorson’s picture

FileSize
37.07 KB

Ooops ... forgot the file. :)

Status: Needs review » Needs work
Issue tags: -Needs reroll, -State system

The last submitted patch, installer.services.60.patch, failed testing.

jthorson’s picture

Status: Needs work » Needs review
Issue tags: +Needs reroll, +State system

#61: installer.services.60.patch queued for re-testing.

sun’s picture

FileSize
1.25 KB
37.29 KB

Yay, thanks @jthorson! :)

Merged HEAD again to verify what else has changed, aside from my own patch that removed user.module. Apparently, the security fix in #1816124: Fix installer PHP code execution issues from SA-CORE-2012-003 (and backport anything to 7.x-dev as necessary) also landed, whereas I previously removed that check entirely, since the installer will blow up anyway later, but if that's what the security team wants... re-incorporated that snippet.

  1. Merged HEAD
  2. Restored/reincorporated #1816124: Fix installer PHP code execution issues from SA-CORE-2012-003 (and backport anything to 7.x-dev as necessary)

I think this patch is ready to go. It unblocks work in several other issues.

sun’s picture

Assigned: Unassigned » sun
sun’s picture

Any chance to move forward here? The patch is ready from my perspective.

sun’s picture

I really need this to move forward with #1067408: Themes do not have an installation status and #1530756: [meta] Use a proper kernel for the installer.

If this won't land this week, I fail to see how either of those can happen for D8. My stance on feature freeze is crystal clear, regardless of how wishy-washy others may put it, and both of them involve sufficient API changes to count as features, no matter how much you bend the interpretation.

Therefore, if this won't land ASAP, I'll postpone and move at least those two to D9.

webchick’s picture

Issue tags: -Needs reroll, -State system

#64: installer.services.64.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs reroll, +State system

The last submitted patch, installer.services.64.patch, failed testing.

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
36.78 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, installer.services.70.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
911 bytes
36.81 KB

This should fix the installation failure.

Berdir’s picture

The patch is rather hard to review, a lot of code is being refactored and moved around. Is there a summary somewhere that explains what exactly is done and why somewhere in this issue or another one? (haven't read anything)

Was interested to see if this improves/changes the installation time, especially during tests. It does (time difference isn't that big, but I am seeing a considerable memory improvement from ~16MB to ~12), but I haven't really been able to understand why. Most differences are just because of things moved between functions.

The strange thing seems to be that one function with the biggest improvement are the curl_exec() calls, which doesn't make much sense to me. Other than that, I'm actually seeing quite a few additional database queries, mostly due to more config() calls.

Looking at the patch, I can see two todos about possibly unnecessary full cache clears. If we'd be able to avoid these, than that would actually be a nice improvement. I'm seeing 3 calls to that function, making up almost 30% of the total wall time and 60% of that is menu router rebuilding.

sun’s picture

FileSize
1.52 KB
37.39 KB
  1. Merged HEAD.
  2. Updated for new DrupalKernel arguments.

Yeah, sorry, there is no summary for the additionally required installer changes yet. I'll write a proper summary and put it into the issue summary in a moment.

sun’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

The D9 decision lays with the core committers alone. Just an API change is not a reason for that as the API freeze not until April 1.

$readable = $writable = drupal_verify_install_file($settings_file, FILE_READABLE|FILE_WRITABLE);

I am not sure whether this is something that we usually do coding standards wise nor is it clear why this is suddenly important when previously we didn't assign this here...?

However, decode returning an array() instead of FALSE is kind of a big deal. We already have a ParseException, can't we throw a NotDrupalConfigException here or something??

In general it seems to me that there is a shocking amount of changes in this patch for something that should be almost trivial...? The installer first, within a request, goes through the install tasks of choosing a language, profile, set database, install system and then it stores the install state because there is now a place to store it. If system is installed then they key-value service should be up and kicking too. It seems the only change is use the keyvalue table instead of the variable table (both are system installed) and use a slightly different API (state->set() instead of variable_set() What am I missing? Why is the installer half-rewritten?

chx’s picture

> directly afterwards calls into the keyvalue service in order to record System module's schema version. But, at that point, there is no keyvalue service to begin with.

Then do not use a service! Can't we call the class directly and call it a day? Avoiding that doesn't worth such a massive refactor especially not on a codebase that's barely tested and known to be super brittle :/

chx’s picture

And, if your concern is that makes the state storage hardwired SQL -- hey, that's my duty to be concerned 'bout that -- then why don't you add an install task which sets the storage class name -- can be stored in the install task PHP variable itself -- and then aspiring profiles can task alter it? Yes it's a PITA but , is this thing here better? Perhaps it didn't get attention because the size and scope of the changes are so scary.

Berdir’s picture

About the $readable change. That's a duplicate of my issue at #1805324: Installer does not verify readability of settings.php, explanation can be found over there. That issue needs an OK from either one of you :)

Crell’s picture

One of the intents of #1530756: [meta] Use a proper kernel for the installer is so that the installer can have its own DIC, wired up totally differently than the main container. Baring that, manually instantiating objects for the installer is totally cool. One of the reasons we've been pushing hard to shift code to be cleanly injected is for exactly that reason: It makes manually wiring things up for odd cases (installer, update.php, etc.) much less fugly and error prone.

David_Rothstein’s picture

Some thoughts, partly along the lines of what @chx wrote above:

  1. Regardless of its relationship to this issue, @sun's patch addresses a legitimate Drupal 8 bug, which is that (a) the installer tries to reset drupal_container() midway through even though other services (like the cache and lock systems) aren't reset, and (b) it does it in a form submit handler that has no guarantee of ever being called.

    The conceptual fix (reset everything in a dedicated installation stage that always runs) makes sense to me, but I'm not surprised that it involves a large rewrite of the installer :)

    I do wonder if we've thought at all about the alternative fix, which is to just use the installer-specific container throughout and never try to reset it... seems a lot simpler?

  2. The installer needs to use state(), but the state storage has to be in memory and swapped out with the regular database backend as soon as it is available.

    I don't understand why having state storage in memory ever makes sense. Doesn't this mean that any code which calls state()->set() before the database is available will appear to succeed, but in practice the data is never stored permanently?

    It actually seems like an architectural problem that it's even possible to swap temporary storage and persistent storage in this way... But regardless of whether it's possible, it seems like a bad idea to do it.

  3. As far as I can tell, the reason converting variables like 'install_task' and #1798738: Move css_js_query_string to state system to the state() system is so hard is that variable_get() never assumed the database exists, but state()->get() in its default configuration does. That seems like an architectural problem to me too (because it means any early-running code which gets converted to state() is introducing a new dependency on the database that didn't previously exist).

    But in lieu of fixing that directly, it does suggest that these issues can be addressed by adding an installer-specific key-value storage that uses the database, but in cases where the database doesn't exist yet it (a) fails with an error on state()->set(), but (b) fails gracefully by returning NULL on state()->get(). Would that work?

  4. As for the specific idea of converting 'install_task' to the state system, I am skeptical that's a good idea in the first place for the following reasons:
    • The state system is designed to be easily swappable, but whether or not Drupal is fully installed is critical information that should never be allowed to accidentally or easily "disappear" (doing so would lead to security issues).
    • The current code assumes that 'install_task' will always be in the database; that is the only reason that existing code like this makes any sense:
        $task = install_verify_completed_task();
        ...
        $install_state['database_tables_exist'] = !empty($task);
      

      (@sun's patch however does address that by removing the assumption as well, but I don't think any of the other simpler patches proposed here did.)

    • If you look at the documentation for state(), it says:
       * Use this to store machine-generated data, local to a specific environment
       * that does not need deploying and does not need human editing; for example,
       * the last time cron was run. Data which needs to be edited by humans and
       * needs to be the same across development, production, etc. environments
       * (for example, the system maintenance message) should use config() instead.
      

      Honestly that doesn't sound like a use case for 'install_task' at all since it's a fundamental property of the site rather than local to a specific environment (arguably, even config() is a better fit for it, although that's not great either).

    So, we should think about doing something else with this variable. @chx's idea of simply hardcoding the SQL storage here is one possibility (although it's a little bit like using the fully-swappable state() system halfway). It makes me wonder whether the combination of state() and config() as currently written are enough for Drupal's use cases, as some of the simpler ways in which the old {variable} table could be used aren't covered by it?

David_Rothstein’s picture

Category: bug » task
Priority: Critical » Normal

As far as I can tell, this was only a critical bug because the original patch broke things. But that looks like it has since been reverted, so I think this can go back to a normal task?

Crell’s picture

@David: Which is why I said back in #46 that if we're rewriting the installer here, let's please stop wasting our time and rewrite it for reals. We don't need 2 "rewrite the installer" issues. We just need to gut it and write a proper installer app, which is what #1530756: [meta] Use a proper kernel for the installer is trying to do. Not gut it and rewrite a still PHP4-era installer.

sun’s picture

For anyone who missed it, the issue summary has been written.

  1. As discussed in #1530756: [meta] Use a proper kernel for the installer, the installer and install tasks flow essentially needs to be rewritten and converted into a Step controller (or "wizard"). This Step controller is only responsible for ensuring and executing a list of ordered steps.
  2. A Step controller is an independent thing and has plenty of use-cases aside from install.php and update.php. Its primary purpose is to store and maintain the current state and step of the process.
  3. Special environments like the installer enforce the Step controller to be re-entrant, because there is no storage in which the Step controller would be able to record anything, until that storage has been created.
  4. Currently, we're hiding away all of the actual underlying storage issues by uglifying all kinds of storage service backend implementations with try/catch statements — so as to not error-out when trying to write something to a storage in a situation where no storage can reasonably exist.
  5. All of that special-casing simply needs to go away, entirely. This patch is just simply doing that: As long as there is no storage, we do not register and do not attempt to use the service backends that would attempt to operate on that non-existing storage.
  6. Instead, we register the appropriate service backends for the early pre-{system}-schema installer phase - which essentially means in-memory backend implementations. (Because, again, there is no storage to write to.)
  7. This essentially means that the (currently procedural) Step controller gets all services with the appropriate storage backends injected. As long as there is no storage, state()->set() and state()->get() can only read and write to memory. Ditto for cache, lock, and everything else.
  8. The same is more or less happening today already; all of the storage backends have been hacked to silently fail on writes and reads — by registering and using the proper service backends, the only difference is that this becomes an explicitly expected and desired behavior, instead of hacking storage backends for a one-off case that actually should blow up during regular runtime on a production site, but does not, because it still contains the special-casing for the installer.
  9. As soon as the required storages exist, the installer switches to a regular, full environment, because there is no reason to special-case any service any longer.
  10. For the interactive installer, this happens in a separate request, which does not attempt to rebuild anything mid-request, but instead directly boots into the full environment. To differentiate between the two environments without performing all kind of conditional checks before booting, the installer leverages the only storage that exists: HTTP request parameters.
  11. Only the non-interactive (scripted) installer reaches the install step that rebuilds the environment "mid-script." This cannot be avoided.
  12. We cannot replace the services with hard-coded implementations for the post-{system}-schema phase, because that would prevent everyone from overriding service definitions with alternative implementations. E.g., if I'd want to use mongodb as key/value store instead of the database, then I need to be able to install Drupal with that storage backend already. Same applies to config and everything else.
  13. Whether the 'install_task' and related values should be converted into state or config or something else doesn't change anything of this — regardless of service/storage, the storage first needs to exist to be usable.
  14. As mentioned before, this patch was explicitly developed with #1530756: [meta] Use a proper kernel for the installer in mind. However, that issue did not focus on these essential parts at all yet. Furthermore, this patch unblocks at least one other major issue, #1067408: Themes do not have an installation status, which is blocked on the way the installer sets up services and how it installs System module — the patches have been developed completely independently from each other, but simply merging this patch into the other makes it magically pass all tests and resolves all Drupal installation errors.
sun’s picture

Issue tags: -Needs reroll
FileSize
37.39 KB

Merged HEAD / Safety re-roll.

chx’s picture

So basically this patch is like half of #1530756: [meta] Use a proper kernel for the installer ? I will see whether I can write something (much) simpler which does what the issue title says and no more.

sun’s picture

I will see whether I can write something (much) simpler which does what the issue title says and no more.

That's extremely offensive. But let me combat it:

You've already indicated that you're scared and afraid of changing the installer, so I don't think you're the right person to touch it.

Thanks for duplicating and blatantly disrespecting all of the work that has gone into this issue, kthxbye.

chx’s picture

Sigh. All the work that went into this issue should be in #1530756: [meta] Use a proper kernel for the installer. That's what I am trying to convey across. I am not disrespecting the work -- I am saying, this issue is not about rewriting the installer or should not be and I would like to investigate whether it is possible to do what this issue should be about without the massive changes here.

thedavidmeister’s picture

chx is there an explicit reason (like another issue that is dependant on this) why you're looking to get a patch written to match the title of the thread without the extra work that Sun has done?

If scope creep is your main concern then duplicated efforts would be even worse, right?

Sun has been posting patches here and at #1067408: Themes do not have an installation status consistently every few days for quite a few weeks now so it would be a shame to not leverage that.

Sun, is it possible for you to split out some of the work you've done into a smaller patch that addresses this issue and move the combined patch to the other thread?

Or, can we just rename this thread and mark the other as a duplicate if there's legitimate concern that not enough progress has been made against all the issues outlined in #83? (especially considering how late in the year we are atm).

Sylvain Lecoy’s picture

re @ #30, from http://drupal.org/node/1787278:

The State API provides a place for developers to store information about the system's state. State information differs from configuration in the following ways:

  • It is specific to an individual environment.
  • You will never want to deploy it between environments.
  • [Other better explanations]

Why not calling it env ? environment API ?

thedavidmeister’s picture

Sylvain, as Dries said in #30, please discuss this in a different issue.

chx’s picture

Do whatever you want, I am out.

David_Rothstein’s picture

I would suggest moving @sun's patch to a separate issue simply because it's a bug fix. What's going on with drupal_container() in the D8 installer currently is definitely a bug, and the patch addresses that. This issue, meanwhile, is a task, and so is #1530756: [meta] Use a proper kernel for the installer...

If it becomes necessary to postpone one or both of those on the bug fix, that's possible, of course. But I think @chx is likely correct that for this issue, it should be possible to make progress independently.

sun’s picture

I'd be interested to learn what's actually wrong with the patch. I think #83 addressed all questions that have been raised so far.

The original objective of this issue was a task, but it revealed that we're facing a range of logical and architectural problems in the installer. In order to accomplish and resolve the task at hand, those problems had to be resolved. As such, this patch does exactly what is being asked for and what needs to be done.

Plus, there's additional confirmation that this patch helps other issues to move forward.

What I'm missing are true technical concerns that can be addressed to get this issue off the table.

thedavidmeister’s picture

fwiw, I just ran through the installation process after checking out latest D8 and applying patch in 84 and there weren't any problems. Unsurprising considering the test bots are happy. Just thought I'd mention that at least one human did it too :)

thedavidmeister’s picture

Also, I looked over the patch the install_task variables now all look something like "state()->get('system.install_task')" which is good.

I'm curious as to why some other things have been moved over too, like:

- $cron_last = variable_get('install_time', 0);
+ $cron_last = state()->get('system.install_time') ?: 0;

But other things haven't been, like " variable_set('install_profile', $profile);" in install_finished().

Is the idea here to "rewrite the installer for reals" like earlier mentioned, or are we stopping at where the latest patch got to for this issue and leaving it to other issues to clean out what's left? if so, what should we be looking for as testers to know what's left undone by design and what hasn't?

Letharion’s picture

@#95 The difference is that "install_time" isn't configuration, but "state". See http://drupal.org/node/1787278.
The variable_get will be converted to CMI, but that's for a separate issue.

David_Rothstein’s picture

What I'm missing are true technical concerns that can be addressed to get this issue off the table.

See #80.2 and #80.4. Though neither of these are concerns with the bulk of your patch, just with the integration with the state system as per this issue (one of the reasons it could make sense to split it to a new issue).

@#95 The difference is that "install_time" isn't configuration, but "state". See http://drupal.org/node/1787278.

See #80.4. Everything (edit: "Some of what" :) I wrote there about 'install_task' applies to 'install_time' too; neither of these seem to make sense as "state" given the description on that documentation page. (It's less important for 'install_time', but that's because the information contained in that variable is less critical.)

thedavidmeister’s picture

uhm, the numbers in #80 are 1, 3, 4. There is no 80.2

Could we write the information about the installation status of a site into its settings.php file and leave it out of the state system? Then you could "swap" states without losing it.

sun’s picture

I've the impression that some of you assume that I would have refactored a lot more than what is actually needed. But reality is, I definitely did not — quite the contrary, I started very simple, but bumped from one hairy problem into the next. That has been a very painful ride that cost me hours of debugging, a few days in total. Only after ending up with a pile of tweaks all over the place, and still facing a non-functional installer, I concluded that hacking the system that much to make it work does not make sense and the problem needs to be addressed with a clear and simple architectural concept instead.

And that's the concept I'm proposing to resolve this issue: It is clear. And it is simple.

I tried to address all points of #80 in #83 already, but let me iterate over the points in question once more:

I don't understand why having state storage in memory ever makes sense. Doesn't this mean that any code which calls state()->set() before the database is available will appear to succeed, but in practice the data is never stored permanently?

When you hit install.php, then nothing exists yet. However, the installer itself as well as various other subsystems are using state() to read and write data to it.

This happens today already, and the only reason this works is because we've hacked all of our storage service implementations to not throw errors in case the storage does not exist yet. The only reason we hacked them is the installer.

Instead of trying to write to and read from a storage that cannot reasonably exist, this patch changes the service registration of affected services to explicitly use appropriate implementations. The only appropriate implementation for the installer is an in-memory implementation. (The only alternative would be a null implementation, but that would only mean that data may possibly be re-regenerated for no good reason.)

Instead of "guessing" and "trying" whether a storage exists, and carrying on the special code for the installer for the entire lifetime of a Drupal site, this patch changes the installer into two essential phases:

1) Initial configuration screens, before storages have been set up.

2) Execution of the installation process, after storages have been set up.

Only the installer steps that happen in phase 1) have the storage service overrides. Those installation steps do not write any persistent data, because they cannot write anything in the first place.

Directly after the storages have been created, the total remainder of the installation is executed in phase 2). All of the critical installation steps to install the application are part of this phase. This includes the installation of System module and User module. This means they get a regular, full-blown DrupalKernel, and the entire system operates as usual, as during regular runtime.

Technically, it should even be possible to redirect to /index.php for the installation steps executed in phase 2) [since the install.php environment is factually identical to index.php], and implement that phase of the installer as a regular module or via built-in Core bundle routes, but I did not try that as it's obviously out of scope for this issue.

Back to your concern, using in-memory storage implementations is perfectly valid and legit for the first, initial installer phase. Instead of hacking the entire system for the installer, we should have done that in the past already.

The only alternative to that would be to not use any services that involve any kind of storage anywhere in the first/initial installer phase. In order to do so, you'd have to duplicate and rewrite a dozen of subsystems into special installer implementations that do not rely on any of those services. However, that makes no sense, since we can properly inject in-memory storage implementations now, so it doesn't matter whether anything uses any of those services.

It actually seems like an architectural problem that it's even possible to swap temporary storage and persistent storage in this way... But regardless of whether it's possible, it seems like a bad idea to do it.

The storage is not swapped. As mentioned before, the entire installer environment is "swapped", but that only means that the overridden kernel and container is destroyed and instead, a completely new, regular kernel and container is instantiated.

As also mentioned before, this rebuild only happens in the non-interactive installer. The interactive installer does not do that. The interactive installer simply redirects as soon as the storage (tables) have been set up, which means that the next request that hits install.php operates in a full-blown, regular Drupal environment.

As far as I can tell, the reason converting variables like 'install_task' and #1798738: Move css_js_query_string to state system to the state() system is so hard is that variable_get() never assumed the database exists, but state()->get() in its default configuration does. That seems like an architectural problem to me too (because it means any early-running code which gets converted to state() is introducing a new dependency on the database that didn't previously exist).

None of these problems exist in HEAD anymore, with and without this patch.

But in lieu of fixing that directly, it does suggest that these issues can be addressed by adding an installer-specific key-value storage that uses the database, but in cases where the database doesn't exist yet it (a) fails with an error on state()->set(), but (b) fails gracefully by returning NULL on state()->get(). Would that work?

This is the exact sort of bad hackery that we've applied to most storages over the years. Instead of continuing to hack the entire system for the installer, the presented solution here is clean and simple:

We actually know the exact point in time in which storages are available. Before that point, it is stupid to even attempt to use a storage that does not exist. After that point, there's no reason to special-case any storage. In short, there's no reason to hack anything.

After this patch has landed, we can remove all of the hacks that have been applied to all the storage implementations throughout core.

The state system is designed to be easily swappable, but whether or not Drupal is fully installed is critical information that should never be allowed to accidentally or easily "disappear" (doing so would lead to security issues).

That's really off-topic for this issue, as it pertains to any service that is swappable. If you swap out the config service, you need to migrate your data from A to B to have a functional system. If you swap out the state/keyvalue service, then you need to do the same, migrate your data from A to B to have a functional and secure system. So, again, that has nothing to do with this issue.

The current code assumes that 'install_task' will always be in the database; that is the only reason that existing code like this makes any sense:

  $task = install_verify_completed_task();
  ...
  $install_state['database_tables_exist'] = !empty($task);

Both points are not correct.

1) The current code assumes that 'install_task' is always stored in a variable. There is no single place throughout core that makes the assumption that it would be in the database. All code uses the Variable API only.

2) The existing installer code that you've nicely outlined here uses this check to determine whether the {system} schema has been installed. And this is actually where the trouble starts: In order to have not any call to state() blow up in the first/initial installer phase, the keyvalue service has to use an in-memory implementation. But doing so inherently makes this poor check here invalid, because we are actually able to read the current task from the memory storage.

The check itself is wrong, because it does not check what it actually intends to check: Its intention is to check whether the {system} database table schema has been created already. As such, the check should use db_table_exists($random_system_tablename).

But adjusting that does not help in any way, because the check is executed too late in install_begin_request() — the entire installer environment has been set up already, so at minimum the check also needs to be moved way up in that function, so we're able to determine whether we still need to override storage services.

Doing so, however, bumps into the next best circular dependency: You cannot call db_table_exists() without a working database connection. [...]

...and after resolving all of the further race conditions which follow after that, you realize that it does not make any sense to determine all of this mid-request.

Instead, what's needed is a clear and simple separation between environments. Why don't you pass me a flag, so I just simply know that I can boot?

Honestly that doesn't sound like a use case for 'install_task' at all since it's a fundamental property of the site rather than local to a specific environment (arguably, even config() is a better fit for it, although that's not great either).

This essentially questions the original conversion task of this issue. However, I disagree with the analysis. The 'install_task' and 'install_time' data is no different than the recorded schema versions of installed modules, which are equally stored in state. They are actually the direct equivalent of schema versions, because at least 'install_task' denotes whether Drupal has been installed - in the exact same way as a schema version of >= 0 denotes that an extension has been installed.


So... with these points hopefully out of the way...

  1. Merged HEAD.
  2. Fixed DrupalKernel unconditionally initializes session.
thedavidmeister’s picture

Please try to remember that we haven't spent days debugging this ourselves, but as a community we're charged with reviewing it anyway so yeah, we're going to ask questions along the way. I don't think anyone is assuming that you're doing things in the wrong way, but if it wasn't immediately obvious to you that the whole system needed to be split into two parts to fix this issue then it won't be to anyone else either.

The writeup in #99 is excellent and is going to be very helpful for anyone reviewing the patch. When I tried to review it previously I definitely had to skip over hunks of the patch where I didn't understand why exactly you were doing something - I think now we should be able to do a better job with more context. Thanks for that.

chx’s picture

FileSize
14.08 KB

Here's a patch which

  1. Removes all services from drupal_container(). Yay!
  2. Converts install_task to use state, properly, if settings.php has container_bundles set up in $conf to override keyvalue, that is obeyed cos we use a kernel from the moment system schema is installed.
  3. Reintroduces a drupal_bootstrap bugfix that sun already removed once from one of my patches and I just spent a couple hours debugging on a testbot because of it.
  4. After a DRUPAL_BOOTSTRAP_FULL call your Drupal is now fully functional again (which might be considered a major regression in itself that it wasnt), no need to independently create a kernel.

All this is happening in a 14KB patch that is a net negative in the number of lines of code. Rewriting the installer can happen in another issue. It is not belonging to this one. I am quite sure this patch is not yet ready but I know it installs and I ran a DrupalUnitTestBase and a WebTestBase test and both passed.

chx’s picture

FileSize
897 bytes
14.88 KB

So I upped the wrong patch, the drupal_bootstrap fix is missing. (Don't worry, it still removes 22 LoC.)

berdir already notes that the drupal_container doxygen is largely deprecated. I will in the next reroll. In turn I would like to note that this minimal container a) does not obey the container_bundles setting in conf, wreaking total havoc on any override b) it's a maintenance nightmare c) we are registering so many services there now that loading a compiled container is surely a better alternative.

chx’s picture

FileSize
1.22 KB
15.88 KB

Ah, UnitTestBase. I am sure the upgrade tests will be problematic too.

Status: Needs review » Needs work

The last submitted patch, 1798732_real_103.patch, failed testing.

xjm’s picture

chx’s picture

Status: Needs work » Needs review

I have submitted my patch as a separate issue. I would recommend postponing the actual conversion until that one is done or retitling this and going with sun's patch.

sun’s picture

#99 came back green - I've administratively sent it for re-test in the hope to get a green result here.

#99 has also been manually tested in all required ways. The interactive installer as well as all the different installer scenarios are not covered by automated tests (and cannot be covered).

Changes to the installer require manual testing, involving all possible permutations of:

- Preexisting/non-existing settings.php.
- Preexisting/non-existing configuration.
- Existing/non-existing database tables.
- Standard profile vs. Minimal profile.

All of these permutations have been extensively tested manually for #99 and are confirmed to work.

#1067408: Themes do not have an installation status and its dependent issues are still blocked by this, are bound to feature freeze, and quickly moving forward here is the only chance for those issues to realistically get ready before Dec 1st.

thedavidmeister’s picture

Title: Convert install_task variable to use state system » Split installer into pre-storage & storage-exists phases in order to safely convert install_task variable to use state system

how's this?

thedavidmeister’s picture

Status: Needs review » Reviewed & tested by the community

and this? as per #106, #107 and #108.

chx’s picture

Title: Split installer into pre-storage & storage-exists phases in order to safely convert install_task variable to use state system » Split installer into pre-storage & storage-exists phase
Status: Reviewed & tested by the community » Needs review

#109, where is the technical review of this patch that made this ready...? I can't see any :/

thedavidmeister’s picture

Oh, I misinterpreted this for one:

#99 has also been manually tested in all required ways. The interactive installer as well as all the different installer scenarios are not covered by automated tests (and cannot be covered).

Apologies.

chx’s picture

The other issue says about this one: "unblocks a range of other features, whereas the kernel patch does not". And what features? I read the issue summary, I read the issue, read the patch, but it's not clear what features you mean.

sun’s picture

See #67, #57, and #47.

Furthermore, there's a full stack of issues that are blocked on the #1067408: Themes do not have an installation status feature. That feature is blocked on this issue though. But it is confirmed to fully work when merged with the changes here.

chx’s picture

I would like to understand how the One Container issue does not help your goals. "After {system} schema, the entire remaining installer process operates in a full environment without any mocked services and also a full, regular DrupalKernel." this is exactly, literally what the One Container patch does, it instantiates a DrupalKernel after the schema is installed (in later requests it instantiaties a kernel in begin request). I have not read yet this excellent issue summary back when I was coding it and it just hit me that this is the right place to do that. We definitely agree on this.

I have read #99 throughly and, while I understand the installer is a weird thing, are we sure the weirdness introduced by #99 is going to be any better? It creates its own bootstrap flow. It uses bootstrap phases to trigger a redirect.

I see your comment #1067408-83: Themes do not have an installation status saying "Unfortunately, the dependency of this issue just started to get hi-jacked, and history/experience leaves me with sufficient confidence that whichever final solution over there will not be sufficient for this issue". I am very sorry if history/experience taught you that. I would like to repeat that our culture is based on collaboration and I am interested in maintaining that culture. So, I have applied the one container first and the linked patch on top of it and while it is entirely possible that I botched up the merge but both minimal and standard installed for me. So I can only repeat the question in #112 -- could you please point out something that breaks the installer after the one container?

Berdir’s picture

#99: installer.services.99.patch queued for re-testing.

sun’s picture

  1. Merged HEAD.
  2. Updated for latest HEAD.

Status: Needs review » Needs work

The last submitted patch, installer.services.116.patch, failed testing.

sun’s picture

Assigned: sun » Unassigned
sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
Issue tags: +Framework Initiative, +API clean-up, +Configuration system, +kernel-followup
FileSize
5.3 KB
37.69 KB
  1. Merged HEAD.
  2. Fixed System module has to be installed exactly like any other module.
sun’s picture

Reverted obsolete changes and clarified docs for drupal_install_system().

sun’s picture

Alright, we're back to green here. Extensive debugging for #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity) showed that it is facing the exact same installer bug as the original state conversion here. We're applying a messy stop-gap fix over there for now, but reality is that System module is not installed correctly during installation of Drupal, which in turn makes it a rather crazy coincidence that things are currently working.

I consider this patch as a fundamental base building block, to not only fix the current brokenness of the installer, but also and much more importantly, to rewrite and redesign the installer in significant ways — i.e., to use a proper kernel, and actually more significantly, to potentially move the majority of the installer operations into the regular Drupal environment.

As mentioned earlier already, with this patch we're this ---> <--- close to perform the actual installation of Drupal in a module. Just today, I (erroneously) installed Drupal without System module getting enabled by the installer.

In short, this patch is a big leap towards turning the "installer" into a minimal micro interface that is primarily used for the very first 1-3 installer screens to configure the fundamental basics of the application only. Once those settings are in place, the entire remaining processing happens in a regular Drupal environment, without any special casing or conditions. As a result, this patch enables us to simplify a ton of things that are currently special-cased for the installer.

Crell’s picture

+++ b/core/includes/install.core.inc
@@ -274,94 +271,98 @@ function install_begin_request(&$install_state) {
+  // Required for module_list() override in early installer environment.
   require_once DRUPAL_ROOT . '/core/includes/file.inc';

I'm fairly certain this is going to conflict in exciting ways with #1331486: Move module_invoke_*() and friends to an Extensions class

Reading through the code, I don't understand all of it (but then, who does understand the installer...) or how it really handles the split in question, but I didn't see anything that jumped out at me as eeek worthy other than the above patch conflict.

sun’s picture

FileSize
36.59 KB

Merged HEAD.

This effectively reverts the messy hack from #1814916: Convert menus into entities now (sorry, I linked to the wrong issue in #121), since the overall changes of this patch make the SystemBundle available at the correct and required time when System module is installed.

chx’s picture

OK, so that does introduce a tangible benefit, the first I am actually seeing and acknowledging. Can we discuss the weird bootstrap workflow a bit? I raised the problem already in #114 but got no reaction to it. What about making a fake session backend instead of skipping it? For example, does the skipping of session also skip prefilling global $user -- I really would like to not introduce new weirdness.

+  // If we are in the interactive installer, all we need to do is to set the
+  // 'bootstrap' parameter. Doing so will trigger a redirect and the next page
+  // request will perform a full boostrap into the new environment, skipping
+  // install_system_rebuild().

Are you sure using bootstrap phases to trigger a redirect is a good idea? Can't make this more explicit? Set a redirect in install_state, make it happen in install_run_task?

As for installing Drupal without system, as long as system carries system_element_info, system_library_info and perhaps a couple more info hooks I cant remember off head, it needs to be in the module list. Not installing it properly is a much more interesting question, eventually, if your profile provides a cache backend, a key-value storage, a queue, lock, session, path and even replaces menu -- then and only then can we not install system_schema. I know there's an issue to get cache tables installed on demand, that's a good idea, it'd decrease testing time (and I wouldnt it recommend doing it non-testing time). All this has little to do with the issue at hand, because with or without this patch all these services are required and to achieve this schema-less system install, your profile needs to task_alter the system install task out and add its own in.

sun’s picture

I'm sorry, my comment about installing Drupal without System module was meant as a gimmick and not supposed to spawn an actual discussion.

re: #124:

  1. Fake session backend: I don't think it makes sense and I don't want to duct-tape this further. The new code contains a @todo for #335411: Switch to Symfony2-based session handling already, and that's what we actually want and need to do. I don't want to duplicate that major effort.

  2. Bootstrap redirects: Yes, I still think that's a good idea. It's part of the essential change here. Once the session handling is replaced with a swappable service/backend, there will only be exactly one bootstrap/environment change in the installer: Pre-storage/pre-session vs. Full environment.

    Right now, session handling depends on {users}. But {users} is only installed when User module's schema is installed. I already tried to hack the relevant parts of session.inc for this patch, but the dependencies are wired too deeply, and the hacks would have broken essential session error handling elsewhere.

    Once session is untangled from User module, that will allow us to remove the weird special-casing that exists for User module right now, which is installed right after System module, so as to ensure that Batch API is able to operate. That is, because Batch API requires a user session to exist, in order to map and validate a batch processing request. Both System module and User module are removed from the actual installation profile module installation batch and installed upfront to make the batch possible. In an ideal world, both modules would just be installed like any other module.

    As a result, this current patch actually has to deal with three steps: 1) Nothing 2) System + User setup 3) The Rest. — which means two instead of three distinct phases. However, I did not want to make the second step more official than it needs to be, given that it will cease to exist.

    Therefore, there is only one install task, install_system_rebuild(), which is skipped for the interactive installer (the interactive installer simply redirects into the full environment), and which is the single install task + kill-switch between 1) and 3) already, which is essentially what you're asking for.

sun’s picture

FileSize
36.55 KB

Merged HEAD (new Lock service conflicts).

Berdir’s picture

+++ b/core/includes/install.core.incundefined
@@ -282,84 +279,97 @@ function install_begin_request(&$install_state) {
+    $conf['keyvalue_default'] = 'keyvalue.memory';
+    $container
+      ->register('keyvalue', 'Drupal\Core\KeyValueStore\KeyValueFactory')
+      ->addArgument(new Reference('service_container'));
+    $container
+      ->register('keyvalue.memory', 'Drupal\Core\KeyValueStore\KeyValueMemoryFactory');
     $container->register('lock', 'Drupal\Core\Lock\NullLockBackend');
+    $conf['cache_classes'] = array('cache' => 'Drupal\Core\Cache\MemoryBackend');

We could simplify the keyvalue part by directly registering the KeyValueMemoryFactory as 'keyvalue', that would not require the $conf change. I had the same idea for the cache in DIC issue.

The (not explicitly existing... ) interface for those factories is the same...

+++ b/core/includes/install.core.incundefined
@@ -852,15 +851,83 @@ function install_verify_requirements(&$install_state) {
+  // This phase is reached in the non-interactive installer only.
+  // We need to unset all overrides and rebuild all necessary facilities to get
+  // into a regular bootstrap state.
+  unset($conf['cache_classes']);
+  unset($conf['keyvalue_default']);
+  unset($conf['lock_backend']);

lock_backend is no more, so that can be removed.

And hm, this might be an argument against my previous suggestion using the memory factory directly. So that might not work :)

+++ b/core/includes/install.core.incundefined
@@ -852,15 +851,83 @@ function install_verify_requirements(&$install_state) {
+  // Ensure that custom service definitions in a prepared settings.php are
+  // applied. This effectively allows to install Drupal with custom Cache,
+  // KeyValue, and Lock backends in the non-interactive installer.
+  $conf_path = conf_path();
+  if (is_readable(DRUPAL_ROOT . '/' . $conf_path . '/settings.php')) {
+    include DRUPAL_ROOT . '/' . $conf_path . '/settings.php';

I know that @chx is working on a mongodb profile that automatically writes a settings.php that configures this accordingly. Has this been tested that this actually works as designed and not just "it should"? :) Not seeing anything wrong with it, just wanted to make sure.

I think we refer to keyvalue as key/value store or something like that, and not "KeyValue". And couldn't there be more services be replaced instead of just those?

+++ b/core/includes/install.core.incundefined
@@ -852,15 +851,83 @@ function install_verify_requirements(&$install_state) {
+  // Bootstrap, but avoid session initialization.
+  // @see install_begin_request()
+  drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES);
+  _drupal_bootstrap_code();
+  $kernel = new DrupalKernel('prod', TRUE, drupal_classloader(), FALSE);

Yep, the session workarounds are ugly. I hope the symfony session replacement will make it in, there hasn't been much progress recently.

+++ b/core/lib/Drupal/Core/Config/FileStorage.phpundefined
@@ -173,9 +173,10 @@ public function encode($data) {
-    // A simple string is valid YAML for any reason.
+    // A simple string is valid YAML for any reason. Also, a config file may
+    // exist but can be empty, in which case $data is NULL.
     if (!is_array($data)) {
-      return FALSE;
+      return array();

Don't know this part well enough, could this not have any unexpected side effects? There might have been a reason for this to return FALSE and not array()?

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/StateSystemUpgradePathTest.phpundefined
@@ -35,6 +35,19 @@ public function testSystemVariableUpgrade() {
+    $expected_state['system.path_alias_whitelist'] = array(
+      'value' => array(
+      ),
+      'variable_name' => 'path_alias_whitelist',

Interesting, I was not aware that this already was converted. I guess this means that the alias cache issue should remove this upgrade again and remove the variable instead.

Berdir’s picture

Ah. See discussion in #1798884: Convert path_alias_whitelist to state system , it was commited but the upgrade path is bogus (it is regenerated anyway) and the alias cache issue will remove it completely.

=> I suggest to remove test coverage for that. It will go away one way or another.

chx’s picture

> I know that @chx is working on a mongodb profile that automatically writes a settings.php that configures this accordingly

Hrm, I wasn't thinking on that but that's an idea. I was more thinking along the lines of profile task alter and then container overrides, I would like to avoid settings() overrides as much as possible -- although I guess it might not be fully avoidable.

sun’s picture

Note that I'm currently testing whether this patch helps to resolve the installer problems of #1331486: Move module_invoke_*() and friends to an Extensions class in a combined patch that merges the ModuleHandler patch onto this patch: http://drupal.org/node/1766036#comment-6935512

Since the ModuleHandler installer problems seem to be identical to the problems of all other issues that are running into this issue, I expect that combined patch to come back green, or at least get the ModuleHandler patch back to the same remaining test failures it had before.

In any case, I'll help to resolve the installer issues of the ModuleHandler patch. It requires some good amount of polishing anyway, and I want to work on that either way.

[A] profile that automatically writes a settings.php that configures this accordingly. Has this been tested that this actually works as designed and not just "it should"?

No, this hasn't really be tested with actual settings.php overrides. We can investigate and do that in a follow-up issue, if needed (it's a new feature to begin with). As long as the profile or whatever kind of code finds a way to plug itself into the installer's code that actually writes and secures settings.php (which I don't think is possible right now), then whatever is in settings.php will be read in. Also note that this code path is only reached for non-interactive installations — an interactive installation simply reads whatever is in settings.php and does not involve any in-memory overrides.

There might have been a reason for this to return FALSE and not array()?

I authored the original code and can confirm that there was no particular reason for returning FALSE aside from common Drupal programming patterns. :) The code did not and does not account for the possibility of a configuration file that exists, but which is empty by design - which is the case for the system.module.disabled config file, since no modules are disabled by default - but yet, the config file should exist as default config.

Changes:

  1. Removed obsolete unset of $conf['lock_backend'].
  2. Removed irrelevant path_alias_whitelist variable from StateSystemUpgradePathTest.
effulgentsia’s picture

Even though I don't know all the intricacies of our installer, chx asked me to provide another pair of eyes on this. I read #130 and it seems totally fine to me. I think a lot of it is a good cleanup.

There's an @todo to deal with the session-related WTFs once #1858196: [meta] Leverage Symfony Session components and friends are implemented, so I don't think those WTFs should hold up this issue.

The addition of a 'bootstrap' request parameter that's equal to the value of a bootstrap phase seems a bit weird to me: my perhaps naive expectation would be to use an 'install_phase' parameter instead. But I think this is a relatively minor point that could be discussed/refined in a follow up or postponed until after we've reduced Drupal to a single bootstrap phase anyway.

+++ b/core/includes/install.core.inc
@@ -852,15 +851,82 @@ function install_verify_requirements(&$install_state) {
+  // Something in the installer bootstrap primes file_get_stream_wrappers() too
+  // early. Resetting that static alone doesn't resolve it.
+  drupal_static_reset();

Is this still the case after #1864292-11: Installation in non-English language fails?

All in all, I'm tempted to RTBC this, but would like to see at least one more comment from Berdir and/or chx in case they have remaining concerns.

chx’s picture

One more annoying question: for installing system, why can't we manually set schema and install the config and then call module_enable instead of copy-pasting the whole function?

Also, the extensions patch is green without this.

sun’s picture

re: #131:
Yep, I just tested and that's still the case. Stream wrappers are primed with an empty list, even though System module has been installed already. That seems to be a general bug with module_enable(). However, as the inline comment states, just resetting the stream wrappers static isn't sufficient - the info seems to be additionally statically cached somewhere else. I hope we have an issue somewhere to convert stream wrappers into services, so this slightly weird issue will be eliminated.

re: #132:

Good idea.

Changed drupal_install_system() to only perform install tasks, then dispatch to module_enable().

Extensively tested the non-interactive + interactive installer once more with that change.

David_Rothstein’s picture

I think several of the issues I raised in #80 are still valid, but won't have time for an in-depth review the next few days.

But particularly on #80.2, I originally thought it was more of a theoretical concern, but actually the patch itself illustrates the problem.

First it does this:

@@ -434,8 +433,8 @@ function install_run_tasks(&$install_state) {
....
-      if ($install_state['database_tables_exist'] && ($task['run'] == INSTALL_TASK_RUN_IF_NOT_COMPLETED || $install_state['installation_finished'])) {
-        variable_set('install_task', $install_state['installation_finished'] ? 'done' : $task_name);
+      if ($task['run'] == INSTALL_TASK_RUN_IF_NOT_COMPLETED || $install_state['installation_finished']) {
+        state()->set('system.install_task', $install_state['installation_finished'] ? 'done' : $task_name);

So that the variable is now being "set" regardless of whether there's a database or not.

Then later it says this:

+function install_system_schema(&$install_state) {
.....
+  // This task is the one that sets up persistent storage tables. Since this
+  // task is executed in the early installer environment, install_run_tasks()
+  // is not able to 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.

It does not make sense to me to set up a situation where state()->set() is called "successfully" but then you can't trust that it was actually recorded anywhere you'll be able to read later. It's a complicated issue either way, but this adds more complexity and I haven't yet figured out why it needs to.

sun’s picture

re: #134:
Note that the comment talks about the interactive installer only. In the non-interactive installer, the information is actually recorded and read in again, since the entire installation is performed within a single process.

In general, the usage of memory backend implementations in the pre-storage environment means a performance improvement for the non-interactive installer, since collected data is actually stored somewhere instead of being thrown away.

So yes, the state()->set() is successful, and it should be successful, because a subsequent get() in the non-interactive installer will be successful, too. The entire point here is to remove as much special-casing from the installer as possible. The less special-casing goes on there, the easier it is to work and interact with the installer and maintain it.

I don't see how this adds complexity to the installer — in my experience with developing this patch, complexity was only removed and everything became simpler. Can you explain a bit more why you think that this adds complexity?

effulgentsia’s picture

Assigned: sun » catch
Status: Needs review » Reviewed & tested by the community

I think this is at a point where it makes sense for catch to evaluate both the #133 patch, and David's concerns in #80, especially #80.2 (note that Chrome and Safari seem to not show the "2", but Firefox does -- it's the blockquote "The installer needs to use state()"...).

sun’s picture

FileSize
35.31 KB

Merged HEAD (safety).

catch’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Needs work

On David's concern here:

I don't understand why having state storage in memory ever makes sense. Doesn't this mean that any code which calls state()->set() before the database is available will appear to succeed, but in practice the data is never stored permanently?

It actually seems like an architectural problem that it's even possible to swap temporary storage and persistent storage in this way... But regardless of whether it's possible, it seems like a bad idea to do it.

So swapping out temporary and persistent is valid for the case of unit tests, and potentially in some cases for drush as well (for example I've worked on migrations where due to performance we wanted to disable certain hooks / services during the migration since they'd be writing somewhere every entity_save() then immediately overwritten again, in some cases adding hours to the migration). I'm not sure we can lock it down to stop people doing that or that we'd want to.

On having to use state when it doesn't actually exist, this sounds like something that should go into $_SESSION then. Except we can't have a session without the {session} table. But... once the Symfony session handling is in, I think that ships with a PHP native session implementation, which ought to work at any time without the database dependency, so perhaps we could switch to using that for the first steps of the installer? Given this I'm fine with this change for now - while it's a wtf, it's a small price to pay to get rid of lots of other wtfs as this patch does. But let's add a @todo to try to replace it later, or better an explicit followup issue.

I didn't read through the latest versions of the patch yet apart from that bit. Not sure if this is really RTBC or 'needs feedback' RTBC so back to CNR for now.

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community

I think this is real RTBC then. Unless someone says otherwise. @sun: want to file the followup issue requested in #138?

sun’s picture

Note that it's OK if #1331486: Move module_invoke_*() and friends to an Extensions class lands first - I think it should be RTBC.

I have a good, in-depth grasp of both in the meantime, so I don't really care which one I'll have to re-roll. ;) Both patches are unblocking work elsewhere, and there was some more "rush" on the other issue very recently (though I didn't understand why).

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

Working on a longer response, but in the meantime, if I install Drupal with this patch and then visit http://drupal.dev/core/install.php afterwards, it lets me go through the first few stages of the installer (including the requirements screen) before stopping me and telling me that Drupal is already installed. It's supposed to stop you right away and tell you that Drupal is already installed.

This is a problem that could lead to unwanted information disclosure, among other issues.

Crell’s picture

Except we can't have a session without the {session} table. But... once the Symfony session handling is in, I think that ships with a PHP native session implementation, which ought to work at any time without the database dependency, so perhaps we could switch to using that for the first steps of the installer?

That is one of the chief reasons for us to switch to the Symfony session handling system, and to refactor the installer to be Kernel/DI-esque so that swapping out the session backend is easy.

David_Rothstein’s picture

Some more feedback after continuing to look at the patch:

  1. I don't see how this adds complexity to the installer — in my experience with developing this patch, complexity was only removed and everything became simpler. Can you explain a bit more why you think that this adds complexity?

    I think it definitely simplifies certain parts of it, but overall adds complexity because it appears to add new and divergent code paths, for example the skipping of install_system_rebuild() for interactive installations only. (Right now, the interactive and non-interactive installer mostly follow the same code path, at least as much as possible.) The 'bootstrap' parameter in the URL also adds new code paths in the early stages of the installer.

    In addition:

    $ diffstat installer.services.137.patch 
     12 files changed, 260 insertions(+), 198 deletions(-)
    

    Although the patch has lots of code comments, so maybe that part evens out :)

  2. So swapping out temporary and persistent is valid for the case of unit tests, and potentially in some cases for drush as well (for example I've worked on migrations where due to performance we wanted to disable certain hooks / services during the migration since they'd be writing somewhere every entity_save() then immediately overwritten again, in some cases adding hours to the migration). I'm not sure we can lock it down to stop people doing that or that we'd want to.

    I don't believe the Drush use case is a good example. What you're looking for there is basically the Drupal 8 equivalent of $GLOBALS['conf']['some_variable'] = 'some_temporary_value', which is a valid use case; however, this should be done via a dedicated method which explicitly sets the variable temporarily. No matter what, variable_set() always attempted to set the variable persistently, and so should state()->set().

    I can see how unit tests might be an exception to that, but only because in a test the entire Drupal environment is being created and destroyed in the same PHP request (such that "temporary" and "persistent" storage are the same thing); this is, of course, an extreme, extreme edge case.

    The interactive installer here is, like the Drush use case, not a valid example of mixing temporary and persistent storage. Nor do I think the non-interactive installer is; while the installation itself happens in one request, your Drupal site still lives on for years afterwards (well hopefully :) - so any state information which the installer tries to save should either be saved in a persistent way for the site to be able to access later on, or it should explicitly fail.

    Thinking about this more, in #80 I suggested that we add an installer-specific key-value storage to deal with this issue; @sun responded that we should not be adding installer-specific implementations. However, along the lines of the Drush example, there are lots of use cases for overriding state variables temporarily. Thus, perhaps what we really want is a generic implementation that overrides all state variables with temporary (NULL?) fallbacks for the duration of the page request, so that persistent storage is never read from. This would mainly be useful for the installer, but could have other uses as well (e.g., a database down kind of situation).

    I would have to dig further into the patch to be sure, but I believe moving away from the memory-based key-value storage here would allow the patch to be simplified, as well as remove the possibility of very-hard-to-diagnose bugs later on.

  3. On having to use state when it doesn't actually exist, this sounds like something that should go into $_SESSION then. Except we can't have a session without the {session} table. But... once the Symfony session handling is in, I think that ships with a PHP native session implementation, which ought to work at any time without the database dependency, so perhaps we could switch to using that for the first steps of the installer? ... But let's add a @todo to try to replace it later, or better an explicit followup issue.

    Agreed that $_SESSION would be better for the 'install_task' variable, and that it's best handled in a followup (PHP native session handling gets a little more complicated on sites with multiple web nodes, and also more to point: we can't store this variable in $_SESSION as long as there is code which runs after the site is installed which still needs to access it, as is the case currently).

    I've come around to the idea that putting 'install_task' in state() is OK after all, but my point in #80 that this seems to go against the current documentation of the state() function still stands.

David_Rothstein’s picture

Title: Split installer into pre-storage & storage-exists phase » Split installer into pre-storage & storage-exists phases, and convert the install_task variable to use the state system

Restoring a title that explains more of what the patch does, including the original purpose of the issue.

I'm not actually convinced the two issues have to be done at the same time, but it doesn't hurt to either...

David_Rothstein’s picture

Component: configuration system » install system
chx’s picture

OK, this is interesting: while I can't reproduce the config entity query fails locally this patch definitely fixes them.

YesCT’s picture

Status: Needs work » Needs review
Issue tags: -Framework Initiative, -API clean-up, -Configuration system, -kernel-followup, -State system

#137: installer.services.137.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Configuration system, +kernel-followup, +State system

The last submitted patch, installer.services.137.patch, failed testing.

YesCT’s picture

Issue tags: +Needs reroll

Tagging for reroll

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
34.19 KB

I think I have correctly re-rolled this. So here goes, lets see what the test bot thinks.

Status: Needs review » Needs work

The last submitted patch, drupal-installer_services-1798732-150.patch, failed testing.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
34.22 KB

OK needed a slight correction there. Removed a stray closing curly brace, and some redundant documentation.

Status: Needs review » Needs work

The last submitted patch, druapl-installer_services-1798732-152.patch, failed testing.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
34.29 KB

Spectacular fail. I think this next one may be better. If not then this is beyond me.

Status: Needs review » Needs work

The last submitted patch, drupal-installer_services-1798732-154.patch, failed testing.

YesCT’s picture

@mbrett5062 was there a conflict that was confusing? you could post it here to get some advice on it.

mbrett5062’s picture

The issue here now is that since the original patch the handling of modules has been passed on to a module_handler. So the re-initialization of system in install.core.inc needs to be handled by those.

So this snippet:

  // Reset module list and hook implementation statics.
  module_list_reset();
  module_load_all(FALSE, TRUE);
  system_list_reset();
  module_implements_reset();

No longer works.

And needs something like the following from module.inc

      $module_handler->setModuleList($module_filenames);
      $module_handler->load($module);
      module_load_install($module);

      // Reset the the hook implementations cache.
      $module_handler->resetImplementations();

      // Flush theme info caches, since (testing) modules can implement
      // hook_system_theme_info() to register additional themes.
      system_list_reset();

The problem is, that I am not sure if this is all needed at the stage of initialization. And it would also require the list of $modeule_filenames, but I am not sure which and how to set that up.

There was of course also some conflict debug text left in the patch, which did not help :(

So here is a cleaned patch, which still will not work. I can go no further, if some one would like to continue, either address the change to $module_handler, or restart from @sun's last good patch in #137, and ignore my patches.

jibran’s picture

Priority: Normal » Critical

According to sun in #1067408-103: Themes do not have an installation status

#1798732: Split installer into pre-storage & storage-exists phases, and convert the install_task variable to use the state system is still required and has to land first, but got blocked a second time on a discussion about technical perfectionism.

As #1067408: Themes do not have an installation status is major so bumping it to critical.

thedavidmeister’s picture

Status: Needs work » Needs review

setting to needs review just so other can see how tests are failing.

Status: Needs review » Needs work

The last submitted patch, druapl-installer_services-1798732-157.patch, failed testing.

juanolalla’s picture

juanolalla’s picture

Status: Needs work » Needs review

setting to needs review

Status: Needs review » Needs work
star-szr’s picture

@juanolalla worked on this reroll during core mentoring today, he said he will take a look at re-rolling #157 :)

juanolalla’s picture

Status: Needs work » Needs review
FileSize
32.68 KB

Re-rolled #157

Status: Needs review » Needs work
chx’s picture

Issue tags: -Needs reroll

I think this needs more than a simple reroll so I am removing that tag. Also, I am inclined to postpone this on the installer tests. The extreme reluctance to move forward with this is due to the entirely missing test coverage here. We have that coming however, I have the first, passing test in my sandbox.

moshe weitzman’s picture

I think the installer tests that chx mentions have landed. Are we clear to proceed here? Anyone available?

jibran’s picture

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Configuration system, +kernel-followup, +State system
chx’s picture

re #168 i do not see #1961938-49: Test the interactive installer in core yet...

jibran’s picture

How can we move forward here? #1961938-56: Test the interactive installer is committed.

markhalliwell’s picture

Ok, here is a re-roll from HEAD. It is by no means working, at all... but at least this gets us somewhere more current.

markhalliwell’s picture

Fixed some whitespace errors I introduced.

markhalliwell’s picture

Not sure, but we might wanna postpone this until #1858196: [meta] Leverage Symfony Session components can land?

andypost’s picture

There's no more {system} table

catch’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
3.37 KB
1.66 KB

Several patches have gone in now which were slowly re-implementing the overall concept here piece by piece, making this a straight conversion.

Uploading one patch that only does task + time, and another that also does install_current_batch.

These worked for me separately and together locally. With the combined patch the variable system is no longer used in the installer.

catch’s picture

Title: Split installer into pre-storage & storage-exists phases, and convert the install_task variable to use the state system » Convert install_task, install_time and install_current_batch to use the state system

Status: Needs review » Needs work

The last submitted patch, combined.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
4.21 KB
858 bytes

Missed a bit.

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -API clean-up, -Configuration system, -kernel-followup, -State system

The last submitted patch, install.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
Issue tags: +Framework Initiative, +API clean-up, +Configuration system, +kernel-followup, +State system

#180: install.patch queued for re-testing.

chx’s picture

Status: Needs review » Reviewed & tested by the community

mmmm state() works this well in installer? Pleasant surprise.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Framework Initiative, -API clean-up, -Configuration system, -kernel-followup, -State system

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

klonos’s picture

...restoring tags eaten by the tag monster.

sun’s picture

The original and more complex problem space that I tried to address in patches up until #137 here has re-appeared now, but this time as a built-in bug/feature:

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

Core has advanced in the meantime, so I'll try to see how much of the relevant changes are still compatible, and whether they will fix the problem.