Problem/Motivation
We currently have a slightly-awkward CSS aggregation strategy.
For any page load, we could potentially see up to 6 aggregate files:
- CSS from the System module that is needed for all pages
- CSS from the System module that is conditionally needed for this page
- CSS from any module that is needed for all pages
- CSS from any module that is conditionally needed for this page
- CSS from the active theme or base themes that is needed for all pages
- 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.
Comment | File | Size | Author |
---|---|---|---|
#20 | 1924436-13-vs-19-interdiff.txt | 3.91 KB | JohnAlbin |
#19 | system-aggregate-be-gone-1924436-19.patch | 7.93 KB | JohnAlbin |
#13 | system-aggregate-be-gone-1924436-13.patch | 4.17 KB | rteijeiro |
#8 | 1924436-8-system-aggregate-be-gone.patch | 4.63 KB | JohnAlbin |
#7 | 1924436-7-system-aggregate-be-gone.patch | 4.65 KB | JohnAlbin |
Comments
Comment #1
JohnAlbinHere's the patch.
Comment #3
Shyamala CreditAttribution: Shyamala commentedAdding tags
Comment #4
JohnAlbinLetting the testbot determine if there really is a dependency on this patch #1924430: Add drupal.base library for base CSS styles
Comment #5
JohnAlbin#1: 1924436-1-system-aggregate-be-gone.patch queued for re-testing.
Comment #7
JohnAlbinRe-rolled patch.
Comment #8
JohnAlbinHmmm… 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.
Comment #9
rteijeiro CreditAttribution: rteijeiro commentedPatch 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?
Comment #10
JohnAlbinRuben, 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. :-)
Comment #11
rteijeiro CreditAttribution: rteijeiro commentedOkay, John. I can see the same number of css files but in different order. These are the css files before applying patch:
And these are the css files after applying the patch:
Do you need more information about that?
Comment #12
JohnAlbinThanks for the review, Ruben!
Hmm… wonder what I did wrong. Investigating.
Comment #13
rteijeiro CreditAttribution: rteijeiro commentedTrying 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 ;)
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedSo, 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?
Comment #16
rteijeiro CreditAttribution: rteijeiro commented#13: system-aggregate-be-gone-1924436-13.patch queued for re-testing.
Comment #17
JohnAlbinYou 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.
Comment #17.0
JohnAlbinAdd more detail in the problem section.
Comment #18
JohnAlbinI 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.
Comment #19
JohnAlbinHere'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.
Comment #19.0
JohnAlbinMore details in proposed resolution.
Comment #19.1
JohnAlbinUpdate proposed solution and API changes to mention bugfix.
Comment #20
JohnAlbinOh, here's the interdiff!
Comment #22
catchPatch 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.
Comment #23
JohnAlbinHi, catch! I copied that quote over to #1924522: Remove separate CSS_AGGREGATE_THEME aggregate file since it is highly relevant to that discussion. :-)
Comment #24
alexpottCommitted 72e2e95 and pushed to 8.x. Thanks!
Comment #25
tim.plunkettComment #26
Shyamala CreditAttribution: Shyamala commentedI am going to try the general change notice.
Comment #27
Shyamala CreditAttribution: Shyamala commentedSummary
Comment #28
catchLooks good except we only need to document the API changes - the bug fixes are handled by the commit log.
Comment #29
catch#2083415: [META] Write up all outstanding change notices before release
Comment #30
JohnAlbinThe new change notice is at: https://drupal.org/node/2090637
Comment #31
JohnAlbinRemoving tag
Comment #32
JohnAlbinActually, we need to backport the bugfix if not the removal of the CSS_SYSTEM aggregate file.
Comment #32.0
JohnAlbinUpdated remaining tasks