Problem/Motivation

We currently have a slightly-awkward CSS aggregation strategy.

For any page load, we could potentially see up to 6 aggregate files:

  1. CSS from the System module that is needed for all pages
  2. CSS from the System module that is conditionally needed for this page
  3. CSS from any module that is needed for all pages
  4. CSS from any module that is conditionally needed for this page
  5. CSS from the active theme or base themes that is needed for all pages
  6. CSS from the active theme or base themes that is conditionally needed for this page

And, for some bizarre reason, we are special-casing the System module and putting all of its CSS into a separate aggregate file from other module CSS. That's nonsensical.

Proposed resolution

Yes, we need to load the System CSS before other module's CSS, but that can be accomplished with a 'weight' inside the existing CSS_AGGREGATE_DEFAULT file instead of a separate CSS file.

That would make CSS aggregate files #1 and #2 (in the above list) be combined with files #3 and #4, respectively.

Bonus bug! When removing this CSS_AGGREGATE_SYSTEM constant, we discovered that drupal_add_library() has a bug in causing the library CSS to be added to the wrong CSS aggregate.

Inside drupal_add_library(), it calls drupal_process_attached($elements, JS_LIBRARY, TRUE, $every_page); with the intention that the library JS is added to the JS_LIBRARY aggregate file for JS. Unfortunately, it also adds the library CSS to a JS_LIBRARY aggregate file for CSS.

That is incorrect for 2 reasons: we are passing a JS_* constant as an option to drupal_add_css(), and there's no reason that the library CSS should be added to a different CSS aggregate file from the default CSS_AGGREGATE_DEFAULT. (In fact our goal in D8 is to only have a single aggregate constant in core. See https://drupal.org/node/1887922#aggregate for details.)

To fix this, we should remove the 2 extra parameters from drupal_process_attached because they are only there for drupal_add_library's use. That's a false abstraction. We should move the code that applies the $every_page and $group to the JS/CSS out of drupal_process_attached() and into drupal_add_library().

Remaining tasks

This patch is part of the larger goal (#1921610: [Meta] Architect our CSS), but this patch fixes existing bugs and improves Drupal's CSS experience in an atomic, step-wise way. In other words, the patch is beneficial on its own, independent of the other tasks being completed.

User interface changes

none

API changes

Removes the CSS_AGGREGATE_SYSTEM constant. Because it's dumb.

Removes the $group = JS_DEFAULT and $every_page = NULL parameters from drupal_process_attached().

Library JS continues to be added to the JS_LIBRARY aggregate file of JS, but the library CSS is now properly added to the CSS_AGGREGATE_DEFAULT aggregate file of CSS.

CommentFileSizeAuthor
#20 1924436-13-vs-19-interdiff.txt3.91 KBJohnAlbin
#19 system-aggregate-be-gone-1924436-19.patch7.93 KBJohnAlbin
#13 system-aggregate-be-gone-1924436-13.patch4.17 KBrteijeiro
#8 1924436-8-system-aggregate-be-gone.patch4.63 KBJohnAlbin
#7 1924436-7-system-aggregate-be-gone.patch4.65 KBJohnAlbin
#1 1924436-1-system-aggregate-be-gone.patch4.75 KBJohnAlbin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Title: Remove CSS_AGGREGATE_SYSTEM aggregate » Remove separate CSS_AGGREGATE_SYSTEM aggregate file
Status: Active » Needs review
FileSize
1924436-1-system-aggregate-be-gone.patch4.75 KB

Here's the patch.

Status: Needs review » Needs work

The last submitted patch, 1924436-1-system-aggregate-be-gone.patch, failed testing.

Shyamala’s picture

Shyamala CreditAttribution: Shyamala commented
Issue tags: +mobile, +d8mux, +d8mux-css-cleanup

Adding tags

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Needs work » Needs review

Letting the testbot determine if there really is a dependency on this patch #1924430: Add drupal.base library for base CSS styles

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Issue tags: -mobile, -d8mux, -d8mux-css-cleanup

#1: 1924436-1-system-aggregate-be-gone.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +mobile, +d8mux, +d8mux-css-cleanup

The last submitted patch, 1924436-1-system-aggregate-be-gone.patch, failed testing.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Needs work » Needs review
FileSize
1924436-7-system-aggregate-be-gone.patch4.65 KB

Re-rolled patch.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
FileSize
1924436-8-system-aggregate-be-gone.patch4.63 KB

Hmmm… that's not going to work. Work from another patch has leaked into this patch when I rebased a branch. system.module.css doesn't exist yet. re-rolled patch again.

rteijeiro’s picture

rteijeiro CreditAttribution: rteijeiro commented
Status: Needs review » Reviewed & tested by the community

Patch tested.

It applies well and it seems all occurrences of CSS_AGGREGATE_SYSTEM has been disappeared. Also nothing seems broken :)

Maybe RTBC or should I test something more?

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Reviewed & tested by the community » Needs review

