DbUpdateController has broken mainteneance mode logic, and update.php doesn't run due to aggregated JS assets

Seems major at least? :)

The actual problem here is that update.php should NEVER use aggregated CSS & JS. It's only by accident that we haven't encountered this problem in the past.

#2510104: Convert drupalSettings from JavaScript to JSON, to allow for CSP in the future just happens to be the first one to trigger this problem.

I think we should make BareHtmlPageRendererInterface-rendered HTML responses never aggregate CSS & JS. The installer and updater don't need better front-end performance (which is what aggregation is for), they need to be reliable.

This is the code retrieving aggregated CSS/JS:

    if (isset($placeholders['styles'])) {
      // Optimize CSS if necessary, but only during normal site operation.
      $optimize_css = !defined('MAINTENANCE_MODE') && $this->config->get('css.preprocess');
      $variables['styles'] = $this->cssCollectionRenderer->render($this->assetResolver->getCssAssets($assets, $optimize_css));
    }

    // Print scripts - if any are present.
    if (isset($placeholders['scripts']) || isset($placeholders['scripts_bottom'])) {
      // Optimize JS if necessary, but only during normal site operation.
      $optimize_js = !defined('MAINTENANCE_MODE') && $this->config->get('js.preprocess');
      list($js_assets_header, $js_assets_footer) = $this->assetResolver->getJsAssets($assets, $optimize_js);
      $variables['scripts'] = $this->jsCollectionRenderer->render($js_assets_header);
      $variables['scripts_bottom'] = $this->jsCollectionRenderer->render($js_assets_footer);
    }

(In \Drupal\Core\Render\HtmlResponseAttachmentsProcessor::processAssetLibraries().)

So, the easy fix is to have BareHtmlPageRenderer do define('MAINTENANCE_MODE', TRUE). But we kind of want to get rid of that global, so…

I'm curious whether care about fast maintenance pages if you are offline

Meaning we should have aggregation on the maintenance page?

I was thinking that too, but… your maintenance page should be super lightweight in the first place (i.e. very little CSS, and probably no JS); so I don't think it really makes sense to want to enable CSS/JS aggregation on the maintenance page.

Actually, install.php does set MAINTENANCE_MODE. Looks like the migration of update.php to a route + controller (DbUpdateController) lost that?

And, in fact, looks like this breaks existing code:

function hook_cache_flush() {
  if (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update') {
    _update_cache_clear();
  }
}

