^10.1 CSS aggregation breaks during maintenance mode

Patch is somewhat self-explanatory, there already exists code that tries to do this, at least since 2015, but it's not checking the right thing consistently.

So an old bug but exposed by the new routes/controllers.

Even the simplest patches need linting.

Before the #5 patch.

After the #5 patch.

Confirmed that the #5 patch working well.

Is it possible to add test coverage for this? If it is by design that CSS/JS is not aggregated in maintenance mode we should probably have an assertion for this.

Yes we've already got test coverage that's similarly incomplete to the code so can extend that.

+++ b/core/modules/system/tests/src/Functional/System/SiteMaintenanceTest.php
@@ -89,6 +93,7 @@ public function testSiteMaintenance() {
     // JS should not be aggregated, so drupal.js is expected in the page source.

This comment should probably mention both CSS and JS.

Reworded a bit.

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
@@ -133,10 +133,11 @@ public function processAttachments(AttachmentsInterface $response) {
+    $maintenance_mode = defined('MAINTENANCE_MODE') || \Drupal::state()->get('system.maintenance_mode');

+++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
@@ -311,17 +311,19 @@ protected function renderPlaceholders(HtmlResponse $response) {
+    $maintenance_mode = defined('MAINTENANCE_MODE') || \Drupal::state()->get('system.maintenance_mode');

Have we stopped relying on the MAINTENANCE_MODE constant for the maintenance mode? I couldn't find where we're setting that for the actual maintenance mode.

I'm also wondering if you had a specific reason for not using dependency injection for the state?

Have we stopped relying on the MAINTENANCE_MODE constant for the maintenance mode?

It's still set in install.php. It used to be used in update.php, but that's been a route for several years now, we should open a new issue to factor it away entirely if there isn't already one - could add a special installer state service for pre-database that always returns it as true or something like that.

I'm also wondering if you had a specific reason for not using dependency injection for the state?

No I was just copying and pasting from HtmlAttachementsResponseProcessor which already doesn't use dependency injection for state for the js case. Can look at injecting it, as long as the patch is still patch release-safe we might as well do it here.

For me, injecting state can be done in a followup given that using \Drupal was an existing pattern here, and I agree that the MAINTENANCE_MODE vs state variable is confusing and that we should open a followup to try and unify that if we can.

edit: #3058881: Update MaintenanceMode service and deprecate legacy API and #2538292: Remove atavistic references to MAINTENANCE_MODE == 'update' and supporting code and a number of similar issues already exist

Committed b63275f and pushed to 11.x. Thanks! Also cherry-picked to 10.1.x.

Was going to open a follow-up for dependency injection, but seeing #3058881: Update MaintenanceMode service and deprecate legacy API I wonder if it should just be that issue (or child issues of that issue for conversions).

A contributor raised a bug about this: #3452672: JavaScript files added by AJAX responses are only optimized in maintenance mode.