Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Steps to reproduce:
- Checkout beta 12
- Install
- Checkout HEAD
- Hit update.php directly
- Problem: JS doesn't work, as its still aggregated, see
js_QM6eTWWfVdmRmLKWmEL6T9u6dxQmFW3TjOahudJ9S54.js:669 Uncaught ReferenceError: drupalSettings is not defined
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#45 | 2560521-45.patch | 5.23 KB | plach |
#45 | 2560521-45.interdiff.txt | 1.23 KB | plach |
#41 | interdiff.txt | 3.49 KB | dawehner |
#41 | 2560521-41.patch | 5.22 KB | dawehner |
#36 | 2560521-35.patch | 5.68 KB | dawehner |
Comments
Comment #2
plachI'm not sure the problem is aggregation. I tried disabling it and I got the same problem. The whole theme seems to be broken in that case.
Comment #3
catchComment #4
catchIs it #2510104: Convert drupalSettings from JavaScript to JSON, to allow for CSP in the future?
Comment #5
dawehnerWell yeah the reason is that again ...
Steps I did
* Disabled aggregation
* Disabled assert caching
* But we still don't render core/drupalSettings before active-link.js
So yeah cache.discovery is the issue
Comment #6
Wim LeersI'd say "then dependencies must be wrong", but this has been the library definition since Feb 2014:
Comment #7
jhedstromThe file missing from the list of js files in #5 is
core/misc/drupalSettingsLoader.js
, which was introduced in #2510104: Convert drupalSettings from JavaScript to JSON, to allow for CSP in the future. The fix may be to just clear css/js cache (or disable completely) on the update.php route.Comment #8
dawehnerYeah we went with discussing about blackholes to this approach.
Comment #9
Wim Leers<WimLeers> dawehner: the downside is that the rest of the site, with potentially thousands of users hitting the maintenance page, they will continuously rebuild those caches. The beauty of selective amnesia/partial blackhole is that it ONLY affects update.php
Comment #10
Wim LeersSo dawehner is concerned that simply emptying all cache backends will cause nasty side-effects. I agree with his concern/caution.
But… to really solve all manifestations of this problem, shouldn't we actually be ensuring we never use any cached data in
update.php
? Aren't there other parts of theupdate.php
responses that in part or full come from some cache?Shouldn't we decorate all caches during
update.php
, and then invalidate all caches afterupdate.php
? i.e. shouldn't we makeupdate.php
map thecache_factory
service to\Drupal\Core\Cache\NullBackendFactory
?Comment #11
dawehnerJust to be clear, this patch certainly just fixes update.php, so we are kinda more save
We invalidate all caches after update.php already ... but well, don't use any cache is highly problematic given that this could invoke more subsystems again. Just imagine the entity type definition as we deal with maybe users somewhere.
Comment #12
Wim Leers… if that's problematic, then any site can run into that if those caches just happen to be empty already.
Comment #13
dawehnerWell, right, I would just try for now to remove the common case surface area. Just imagine if in the middle of one to the next batch we clear the cache. I just fear that this would make the system more fragile than before
Comment #14
Wim LeersFair.
Comment #15
catchWe could decorate caches to return FALSE from get and getMultiple() all the time - that way every update run gets the 'worst case' of caches being empty.
Caches will only get populated if something accesses them - which is either update.php itself or a specific update.
This seems less risky than a rebuild, but reducing surface area of update.php as we go along (so it doesn't load the cache items at all) will improve things. Then as discussed in irc we could try to hard-code the libraries update.php uses so it doesn't rely on library discovery at all.
I think that can probably all happen in a follow-up, patch needs to explain the fact that new code can make the library cache invalid and that update.php is relying on the new definitions though, 'make it work' doesn't quite get that across :P
Comment #16
catchAlso I think we could add a regression test for this:
- database that does not have aggregation on
- visit update.php
- confirm the drupal settings js file is in the markup.
Comment #17
Wim LeersAnd dawehner pointed out that it's in fact possible to test
update.php
.Comment #18
dawehnerWell, while I think we should explore that, its tricky, because
a) We want the cache entries have the most up to date information
b) We want the cache rebuild entries not to call out to the entirety of the world, which I fear could actually happen easily.
Just imagine ECK having to rely on something in order to rebuild the entity type definitions. This would increase the surface area A LOT, so I'm not entirely sure its the right thing to do.
Especially for this issue we want to first fix the JS side of things. Having to press f5 multiple times (its amazing that it though works) is kinda of a nogo.
Comment #19
Xano@lauriii and I were hacking on this a bit. This patch simply disables caching for the asset resolver during update.php runs.
Comment #20
larowlanworking on a test
Comment #21
larowlanSomething like so for the test @catch?
Comment #23
jhedstromMy understanding of the issue is that sites which have
library_info
cached don't pick up the new javascript file. If we manually populate that particular cache during the database load in the test, this would mimic such a site (all of the db dump files have empty cache tables).Comment #25
jhedstromHowever, that test fails, so it may be good as-is.
Comment #26
lauriiiFixed the exceptions and combined the patches.
Comment #28
larowlanComment #29
larowlanYeah so the test is dud, as per #23
The doPreUpdateTests needs to be after the ->continue link click.
Comment #30
dawehnerFixed a couple of things:
Just to be sure, this caused that no JS is loaded for that initial page, ... so yeah I changed DBUpdateController to always include it. I think this should be fne.
On the longrun though we really maybe want to implement our cache backend decorator
Comment #31
larowlanOne minor observation, but other than that I think this is RTBC
Should we use constants here instead of ints? less magic, self documenting etc
Comment #32
dawehnerWell, unlike other cases where we use ints for the fact that PHP does not have ENUMs here we are clearly using ints as ints.
Comment #33
dawehnerOne quick additional rename.
Comment #35
larowlanTrue - I think this is ready then
Comment #36
dawehnerQuick reroll
Comment #37
dawehnerTried to do an actual update and this problem is fixed by that particular patch.
Comment #38
larowlanComment #39
Wim LeersIt'd be great if this said why.
I guess this is why: to build a fresh container for every request to update.php. An
@see
to this in point 1 would be great.… so that these overrides always take effect.
An
@see
to this in point 1 would be great.Why? This should not be necessary.
Comment #40
dawehnerLet me quickly explain that ... as you remember quite hard we don't have any JS enabled for anonymous users by default ... so meh, the test fails because we don't add any JS (beside the HML5 shim). Not sure exactly what to do though. The JS file is just added in case you have logged in first, but for running /update.php with update_free_access the test doesn't work without it.
Comment #41
dawehnerThere we go.
Comment #42
larowlancan be fixed on commit
Comment #43
Wim LeersMuch better.
Comment #44
plachThis looks great, only a couple of nitpicks :)
Missing spaces after the double slashes.
Can we add a break here? :)
Comment #45
plachFixing my own nits. Btw, I manually tested this and it worked great!
Comment #46
lauriiiChanges look good!
Comment #47
alexpottIt's nice to see really good test coverage for this. Thanks everyone. Committed ffeb4a7 and pushed to 8.0.x. Thanks!