Ruben, that review is pretty good. But you should also check that the number of aggregated CSS files for a page has been reduced by 1 after the patch is introduced. :-)

rteijeiro’s picture

rteijeiro CreditAttribution: rteijeiro commented
Status: Needs review » Needs work

Okay, John. I can see the same number of css files but in different order. These are the css files before applying patch:

<style media="all">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/system/system.base.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/system/system.theme.css?mltwec");
</style>
<style media="all">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/misc/ui/themes/base/jquery.ui.core.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/misc/ui/themes/base/jquery.ui.theme.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/overlay/overlay-parent.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/contextual/contextual.base.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/contextual/contextual.theme.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/edit/css/edit.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/contextual/contextual.toolbar.css?mltwec");
</style>
<style media="screen">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/tour/css/joyride-2.0.3.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/tour/css/tour.css?mltwec");
</style>
<style media="all">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/toolbar/css/toolbar.menu.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/toolbar/css/toolbar.base.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/toolbar/css/toolbar.theme.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/toolbar/css/toolbar.icons.css?mltwec");
</style>
<style media="all">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/field/theme/field.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/user/user.css?mltwec");
</style>
<style media="all">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/views/css/views.base.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/shortcut/shortcut.base.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/shortcut/shortcut.theme.css?mltwec");
</style>
<style media="all">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/themes/bartik/css/layout.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/themes/bartik/css/style.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/themes/bartik/css/colors.css?mltwec");
</style>
<style media="print">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/themes/bartik/css/print.css?mltwec");
</style>

And these are the css files after applying the patch:

<style media="all">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/misc/ui/themes/base/jquery.ui.core.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/misc/ui/themes/base/jquery.ui.theme.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/overlay/overlay-parent.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/contextual/contextual.base.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/contextual/contextual.theme.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/edit/css/edit.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/contextual/contextual.toolbar.css?mltwec");
</style>
<style media="screen">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/tour/css/joyride-2.0.3.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/tour/css/tour.css?mltwec");
</style>
<style media="all">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/toolbar/css/toolbar.menu.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/toolbar/css/toolbar.base.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/toolbar/css/toolbar.theme.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/toolbar/css/toolbar.icons.css?mltwec");
</style>
<style media="all">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/system/system.base.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/field/theme/field.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/user/user.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/system/system.theme.css?mltwec");
</style>
<style media="all">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/views/css/views.base.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/shortcut/shortcut.base.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/modules/shortcut/shortcut.theme.css?mltwec");
</style>
<style media="all">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/themes/bartik/css/layout.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/themes/bartik/css/style.css?mltwec");
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/themes/bartik/css/colors.css?mltwec");
</style>
<style media="print">
@import url("http://drewpull.bitnamiapp.com/drupal-8/core/themes/bartik/css/print.css?mltwec");
</style>
    <script>

Do you need more information about that?

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented

Thanks for the review, Ruben!

Hmm… wonder what I did wrong. Investigating.

rteijeiro’s picture

rteijeiro CreditAttribution: rteijeiro commented
Status: Needs work » Needs review
FileSize
system-aggregate-be-gone-1924436-13.patch4.17 KB

Trying to figure out how to solve the issue but I guess it's out of my hands :(

Al least I re-rolled it with the latest changes in 8.x branch ;)

Status: Needs review » Needs work

The last submitted patch, system-aggregate-be-gone-1924436-13.patch, failed testing.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

So, this was initially done so that CSS files that are going to be unconditionally present in all pages are in a separate aggregate, so they don't have to be downloaded over and over again every time the user hits a page that has a different set of CSS files.

Is there anything changed that makes this not relevant anymore?

rteijeiro’s picture

rteijeiro CreditAttribution: rteijeiro commented
Status: Needs work » Needs review

#13: system-aggregate-be-gone-1924436-13.patch queued for re-testing.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented

So, this was initially done so that CSS files that are going to be unconditionally present in all pages are in a separate aggregate, so they don't have to be downloaded over and over again every time the user hits a page that has a different set of CSS files.

You are almost correct. The reasoning you are giving is exactly the reason why CSS_AGGREGATE_DEFAULT was created. (In D7, this was called CSS_DEFAULT.)

However, this patch removes the System-module-specific aggregate file, which I see no reason for ever having been created. Why do we need to aggregate the system module's CSS separately from other module's CSS?

If we want System module CSS to load before other modules' CSS, we can just give it a lower weight so that it goes into the aggregated file first before other module CSS.

That's what this patch is supposed to do.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Issue summary: View changes

Add more detail in the problem section.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Title: Remove separate CSS_AGGREGATE_SYSTEM aggregate file » Remove separate CSS_AGGREGATE_SYSTEM aggregate file and fix drupal_add_library() to aggregate CSS properly
Category: task » bug
Priority: Normal » Major
Status: Needs review » Needs work

I found the issue!

