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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tayzlor’s picture

Status: Active » Needs review
FileSize
5.57 KB
andreiashu’s picture

Status: Needs review » Needs work

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

tayzlor’s picture

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.

tayzlor’s picture

Status: Needs work » Needs review
alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Looks good! Thanks.

alexpott’s picture

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

tayzlor’s picture

Status: Needs work » Needs review
FileSize
7.63 KB

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.

alexpott’s picture

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).

sun’s picture

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.

alexpott’s picture

Status: Postponed » Needs review
FileSize
9.95 KB

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.

aspilicious’s picture

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
9.88 KB
777 bytes

Whoops!

sun’s picture

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?

alexpott’s picture

Good points :) ... whoops again!

sun’s picture

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.

aspilicious’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

pcambra’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +State system
alexpott’s picture

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

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Now actually doing what I said I would in #20

catch’s picture

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.

Albert Volkman’s picture

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

Albert Volkman’s picture

Status: Needs work » Needs review
aspilicious’s picture

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Still green? GO!

David_Rothstein’s picture

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).

Berdir’s picture

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

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

gdd’s picture

Status: Needs work » Needs review
FileSize
8.36 KB

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.

Cameron Tod’s picture

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.

xjm’s picture

#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.

mtift’s picture

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.

mtift’s picture

Issue summary: View changes

mtift updated issue summary.

mtift’s picture

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.

mtift’s picture

Status: Needs work » Needs review
FileSize
8.67 KB

Here's a cleaned-up version.

Status: Needs review » Needs work

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

mtift’s picture

Status: Needs work » Needs review
FileSize
8.67 KB

MIssed a parenthesis

Status: Needs review » Needs work

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

mtift’s picture

Status: Needs work » Needs review
FileSize
644 bytes
8.65 KB

Let's try this again

Status: Needs review » Needs work

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

catch’s picture

Priority: Normal » Critical

Bumping priority since this blocks variable_*() removal.

ianthomas_uk’s picture

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

ianthomas_uk’s picture

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.

ianthomas_uk’s picture

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')

ianthomas_uk’s picture

Status: Needs work » Needs review
Issue tags: -State system
FileSize
1.87 KB
8.89 KB

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.

ianthomas_uk’s picture

Issue tags: +State system
FileSize
967 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.

catch’s picture

+++ 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.

ianthomas_uk’s picture

Status: Needs work » Needs review
FileSize
9.21 KB

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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
12.32 KB
4.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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
14.37 KB
3.25 KB

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

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.