Javascript does not work on beta 12 => HEAD update.php

I'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.

Is it #2510104: Convert drupalSettings from JavaScript to JSON, to allow for CSP in the future?

But we still don't render core/drupalSettings before active-link.js

I'd say "then dependencies must be wrong", but this has been the library definition since Feb 2014:

drupal.active-link:
  version: VERSION
  js:
    misc/active-link.js: {}
  dependencies:
    - core/drupal
    - core/drupalSettings
    - core/classList

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

<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

So 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 the update.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 after update.php? i.e. shouldn't we make update.php map the cache_factory service to \Drupal\Core\Cache\NullBackendFactory?

don't use any cache is highly problematic given that this could invoke more subsystems again

… if that's problematic, then any site can run into that if those caches just happen to be empty already.

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

Fair.

We 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

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

And dawehner pointed out that it's in fact possible to test update.php.

@lauriii and I were hacking on this a bit. This patch simply disables caching for the asset resolver during update.php runs.

working on a test

Something like so for the test @catch?

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

However, that test fails, so it may be good as-is.

Fixed the exceptions and combined the patches.

Yeah so the test is dud, as per #23
The doPreUpdateTests needs to be after the ->continue link click.

One minor observation, but other than that I think this is RTBC

+++ b/core/lib/Drupal/Core/Update/UpdaterServiceProvider.php
@@ -0,0 +1,42 @@
+    $definition->replaceArgument(5, $argument);
...
+    $definition->replaceArgument(0, $argument);

Should we use constants here instead of ints? less magic, self documenting etc

Well, unlike other cases where we use ints for the fact that PHP does not have ENUMs here we are clearly using ints as ints.

True - I think this is ready then

1. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
   @@ -29,6 +29,33 @@ class UpdateKernel extends DrupalKernel {
   +    // Always force a container rebuild.
   

   It'd be great if this said why.

2. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
   @@ -29,6 +29,33 @@ class UpdateKernel extends DrupalKernel {
   +    // Don't save this particular container to cache.
   

   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.

3. +++ b/core/lib/Drupal/Core/Update/UpdateServiceProvider.php
   @@ -0,0 +1,42 @@
   +    $definition = $container->getDefinition('asset.resolver');
   +    $argument = new Reference('cache.null');
   +    $definition->replaceArgument(5, $argument);
   +
   +    $definition = $container->getDefinition('library.discovery.collector');
   +    $argument = new Reference('cache.null');
   +    $definition->replaceArgument(0, $argument);
   

   … so that these overrides always take effect.

   An @see to this in point 1 would be great.

4. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -186,6 +186,9 @@ public function handle($op, Request $request) {
   +    // Ensure that at least the drupalSettings js is loaded.
   +    $output['#attached']['library'][] = 'core/drupalSettings';
   

   Why? This should not be necessary.

+++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
@@ -275,11 +275,12 @@ protected function replaceUser1() {
+    //update.php environment before running update.php can override this
+    //method and implement their required tests.

can be fixed on commit

Much better.

This looks great, only a couple of nitpicks :)

1. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -273,4 +274,13 @@ protected function replaceUser1() {
   +    // No-op. Tests wishing to do test the selection page or the general
   +    //update.php environment before running update.php can override this
   +    //method and implement their required tests.
   

   Missing spaces after the double slashes.

2. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestJavaScriptTest.php
   @@ -0,0 +1,57 @@
   +        $found = TRUE;
   

   Can we add a break here? :)

Fixing my own nits. Btw, I manually tested this and it worked great!

Changes look good!

It's nice to see really good test coverage for this. Thanks everyone. Committed ffeb4a7 and pushed to 8.0.x. Thanks!