All of Drupal libraries are having their CSS added into the JS_LIBRARY group. This is incorrect. We shouldn't use JS_* constants to add CSS to the system; a major bug, IMO. The default CSS aggregate is CSS_AGGREGATE_DEFAULT and I see no reason for library CSS to be added into a different aggregate group then the default for modules.

Working on a patch now. Apparently, drupal_add_library() is using 2 parameters in drupal_process_attached() ($group and $every_page) that look like they were specifically added as "helpers" for use by drupal_add_library. That's a false abstraction. I'll explain more when I upload the patch.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Needs work » Needs review
FileSize
system-aggregate-be-gone-1924436-19.patch7.93 KB

Here's the patch. I'm going to update the issue summary to explain why we need to simplify the parameters to drupal_process_attached().

BTW, here's the rationale for combining the bugfix for drupal_process_attached/drupal_add_library into this patch. If this issue is committed using only the patch in #19 without the bugfix, then the follow-up issue would be a critical bug because our CSS aggregation system would be producing incorrect output. Technically, it is already producing incorrect output, but the CSS_AGGREGATE_SYSTEM constant (which has the same value as JS_LIBRARY) is masking the error.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Issue summary: View changes

More details in proposed resolution.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Issue summary: View changes

Update proposed solution and API changes to mention bugfix.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
FileSize
1924436-13-vs-19-interdiff.txt3.91 KB

Oh, here's the interdiff!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

Patch looks great to me.

The module/theme division makes sense (although probably ought to be optional) - having an admin theme or multiple front-end themes means that you don't get duplicate CSS served for modules for pages themed differently.

Stuff in the system group technically wouldn't change when a module is enabled or disabled, which is the only reason I can think this might have been added, but that forces a new aggregate anyway due to the query string so it just doesn't make sense.

Looks worth considering for a Drupal 7 backport as well. Can't remove the constant but can stop stuff being grouped that way I think.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented

The module/theme division makes sense (although probably ought to be optional) - having an admin theme or multiple front-end themes means that you don't get duplicate CSS served for modules for pages themed differently.

Hi, catch! I copied that quote over to #1924522: Remove separate CSS_AGGREGATE_THEME aggregate file since it is highly relevant to that discussion. :-)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Remove separate CSS_AGGREGATE_SYSTEM aggregate file and fix drupal_add_library() to aggregate CSS properly » Change notice: Remove separate CSS_AGGREGATE_SYSTEM aggregate file and fix drupal_add_library() to aggregate CSS properly
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 72e2e95 and pushed to 8.x. Thanks!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Category: bug » task
Shyamala’s picture

Shyamala CreditAttribution: Shyamala commented
Assigned: Unassigned » Shyamala
Status: Active » Needs work

I am going to try the general change notice.

Shyamala’s picture

Shyamala CreditAttribution: Shyamala commented
Status: Needs work » Needs review

Summary

  1. Improve on CSS aggregation strategy, by removing the CSS_AGGREGATE_SYSTEM constant. The CSS from System module is combined with CSS from any module, thus reducing the no of aggregation files from 6 to 4.
  2. Loading System CSS before module's CSS can be accomplished with a 'weight' inside the existing CSS_AGGREGATE_DEFAULT file
  3. Fixed bug in drupal_add_library() that was causing the library CSS to be added to the wrong CSS aggregate
  4. Removes the $group = JS_DEFAULT and $every_page = NULL parameters from drupal_process_attached()
  5. Library JS continues to be added to the JS_LIBRARY aggregate file of JS, but the library CSS is now properly added to the CSS_AGGREGATE_DEFAULT aggregate file of CSS.
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Looks good except we only need to document the API changes - the bug fixes are handled by the commit log.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Critical » Major

#2083415: [META] Write up all outstanding change notices before release

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Title: Change notice: Remove separate CSS_AGGREGATE_SYSTEM aggregate file and fix drupal_add_library() to aggregate CSS properly » Remove separate CSS_AGGREGATE_SYSTEM aggregate file and fix drupal_add_library() to aggregate CSS properly
Assigned: Shyamala » Unassigned
Status: Needs review » Fixed

The new change notice is at: https://drupal.org/node/2090637

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Issue tags: -Needs change record

Removing tag

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
Issue tags: -mobile, -d8mux, -d8mux-css-cleanup

Actually, we need to backport the bugfix if not the removal of the CSS_SYSTEM aggregate file.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Issue summary: View changes

Updated remaining tasks

  • alexpott committed 9ddac9e on 8.3.x 
    Issue #1924436 by JohnAlbin, rteijeiro: Remove separate...

  • alexpott committed 9ddac9e on 8.3.x 
    Issue #1924436 by JohnAlbin, rteijeiro: Remove separate...

  • alexpott committed 9ddac9e on 8.4.x 
    Issue #1924436 by JohnAlbin, rteijeiro: Remove separate...

  • alexpott committed 9ddac9e on 8.4.x 
    Issue #1924436 by JohnAlbin, rteijeiro: Remove separate...