function user_role_permissions(array $roles) {
  if (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update') {
    return _user_role_permissions_update($roles);
  }

  // Install and update pages are treated differently to prevent theming overrides.
  if (defined('MAINTENANCE_MODE') && (MAINTENANCE_MODE == 'install' || MAINTENANCE_MODE == 'update')) {

Those things no longer run because DbUpdateController doesn't set const MAINTENANCE_MODE = 'update';. Only authorize.php still does.
Looks like DbUpdateController should too?

Besides the discussion that defining the constant might do it, I would strongly argue that this is just a workaround. There should be a proper API for that.

Agreed in principle, but attachments, including assets, are only handled AFTER we have a HtmlResponse object. So whatever non-global-state API we provide, would need to make its way into HtmlResponse too; i.e. HtmlResponse needs to carry the configuration.

(Not saying that's bad, just pointing it out.)

So DbUpdateController sets a value in state rather than the constant. That's why #11 points out broken code. Here's a patch with the quick'n'dirty fix.

This could be a solution that's equally hacky as the current mess. But agreed with #10 in principle that we want a proper API. Just not sure that's worth it, because the API simply is… the configuration. The only places where you want to disable aggregation are places where the site's deployed state is being modified.

Actually, perhaps this could be implemented using a config override?

Opened #2538292: Remove atavistic references to MAINTENANCE_MODE == 'update' and supporting code.

Alternatively, here's a config override-based approach. Untested.

#13 looks reasonable if it resolves the bug. What's wrong with that apporach?

Looks like that need to merge with Wims patch?

Are the 2 extra method calls that much of a worry?

We will need some tests - but #19 looks fine on its own.

Can we even just disable JS and not CSS here? Especially with the plan to break CSS into many more small files that may break worse on some browsers, and generally the CSS isn't functional for update.php

I prefer consistency and prudence, but don't have a strong opinion.

Peter, can you link to the issue where we are splitting up CSS in many small files?

I think #26 was referring to #2395853: Split system.module.css and system.theme.css files into SMACSS style components

Yes, that's the one davidhernandez was working on recently.

Testing this locally, I don't see that the JS files are non-aggregated with this patch applied:

--- a/core/modules/system/src/Controller/DbUpdateController.php
+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -155,6 +155,10 @@ public function handle($op, Request $request) {
       $_SESSION['update_ignore_warnings'] = TRUE;
     }
 
+    // Disable JS aggregation so as to never serve old aggregated code that
+    // might break the update batch process.
+    $this->config('system.performance')->setSettingsOverride(['js.preprocess' => FALSE]);
+
     $regions = array();
     $requirements = update_check_requirements();
     $severity = drupal_requirements_severity($requirements)

Am I missing something about how this should work?

Right.

What do you propose, @dawehner?

Wow, the logic in DbUpdateController seems to be 2x broken.

This is a new patch.

+++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
@@ -164,7 +164,7 @@ protected function processAssetLibraries(array $attached, array $placeholders) {
+      $optimize_js = !defined('MAINTENANCE_MODE') && $this->config->get('js.preprocess') && !\Drupal::state()->get('system.maintenance_mode');

If we go this way after all, can we then at least keep the two "is maintenance mode?"-checks together?

All we need now, is test coverage.

Trying to write the test, I found a serious bug in \Drupal\Core\Asset\AssetResolver::getJsAssets() in that is was not including the $optimize flag in the cache ID, so was serving incorrect cached assets.

So, this includes that fix plus a small test change to verify that the JS aggregation is off when maintenance mode is enabled.

@dawehner, the other issue doesn't seem to have a real fix?

Applying patch from https://www.drupal.org/node/2435135#comment-10165198

credit to wuinfo for that

Expanding the test so we check running both running updates starting from out of or in maintenance mode and making sure that's preserved.

Also, 2 test only patches to show the fails during update.php and the fails around aggregation.

This looks great! Pretty much only nits. RTBC once they are fixed. Especially considering #39.

1. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -221,7 +221,7 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
        // Add the theme name to the cache key since themes may implement
        // hook_js_alter(). Additionally add the current language to support
        // translation of JavaScript files.
   

   We should update this code comment too, to remain in sync.

2. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -544,13 +544,13 @@ protected function updateTasksList($active = NULL) {
   +    // Store the prior value in the session so it can be restored at the end
   +    // of the batch.
   

   Nit: 80 cols.

3. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -544,13 +544,13 @@ protected function updateTasksList($active = NULL) {
   +    // During the update, always bring the site offline so that schema changes
   

   Nit: "bring the site offline" sounds a bit strange. Why not just "put in maintenance mode"?

4. +++ b/core/modules/system/src/Tests/System/SiteMaintenanceTest.php
   @@ -42,6 +44,10 @@ protected function setUp() {
   +    // JS Should be aggregated, so drupal.js is not in the page source.
   
   @@ -52,7 +58,10 @@ protected function testSiteMaintenance() {
   +    // JS Should not be aggregated, so drupal.js is in the page source.
   

   s/Should/should/

5. +++ b/core/modules/system/src/Tests/Update/UpdateScriptTest.php
   @@ -173,6 +173,54 @@ function testNoUpdateFunctionality() {
   +  }
   +  /**
   

   Missing \n.

@Wim Leers -

43.1: the CSS code above doesn't mention the $optimize flag, even though it includes it already. I don't think that needs a comment, since it's obvious why it's needed.

  public function getCssAssets(AttachedAssetsInterface $assets, $optimize) {
    $theme_info = $this->themeManager->getActiveTheme();
    // Add the theme name to the cache key since themes may implement
    // hook_css_alter().
    $cid = 'css:' . $theme_info->getName() . ':' . Crypt::hashBase64(serialize($assets)) . (int) $optimize;

Fixed other comment issues.

+1

Committed fbde5c4 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

1. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -221,7 +221,7 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
   -    $cid = 'js:' . $theme_info->getName() . ':' . $this->languageManager->getCurrentLanguage()->getId() . ':' .  Crypt::hashBase64(serialize($assets));
   +    $cid = 'js:' . $theme_info->getName() . ':' . $this->languageManager->getCurrentLanguage()->getId() . ':' .  Crypt::hashBase64(serialize($assets)) . (int) $optimize;
   

   Not a huge fan of just tacking a 0 or 1 on to the end of the key but this is exactly the same as the CSS version so okay.

2. +++ b/core/modules/system/src/Tests/Update/UpdateScriptTest.php
   @@ -173,6 +173,55 @@ function testNoUpdateFunctionality() {
   +  function testMaintenanceModelUpdateFunctionality() {
   

   Fixed Model -> Mode on commit.