When we are in ANY maintenance mode, we do not want to allow aggregation.

When the database is down and the site is in error maintenance mode, the error pages would be attempting to do aggregation without having any way to update the state, causing additional load. The fallback maintenance page does not have any business attempting to aggregate anyway.

This check for != update mode had originally been intended in #199946: Stale css+js files on upgrade to prevent both miscaching and exposing the drupal version, but it should have been taken out when the logic was changed to use an incrementing number.

You can see the original code that led to this in #199946-22: Stale css+js files on upgrade.

CommentFileSizeAuthor
#9 3015223-9-disable-aggregation-for-all-maint.patch1.13 KBizmeez
#2 3015223-disable-aggregation-for-all-maint.patch2.01 KBbdragon

Comments

bdragon created an issue. See original summary.

bdragon’s picture

bdragon commented
StatusFileSize
new 3015223-disable-aggregation-for-all-maint.patch2.01 KB
bdragon’s picture

bdragon commented
Status: Active » Needs review
fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Drupal 7.70 target

RTBC - Looks great to me!

fabianx’s picture

fabianx commented
Issue tags: +Needs change record
bdragon’s picture

bdragon commented

change record drafted @ https://www.drupal.org/node/3018664

heddn’s picture

heddn
Primary language English
Location Illinois
commented

Reviewed the CR. It looks good.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Issue tags: -Drupal 7.70 target +Drupal 7.74 target

This is a smart change, thanks.

izmeez’s picture

izmeez commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3015223-9-disable-aggregation-for-all-maint.patch1.13 KB

Patch in #2 no longer applies. Attached is a quick re-roll.

mcdruid’s picture

mcdruid commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Drupal 7.74 target, -Needs change record +Pending Drupal 7 commit

LGTM

fabianx’s picture

fabianx commented

RTBC + 1 again :) - this happened on a real site :)

  • mcdruid committed 021554e on 7.x 
    Issue #3015223 by bdragon, izmeez, heddn: Never use aggregation in...
mcdruid’s picture

mcdruid commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Thanks everybody!

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone commented

Published change record