Updated: Comment #34

Problem/Motivation

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

Original Goal

Convert the 'css_js_query_string' variable to the State system.

Problem

There is a fatal error on a fresh install without installer changes

Proposed Solution

During installation, check to see if the database is available and if it is not, use MemoryStorage for state.

Remaining Tasks

Determine if mixing database (persistent) and memory (temporary) storage will cause additional problems.
Determine source of fatal error (caused by #34):

#1798732: Convert install_task, install_time and install_current_batch to use the state system
#1849004: One Service Container To Rule Them All
#1934508: Cache clear doesn't affect logo or favicon

Files: 
CommentFileSizeAuthor
#54 52-54-interdiff.txt3.25 KBalexpott
#54 1798738-54.patch14.37 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,555 pass(es).
[ View ]
#52 50-52-interdiff.txt4.69 KBalexpott
#52 1798738-52.patch12.32 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#50 1798738-css-js-query_string-50.patch9.21 KBianthomas_uk
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#47 40-46-interdiff.txt967 bytesianthomas_uk
#46 1798738-css-js-query_string-46.patch8.89 KBianthomas_uk
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#46 40-46-interdiff.txt1.87 KBianthomas_uk
#40 1798738-css-js-query_string-40.patch8.65 KBmtift
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#40 interdiff.txt644 bytesmtift
#38 1798738-css-js-query_string-38.patch8.67 KBmtift
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#36 1798738-css-js-query_string-36.patch8.67 KBmtift
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/common.inc.
[ View ]
#34 1798738-css-js-query_string-34.patch8.55 KBmtift
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#32 1798738-css-js-query_string-32.patch7.82 KBcam8001
PASSED: [[SimpleTest]]: [MySQL] 52,594 pass(es).
[ View ]
#32 system.module.rej_.txt453 bytescam8001
#31 1798738-css-js-query_string-31.patch8.36 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 50,701 pass(es).
[ View ]
#24 css_js_query_string-1798738-24.patch9.94 KBAlbert Volkman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch css_js_query_string-1798738-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 12-14-interdiff.txt1.36 KBalexpott
#14 1798738-14-css_js_query_string.patch9.9 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1798738-14-css_js_query_string.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 10-12-interdiff.txt777 bytesalexpott
#12 1798738-12-css_js_query_string.patch9.88 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 46,426 pass(es).
[ View ]
#10 1798738-10-css_js_query_string.patch9.95 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 46,442 pass(es).
[ View ]
#8 1798738-8-css_js_query_string.patch10.23 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 42,185 pass(es).
[ View ]
#7 1798738-css_js_query_string.patch7.63 KBtayzlor
PASSED: [[SimpleTest]]: [MySQL] 41,898 pass(es).
[ View ]
#3 1798738-css_js_query_string.patch5.63 KBtayzlor
PASSED: [[SimpleTest]]: [MySQL] 41,909 pass(es).
[ View ]
#1 1798738-css_js_query_string.patch5.57 KBtayzlor
PASSED: [[SimpleTest]]: [MySQL] 41,903 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new5.57 KB
PASSED: [[SimpleTest]]: [MySQL] 41,903 pass(es).
[ View ]

Status:Needs review» Needs work

Hey Graham,
One thing that I also forgot when started was the default value. You can use the ?: operator

StatusFileSize
new5.63 KB
PASSED: [[SimpleTest]]: [MySQL] 41,909 pass(es).
[ View ]

New patch that provides the default '0' value.

I dont think we need an upgrade path for this as _drupal_flush_css_js() will get called on update.php which then sets the css_js_query_string variable.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

Looks good! Thanks.

Status:Reviewed & tested by the community» Needs work

This causes a fatal error on a fresh install

Fatal error: Call to undefined function Drupal\Core\KeyValueStore\db_query() in /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.php on line 39

Status:Needs work» Needs review
StatusFileSize
new7.63 KB
PASSED: [[SimpleTest]]: [MySQL] 41,898 pass(es).
[ View ]

New patch which does a check in the installer to see if the database is available and if it isn't it uses MemoryStorage for state, which fixes the install issue.

StatusFileSize
new10.23 KB
PASSED: [[SimpleTest]]: [MySQL] 42,185 pass(es).
[ View ]

Rerolled and added variable clean up update in system.install.

The only difficult thing about this patch is that fact that the maintenance theme uses drupal_pre_render_styles() which means that we need a keyvalue service on the container before the database exists. The current KeyValueFactory now hardcodes a dependency on DatabaseStorage. This patch add a constructor to the factory so we can inject the storage class during install time (if necessary).

Status:Needs review» Postponed

For the KeyValueFactory changes, see #1809206: KeyValueFactory hard-codes DatabaseStorage

Postponing on that. The installer changes won't be covered over there though, so we'll have to keep them here.

Status:Postponed» Needs review
StatusFileSize
new9.95 KB
PASSED: [[SimpleTest]]: [MySQL] 46,442 pass(es).
[ View ]

Now that #1809206: KeyValueFactory hard-codes DatabaseStorage has landed I've rerolled.

This patch adds the keyvalue service into the installer's container and uses $conf['keyvalue_default'] so that the install uses keyvalue's memory storage before the db is available. Once the database is available this override is removed.

Status:Needs review» Needs work

+++ b/core/includes/install.core.incundefined
@@ -322,6 +327,13 @@ function install_begin_request(&$install_state) {
+    //$container->register('keyvalue', 'Drupal\Core\KeyValueStore\KeyValueFactory')
+    //  ->addArgument('Drupal\Core\KeyValueStore\MemoryStorage');

These lines should be removed

Status:Needs work» Needs review
StatusFileSize
new9.88 KB
PASSED: [[SimpleTest]]: [MySQL] 46,426 pass(es).
[ View ]
new777 bytes

Whoops!

Looks good. Remaining tasks:

+++ b/core/includes/install.core.inc
@@ -322,6 +327,12 @@ function install_begin_request(&$install_state) {
+      ->addArgument(new Reference('service_container'));;

Double ;;

+++ b/core/modules/system/system.install
@@ -2203,6 +2203,15 @@ function system_update_8032() {
/**
+ * Cleans up css_js_query_string variable.

Since we're only deleting variables and not converting them, we can happily add this to the existing previous system update, which also just deletes a state variable. (and adjust the phpDoc accordingly)

+++ b/core/modules/system/system.install
@@ -2203,6 +2203,15 @@ function system_update_8032() {
+/*function system_update_8033() {
+  update_variable_del('css_js_query_string');
+}
+*/

Commented out?

StatusFileSize
new9.9 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1798738-14-css_js_query_string.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.36 KB

Good points :) ... whoops again!

Status:Needs review» Reviewed & tested by the community

Thanks!

Status:Reviewed & tested by the community» Needs work
Issue tags:-Configuration system, -State system

The last submitted patch, 1798738-14-css_js_query_string.patch, failed testing.

Status:Needs work» Needs review

#14: 1798738-14-css_js_query_string.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1798738-14-css_js_query_string.patch, failed testing.

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

#14: 1798738-14-css_js_query_string.patch queued for re-testing.

Resetting to RTBC (as per #15) since the failures where due to testbot issues.

Status:Needs review» Reviewed & tested by the community

Now actually doing what I said I would in #20

Status:Reviewed & tested by the community» Needs work
Issue tags:+Configuration system, +State system

The last submitted patch, 1798738-14-css_js_query_string.patch, failed testing.

StatusFileSize
new9.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch css_js_query_string-1798738-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll as some methods were renamed with this commit.

Status:Needs work» Needs review

#24: css_js_query_string-1798738-24.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Still green? GO!

Status:Reviewed & tested by the community» Needs review

   // This override is reverted as soon as the config directory has been set up
   // successfully.
   // @see drupal_install_config_directories()
-  if (!$install_state['config_verified']) {
+  if (!$install_state['config_verified'] || !$install_state['database_verified']) {

This would seem to make the code comment no longer accurate... See also #1849004: One Service Container To Rule Them All which makes some similar changes here (and which would be better to commit first).

@@ -322,6 +327,12 @@ function install_begin_request(&$install_state) {
...
+    $container->register('keyvalue', 'Drupal\Core\KeyValueStore\KeyValueFactory')
+      ->addArgument(new Reference('service_container'));
+    $container->register('keyvalue.memory', 'Drupal\Core\KeyValueStore\KeyValueMemoryFactory');
+    // Override the default keyvalue storage to use memory as the database is
+    // not available.
+    $conf['keyvalue_default'] = 'keyvalue.memory';

Mixing database (persistent) and memory (temporary) storage like that still seems really iffy. See discussion in #1798732: Convert install_task, install_time and install_current_batch to use the state system (although I haven't had time to circle back to some of the latest comments there yet).

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

The last submitted patch, css_js_query_string-1798738-24.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.36 KB
PASSED: [[SimpleTest]]: [MySQL] 50,701 pass(es).
[ View ]

Here's a reroll. A lot of the stuff in this patch has since been committed as parts of other patches in the meantime, so it just got removed, which is why the new patch is smaller.

This would seem to make the code comment no longer accurate... See also #1849004: One Service Container To Rule Them All which makes some similar changes here (and which would be better to commit first).

I believe the commit in that issue took care of this, so that hunk has been removed.

I don't have an answer to Dave's other comment however I will add one of my own

+++ b/core/includes/install.core.incundefined
@@ -1099,6 +1106,10 @@ function install_settings_form_submit($form, &$form_state) {
+  // The container is about to be rebuilt so we need to unset the keyvalue
+  // storage override that the installer is using.
+  unset($conf['keyvalue_default']);

It seems like we should find another place to unset this, because it means that only interactive installs will properly unset the value.

StatusFileSize
new453 bytes
new7.82 KB
PASSED: [[SimpleTest]]: [MySQL] 52,594 pass(es).
[ View ]

Here's a re-roll, with a @todo comment removed (see the .rej file). It looks like #1324618: Implement automated config saving for simple settings forms isn't happening, and systems_settings_form() is getting bumped entirely.

Does that have implications for this patch? Manually installing and testing locally, everything seemed fine.

#1934508: Cache clear doesn't affect logo or favicon introduces a couple new instances of this variable, so we'll need to convert those instances as well if that goes in first.

Agreed with @heyrocker; we probably need to get that unset out of the submit handler. Can someone test with drush? Also, let's update the issue summary explaining why the installer changes are necessary (see #6 on). Also summarize the concern from #28 under the remaining tasks section.

Issue tags:-Needs issue summary update
StatusFileSize
new8.55 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Here's another re-roll. A lot has changed since #32. The patch now applies cleanly, but it's causing another fatal error on a fresh install:

Fatal error: Method Drupal\Core\Template\RenderWrapper::__toString() must not throw an exception in /home/user/Sites/d8/core/themes/engines/twig/twig.engine on line 0

In a few seconds, I'll update the issue summary.

Issue summary:View changes

mtift updated issue summary.

Issue summary:View changes

Updated issue summary.

Status:Needs review» Needs work

The last submitted patch, 1798738-css-js-query_string-34.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.67 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/common.inc.
[ View ]

Here's a cleaned-up version.

Status:Needs review» Needs work

The last submitted patch, 1798738-css-js-query_string-36.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.67 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

MIssed a parenthesis

Status:Needs review» Needs work

The last submitted patch, 1798738-css-js-query_string-38.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new644 bytes
new8.65 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Let's try this again

Status:Needs review» Needs work

The last submitted patch, 1798738-css-js-query_string-40.patch, failed testing.

Priority:Normal» Critical

Bumping priority since this blocks variable_*() removal.

Regarding the error in #34, that's the theming layer eating your exception. I've filed that as https://drupal.org/node/2086219

The actual error as of #40 is:

The service definition "state" does not exist. in/Applications/MAMP/htdocs/drupal/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php on line 483

OK, the reason for the test failure / exception is this change (and the corresponding JsCollectionRenderer code):

--- a/core/lib/Drupal/Core/Asset/CssCollectionRenderer.php
+++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.php
@@ -24,7 +24,7 @@ public function render(array $css_assets) {
     // browser-caching. The string changes on every update or full cache
     // flush, forcing browsers to load a new copy of the files, as the
     // URL changed.
-    $query_string = variable_get('css_js_query_string', '0');
+    $query_string = \Drupal::state()->get('system.css_js_query_string') ?: '0';
     // Defaults for LINK and STYLE elements.
     $link_element_defaults = array(

The error is being thrown at the very start of the installation process, so I assume the problem is we haven't bothered building a state yet as we've not got anything to put in it, but I'm not sure what the appropriate fix would be. $query_string is used to generate a unique URL to avoid stale browser caches

Options I see are:
1. Build the state earlier so it is available by the time this is called. This sounds complicated, high risk and possibly impossible.
2. Detect that we're at the start of the installation process (by checking bootstrap level?) and skip this line, setting $query_string to REQUEST_TIME.

In manual testing, if I catch and ignore both exceptions then I can successfully install Drupal and load the homepage.

After further digging I can see that state is initialised in Drupal\Core\CoreServiceProvider:

  protected function registerModuleHandler(ContainerBuilder $container) {
    // The ModuleHandler manages enabled modules and provides the ability to
    // invoke hooks in all enabled modules.
    if ($container->getParameter('kernel.environment') == 'install') {
      // During installation we use the non-cached version.
      $container->register('module_handler', 'Drupal\Core\Extension\ModuleHandler')
        ->addArgument('%container.modules%');
    }
    else {
      $container->register('module_handler', 'Drupal\Core\Extension\CachedModuleHandler')
        ->addArgument('%container.modules%')
        ->addArgument(new Reference('state'))
        ->addArgument(new Reference('cache.bootstrap'));
    }
  }

This if was added in https://drupal.org/node/1331486#comment-6936068

I don't have access to a development environment to test this at the moment, and I can't see from that issue why you can't add a state argument to the ModuleHandler, which would be the patch required to implement #44 option 1.

If we prefer #44 option 2, the check I'd implement would be if ($container->getParameter('kernel.environment') == 'install')

Status:Needs work» Needs review
Issue tags:-State system
StatusFileSize
new1.87 KB
new8.89 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Please ignore #45, I jumped to conclusions without reading the code properly.

I don't think there's any way we'll be able to store this value at the start of the install process, so we'll have to cope without it. Here's a patch that should fix the test failures and ensures people won't get outdated css/js files by disabling css/js browser-side caching during the installation process.

I've also removed an extra blank like which seems to have been added accidentally.

Issue tags:+State system
StatusFileSize
new967 bytes

Adding back state system tag which has been lost somehow.
Here is an easier to read interdiff, without changes that only matter to git.

Status:Needs review» Needs work

The last submitted patch, 1798738-css-js-query_string-46.patch, failed testing.

+++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.php
@@ -24,7 +24,12 @@ public function render(array $css_assets) {
+    if (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'install') {

The installer using null storage ought to mean this is just 0 (which is fine). I see the key/value store being set to memory, I'd assume the state service also needs to be set up in the same place.

Status:Needs work» Needs review
StatusFileSize
new9.21 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

I see where you mean. Does this look better?

@@ -430,6 +437,11 @@ function install_begin_request(&$install_state) {
+    $container->register('state', 'Drupal\Core\KeyValueStore\KeyValueStoreInterface')
+      ->setFactoryService(new Reference('keyvalue'))
+      ->setFactoryMethod('get')
+      ->addArgument('state');
+

Patch attached is 40 plus the above.

Status:Needs review» Needs work

The last submitted patch, 1798738-css-js-query_string-50.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new12.32 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new4.69 KB

Just need to use the container to inject state rather than calling out to to \Drupal because CssCollectionRenderer is unit tested.

Status:Needs review» Needs work

The last submitted patch, 1798738-52.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.37 KB
PASSED: [[SimpleTest]]: [MySQL] 58,555 pass(es).
[ View ]
new3.25 KB

Can remove variable_get() function definition from test and inject state into Drupal\Core\Asset\JsCollectionRenderer too

Status:Needs review» Reviewed & tested by the community

read through the patch, it looks good to go to me.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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

Issue summary:View changes

Updated issue summary.