^10.1 CSS aggregation breaks during maintenance mode

The bug is old, but it's showing up in 10.1 due to the new aggregate routes/controllers. Short version: we're trying to do this already, but not checking the right things consistently.

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?

Yes we've already got test coverage that's similar 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.

The last submitted patch, 9: 3373328-test-only.patch, failed testing. View results →

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?

Glad to be the proverbial canary in the coal mine for this. Love seeing all the work going into this.

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.

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 📌 Update MaintenanceMode service and deprecate legacy API Needs work I wonder if it should just be that issue (or child issues of that issue for conversions).

Awesome work. Thanks everyone.

Automatically closed - issue fixed for 2 weeks with no activity.

A contributor raised a bug about this: 🐛 Aggregate JavaScript files is getting optimized only in maintenance mode Postponed: needs info .