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):
Related Issues
#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
Comment | File | Size | Author |
---|---|---|---|
#54 | 52-54-interdiff.txt | 3.25 KB | alexpott |
#54 | 1798738-54.patch | 14.37 KB | alexpott |
#52 | 50-52-interdiff.txt | 4.69 KB | alexpott |
#52 | 1798738-52.patch | 12.32 KB | alexpott |
#50 | 1798738-css-js-query_string-50.patch | 9.21 KB | ianthomas_uk |
Comments
Comment #1
tayzlor CreditAttribution: tayzlor commentedComment #2
andreiashu CreditAttribution: andreiashu commentedHey Graham,
One thing that I also forgot when started was the default value. You can use the ?: operator
Comment #3
tayzlor CreditAttribution: tayzlor commentedNew 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.
Comment #4
tayzlor CreditAttribution: tayzlor commentedComment #5
alexpottLooks good! Thanks.
Comment #6
alexpottThis 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
Comment #7
tayzlor CreditAttribution: tayzlor commentedNew 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.
Comment #8
alexpottRerolled 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).Comment #9
sunFor 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.
Comment #10
alexpottNow 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.Comment #11
aspilicious CreditAttribution: aspilicious commentedThese lines should be removed
Comment #12
alexpottWhoops!
Comment #13
sunLooks good. Remaining tasks:
Double ;;
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)
Commented out?
Comment #14
alexpottGood points :) ... whoops again!
Comment #15
sunThanks!
Comment #17
aspilicious CreditAttribution: aspilicious commented#14: 1798738-14-css_js_query_string.patch queued for re-testing.
Comment #19
pcambra#14: 1798738-14-css_js_query_string.patch queued for re-testing.
Comment #20
alexpottResetting to RTBC (as per #15) since the failures where due to testbot issues.
Comment #21
alexpottNow actually doing what I said I would in #20
Comment #22
catch#14: 1798738-14-css_js_query_string.patch queued for re-testing.
Comment #24
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll as some methods were renamed with this commit.
Comment #25
Albert Volkman CreditAttribution: Albert Volkman commentedComment #26
aspilicious CreditAttribution: aspilicious commented#24: css_js_query_string-1798738-24.patch queued for re-testing.
Comment #27
aspilicious CreditAttribution: aspilicious commentedStill green? GO!
Comment #28
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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).
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).
Comment #29
Berdir#24: css_js_query_string-1798738-24.patch queued for re-testing.
Comment #31
gddHere'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.
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
It seems like we should find another place to unset this, because it means that only interactive installs will properly unset the value.
Comment #32
Cameron Tod CreditAttribution: Cameron Tod commentedHere'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.
Comment #33
xjm#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.Comment #34
mtiftHere'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:
In a few seconds, I'll update the issue summary.
Comment #34.0
mtiftmtift updated issue summary.
Comment #34.1
mtiftUpdated issue summary.
Comment #36
mtiftHere's a cleaned-up version.
Comment #38
mtiftMIssed a parenthesis
Comment #40
mtiftLet's try this again
Comment #42
catchBumping priority since this blocks variable_*() removal.
Comment #43
ianthomas_ukRegarding 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
Comment #44
ianthomas_ukOK, the reason for the test failure / exception is this change (and the corresponding JsCollectionRenderer code):
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.
Comment #45
ianthomas_ukAfter further digging I can see that state is initialised in Drupal\Core\CoreServiceProvider:
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')
Comment #46
ianthomas_ukPlease 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.
Comment #47
ianthomas_ukAdding back state system tag which has been lost somehow.
Here is an easier to read interdiff, without changes that only matter to git.
Comment #49
catchThe 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.
Comment #50
ianthomas_ukI see where you mean. Does this look better?
Patch attached is 40 plus the above.
Comment #52
alexpottJust need to use the container to inject state rather than calling out to to \Drupal because CssCollectionRenderer is unit tested.
Comment #54
alexpottCan remove variable_get() function definition from test and inject state into Drupal\Core\Asset\JsCollectionRenderer too
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedread through the patch, it looks good to go to me.
Comment #56
webchickCommitted and pushed to 8.x. Thanks!
Comment #57.0
(not verified) CreditAttribution: commentedUpdated issue summary.