Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Currently both the JS and CSS preprocessors do not work for all use cases. Since drupal_build_js_cache() preforms the JavaScript preprocessing to roll all of the js files into one file, developers try to alleviate this with other methods such as the sf_cache module.
It has been proposed to make the JS and CSS preprocessors pluggable, possibly with the help of assetic. To be more specific Owen Barton suggested 2 interfaces, a bundler and a packager.
Proposed resolution
- We make the JS and CSS preprocessing pluggable with two separate interfaces, the bundler and the packager.
- We also integrate assetic into core as the packager.
Remaining tasks
- We need to reach a consensus on if we want to actually integrate assetic into core
- We also need to create a patch for making the JS and CSS preprocessors pluggable
- If assetic was voted to be integrated with core we need to either
- Create a patch here or
- Create a separate issue for assetic's integration into core
Comment | File | Size | Author |
---|---|---|---|
#160 | pluggable_preprocessing-352951-160.patch | 205.94 KB | Wim Leers |
#160 | interdiff.txt | 3.88 KB | Wim Leers |
#151 | pluggable_preprocessing-352951-151.patch | 205.42 KB | JohnAlbin |
#148 | pluggable_preprocessing-352951-148.patch | 206.73 KB | Wim Leers |
#148 | interdiff-fixes.txt | 3.31 KB | Wim Leers |
Comments
Comment #1
mfer CreditAttribution: mfer commentedHere is my first pass at a pluggable JavaScript preprocessor.
Comment #2
mfer CreditAttribution: mfer commentedUpdated the interface name to JSPreprocessingInterface for consistency and fixed some comments.
Comment #3
drewish CreditAttribution: drewish commentedLooks pretty good to me. Some formatting issues:
need to indent two spaces under the @s. That goes for just about all of the PHPDocs.
I know these bits are just getting moved but that's no reason not to improve them:
I'd say "Ensure there's a js directory with in the files directory that it is writable."
I think you should also make it writable:
Comment #4
mfer CreditAttribution: mfer commentedThanks for the feedback, drewish. Updated.
Comment #6
mfer CreditAttribution: mfer commentedPrevious patch still applies with no issues for me.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedAny chance you can name preprocessing to something else? The theme system has kinda taken over that name.
Comment #8
mfer CreditAttribution: mfer commented@moshe weitzman - I'm open to a name change. Drupal 6 already uses preprocess when refering to JavaScript so it would be a name change from an existing convention. Though, this topic is out of the scope of this patch which is just to take an existing system (with an existing name) and make it pluggable. It's primarily a re-factoring of code.
Comment #9
RobLoachStuck up a note about this issue in the JavaScript Aggregator module: #356596: Javascript Aggregator » D7 port.
Would be nice to have a test case. I'll see what I can do....
Comment #10
RobLoachHad to make the preprocessor able to lazy load classes from different files. This meant moving JSPreprocessingInterface to common.inc, and changing "preprocess_js_system" to:
....When the preprocessor is loaded, it will check "file" and include that file before creating the interface.
As for the tests, I got pretty far, but couldn't get it to work all the time. Help appreciated.
Comment #11
RobLoach....... Forgot the patch.
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commented@rob - thats D6 style code. We no longer have to specify files, with the code registry.
Comment #13
mfer CreditAttribution: mfer commentedWhy are we telling it what file to load? The registry takes care of loading files for classes and interfaces. If it's in the includes folder it just finds it. If it's a module you define what module files to scan. See http://drupal.org/node/224333#registry and http://api.drupal.org/api/group/registry/7.
Comment #14
RobLoachFixed the tests, but am having troubles with the registry. Assistance would be nice.
Comment #15
RobLoachAhhh, had to add common.test to the registry.
Comment #16
mfer CreditAttribution: mfer commentedI updated the test to remember the configured class for the JS Preprocessing and restore it after the test.
2 Questions:
Comment #17
RobLoachI'd love for it to be referenced somehow else, but can't think of anything since it's running on a different process and the class isn't loaded. Any other ideas would be awesome.
I don't think it really matters, but we could move it to a different class if that's prefered.
Comment #18
mfer CreditAttribution: mfer commentedThe attached patch moves including common.test to a javascript_test module in the tests folder. The javascript_test.info file includes the module. The javascript_test.module file is a dummy file. If we change core to not require module files this can go away. The setup function for the javascript tests enables this module.
Comment #19
RobLoachThis is looking pretty much RTBC, but Crell just posted #363787: Plugins: Swappable subsystems for core, so I think we should see how it grows...
Comment #20
Wim LeersGreat work! The same should be applied to CSS preprocessing.
Subscribing.
Comment #21
sunComment #22
andreiashu CreditAttribution: andreiashu commentednice, subscribing
Comment #23
mfer CreditAttribution: mfer commentedI don't think the plugins/handlers system will make it into D7. So, here is an update to the previous series of patches that store the class in a variable.
This is the overall layout of the functionality but the tests could use some work.
Comment #25
Wim LeersRerolled the patch. Didn't test it, just a straight re-roll.
Comment #27
SeanBannister CreditAttribution: SeanBannister commentedSub
Comment #28
sunComment #29
catchsubscribing.
Comment #30
threading_signals CreditAttribution: threading_signals commentedSubscribing
Comment #31
Owen Barton CreditAttribution: Owen Barton commentedAgree with #20 that CSS should be included, so updating title
Comment #32
dvessel CreditAttribution: dvessel commentedsubscribing
Comment #33
mikey_p CreditAttribution: mikey_p commentedSubscribing.
Comment #34
ruplSubscribing.
Comment #35
RobLoachShould be one line of comments rather than two.
So excited to see what this spawns in contrib :-) .
Is it just me, or does DrupalPreprocessJS not exist in this patch?
Some tab characters.
5 days to next Drupal core point release.
Comment #36
skottler CreditAttribution: skottler commentedSubscribe
Comment #37
ericduran CreditAttribution: ericduran commentedsub.
Comment #38
RobLoachNot sure where I found this, but it definitely looks awesome: https://github.com/kriswallsmith/assetic
Comment #39
catch#1332580: Clean up API docs for color module is adding docblock headers to JavaScript. That is good for docs, but without progress on this issue we are looking at adding 2-3kb to every request to Drupal if contrib modules follow suit since we don't touch comments at all with js aggregation at the moment.
Also issues like #865536: drupal_add_js() is missing the 'browsers' option and the very different alters I had to do in http://drupal.org/project/agrcache show that our css and js handling are woefully out of sync. So bumping priority of this. Adding comments to JavaScript files should not be an (admittedly small) performance regression.
Comment #40
ericduran CreditAttribution: ericduran commentedAfter all the work on #865536: drupal_add_js() is missing the 'browsers' option I created a new ticket #1332626: Remove duplicate code between all the css/js function such as the aggregation, grouping, etc about combining the all the JS/CSS grouping, aggreation etc.
Would it makes more sense for that to be done here? I figured we can combined them 1st, and then this would be a lot less work.
Comment #41
mfer CreditAttribution: mfer commented@Rob Loach it doesn't look like Assetic does aggregation of files. Is this the case?
Comment #42
nod_This can finally go somewhere: #1497366: Introduce Plugin System to Core go nuts people! :)
Comment #43
RobLoachIt aggregates the files by default.
For our use, we would probably need to $js->dump() into the aggregated file.
Comment #44
mfer CreditAttribution: mfer commented@Rob Loach I see that. What do you think of this for a path forward.
If this sounds good we can start coding.
Comment #45
nod_I would really really like to see this moving forward, ping me when reviews are needed, don't have much time to work on a patch for now.
The plan looks great to me. I'd just like to see/confirm where we're going. Currently we have the following functions to handle JS (same for CSS) in common.inc:
So I guess what i'm really asking is what could be the API that is going to be implemented and what will it cover?
Comment #46
Owen Barton CreditAttribution: Owen Barton commentedI actually think we need (at least) 2 main interfaces of pluggability:
- A "grouper" (or "bundler" perhaps?) defines the groups and ordering, given input/hints about what has been requested on the current page and potentially other things it knows.
- A "packager" which actually does the filtering and concatenation work.
The main reason for this is that the "grouper" really needs to operate in the HTML page request, since it needs to know all the assets the page needs fairly holistically and needs to be able to affect the page HTML to put in tags in the right order and with the right attributes - the "packager" however, needs to be (perhaps optionally) able to live in a separate page request for lazy generation of assets (imagecache/advagg style) - passed in a list of assets, or a reference to a list of assets, it caches the file and returns it to the browser.
Assetic is really a "packager" plugin and I think keeping that interface separate is an ideal layer of abstraction - much of our own complexity is because we have mixed up parts of the grouping and packaging operations (of course Assetic is pluggable itself on a more fine grained level, which is great too). Also, there is lots of benefit from being able to switch out the grouping subsystem separately from the packaging subsystem - as we know it is hard to have a single grouping algorithm get optimal groupings for all sites.
Comment #47
JohnAlbinI'd love to see an expansion of comment #44 as an issue summary. It's not clear (to me) how the plan will be implemented and how people can help out.
I don't want this issue to slip until D9. :-(
Comment #48
Wim Leers#46 is spot-on.
Comment #49
ZenDoodles CreditAttribution: ZenDoodles commentedTagging for issue summary
Comment #50
catchYep, completely agreed with #46, they're two separate systems and should both be pluggable.
Comment #51
Zgear CreditAttribution: Zgear commentedah forgot to remove the issue summary initiative... done!
Comment #52
klonosI don't speak code, but I think that the assetic part should be its own issue. Just to keep issues clean. After all, it's not a part of this issue (yet), just a separate think that would make it easier on us ;)
...I'd start a new issue for that myself, but we'd better leave this to somebody with more knowhow. If a new issue is created and we reach consensus that assetic should be in core, then we should perhaps postpone this while waiting on that.
Comment #52.0
klonosThe issue required an issue summary to be moved forward
Comment #52.1
klonos...made instances of "assetic" a link to the github project: https://github.com/kriswallsmith/assetic
Comment #53
cosmicdreams CreditAttribution: cosmicdreams commentedIf we're voting +1 for assetic
Comment #54
effulgentsia CreditAttribution: effulgentsia commentedI'm wondering how to reconcile this statement with what's being proposed in #1542344: Use AMD for JS architecture
Comment #55
nod_Maybe that can help?
http://drupal.org/node/1667500
http://drupal.org/node/1667512
http://drupal.org/node/1667514
Comment #56
nod_Plugins are in, who is interested to work on that? This is borderline critical for us.
Ping me on IRC/email/smoke signals/whatever if you're interested and I'll get the stuff needed to get started.
Comment #57
cosmicdreams CreditAttribution: cosmicdreams commentedHey, nod_, If I can learn enough of the plugin system I want to help with this.
Comment #58
nod_All right, so we have cosmicdreams, SebCorbin and possibly Rob Loach interested in getting this going. I'll try to see how we can get that going :)
Comment #59
Owen Barton CreditAttribution: Owen Barton commentedI am interested in working on this also - perhaps we should schedule an IRC chat sometime to hash what the interface looks like in a bit more detail?
Comment #60
cosmicdreams CreditAttribution: cosmicdreams commentedHad a great weekend at MDS and saw how a plugins are made. EclipseGC has converted a number of our standard blocks over to the plugin system. I think that should be either in his sandbox or in a patch somewhere. I try to track it down and post it here.
In short, its crazy awesome/simple.
Comment #61
neclimdulSince we're interested in using the plugin system, tagging for aggregation.
Comment #62
neclimdulAlso, Figuring out the class interface is definitely the first step. Probably would make sense to have 2 interfaces and 2 plugin types, one for CSS and one for JS just for sanity. If that IRC meeting happens just ping me ahead of time so I know to be there :)
Comment #63
Kiphaas7 CreditAttribution: Kiphaas7 commentedVery interested in this. If I can help, I will.
Comment #64
nod_Trying out an implementation with Assetic #1751602: Use Assetic to package JS and CSS files help welcome :)
Comment #65
Owen Barton CreditAttribution: Owen Barton commentedPlease note #46 though - Assetic is only half of what we want to make pluggable, I think (though I think it is a good basis for that half!).
Comment #66
mcjim CreditAttribution: mcjim commentedI've attached a diagram of how this might work. I've discussed it with @sebcorbin who thinks it should work with the diagram here: http://drupal.org/node/1667500
I've also attached a not-to-be-tested patch with some code I was was working on yesterday to move much of the CSS/JS preprocessing code to an overridable class, based on how the cache system works. It's pretty scrappy and I'm going to rewrite it, it's just here mainly to show that progress is being made.
Comment #67
Kiphaas7 CreditAttribution: Kiphaas7 commentedAwesome, keep up the good work!
Comment #68
mcjim CreditAttribution: mcjim commentedAnother work-in-progress patch.
Same functionality as before but now using the core Dependency Injection Container (thanks for the help, @alexpott).
Changed the AssetManager to an abstract class rather than an interface and renamed things generally.
Setting to needs review just to let the test bot find issues. Lots of work still on-going.
Comment #69
mcjim CreditAttribution: mcjim commentedComment #71
mcjim CreditAttribution: mcjim commentedRegistered the bundle in install.core.inc
Hopefully will work…
Comment #73
mcjim CreditAttribution: mcjim commentedLooks like it's the call to
drupal_get_css()
inajax_render()
which is causing the fails.Comment #74
mcjim CreditAttribution: mcjim commentedSmall amends to keep up-to-date with core and to (hopefully) fix the ajax_render() fails.
Comment #76
mcjim CreditAttribution: mcjim commentedRe-roll.
update.php and authorize.php weren't aware they needed to know about the css_asset_manager class, so I told them.
Fingers crossed.
Comment #77
mcjim CreditAttribution: mcjim commentedAfter discussion with sebcorbin, going to move development of this and the Assetic work into a sandbox, which I hope to set up in the next day or two.
I'll post details here. Just ping me if you want access.
Comment #78
kscheirerAssetic was added to Drupal core here: #1784774: Remove Assetic component from core
Comment #79
RobLoachComment #80
mcjim CreditAttribution: mcjim commentedSandbox at http://drupal.org/sandbox/mcjim/1813618
Comment #81
Owen Barton CreditAttribution: Owen Barton commentedTook a pass at adding a JS asset manager. It's basically untested and still a bit wonky (we need to find tune the methods on the abstract class, and I think the AJAX pre-render stuff just needs to go somewhere else), but I tried not to actually change anything functionality wise. WIP regular and branch patch attached.
Comment #82
neclimdulI don't think the JsAssetManager class is in the patch.
Comment #83
attiks CreditAttribution: attiks commentedThis is my first look in a long time to this, I like the idea, some small comments:
the return looks strange?
comment too long and trailing whitespace
Comment #85
mcjim CreditAttribution: mcjim commentedRe-roll of patch in 76 to keep up with HEAD.
Now looking at a re-roll of 81.
Comment #86
cweagansSo what's left to do here? It looks like we still need a way to define pluggable transformations to apply to the assets that the asset manager knows about (minify, compress, aggregate, etc) and the Javascript asset manager is missing from #81. mcjim, do you have a todo list that you're working from here? I'd love to spend some time helping to move this forward.
Comment #87
Owen Barton CreditAttribution: Owen Barton commentedI had a look for the JsAssetManager and it appears to be kaput. Shouldn't be too hard to recreate it though.
In terms of what is remaining in the big picture, I think the answer is "a lot". At BADcamp I spoke with several people (and had a long call with sdboyer on Friday) and there seems to a lot of agreement around refactoring the processing so that the grouping and preprocessing happen at a "build" stage, where they can get a holistic view of what assets are being registered and what kind of context (e.g. paths, blocks, permissions) they may be needed in, rather than during a random page build which provides insufficient information to actually group sensibly and causes no end of issues with concurrent building on busy sites. This switch is pretty much dependent on getting AMD or equivalent in (so we can include JS without enabling it), and probably a few other things (mechanism for blocks to pass their asset needs from blocks in the response, and probably some kind of introspection mechanism for blocks) - I am working on a summary.
In the mean time I think this is still a beneficial change - getting our current grouping logic into a plugin and (as a second step) converting the existing preprocessing to use Assetic (which is a separate issue) is all stuff we need to do anyway.
Comment #88
hass CreditAttribution: hass commentedI'm not sure what this patches are all doing here, but could you guys make sure the packager is re-usable as stand alone function for modules, please. See #1825434: Remove whitespace characters in Code snippet (before) for the reasons.
Comment #89
YesCT CreditAttribution: YesCT commented#85: pluggable-asset-management-352951-85.patch queued for re-testing.
Comment #91
YesCT CreditAttribution: YesCT commentedis this related to or impacting #886488: [D7] Add stampede protection for css and js aggregation ?
Comment #92
Wim LeersI sat down with sdboyer at DrupalCon to discuss a MVP for front-end performance. Drupal 8 will ship with much more JS. It needs to be minified. We need a better way of overriding CSS/JS aggregation & minification in Drupal 8 than we've had in Drupal 7 and earlier: it's nigh impossible to cleanly override it right now.
The MVP is very simple and it's this: "keep all existing logic, massage it until every phase of the aggregation/minification process is in its own pluggable service". So: no sweeping changes like proposed earlier in this issue, e.g. at #85 (no more Assetic here, for that we have #1751602: Use Assetic to package JS and CSS files). We're too close to code freeze (July 1). We want small steps, where each small step may not make front-end performance better at all, but where it finally allows for contrib to provide that in a clean way.
In our brainstorming, sdboyer and I took into account the needs of SCOTCH and the eventual need for dependency resolution between libraries and even potentially a "compilation" phase to allow for SASS/CoffeeScript and all that jazz. We won't build that, but we made sure that we're not preventing it either.
I hope to post a patch within the next 48 hours that includes test coverage. Note: we've never had test coverage for CSS/JS aggregation…
Comment #93
Owen Barton CreditAttribution: Owen Barton commentedAgreed. I would add that we have no way to assess if any improvements to the logic actually make a measurable difference to page load performance an typical Drupal site with typical usage patterns. I have been working on something that hopefully can provide this, but it's also ways off.
Comment #94
Wim LeersStatus update
I didn't make that 48 hour timeframe. I did not sit still though. I've made huge progress. In this first patch, I want to achieve a few things:
Everything is working, and there is PHPUnit test coverage for the "grouping" step already. I now need to work on writing comments and additional test coverage. Hopefully I'll get all that done tonight.
Comment #95
cosmicdreams CreditAttribution: cosmicdreams commentedexcellent news!
Comment #96
Wim LeersThe proposed structure:
AssetCollectionOptimizerInterface
: optimizes the whole collection of assets, returns a (in our case: smaller because aggregated) set of assetsAssetCollectionGrouperInterface
: used/called byAssetCollectionOptimizerInterface
, groups a collection of assets into logical groups (e.g. based on media type for CSS, but could also take into account typical user's navigation path)AssetOptimizerInterface
: used/called byAssetCollectionOptimizerInterface
, optimizes an individual asset, returns optimized data as stringAssetDumperInterface
: used/called byAssetCollectionOptimizerInterface
, dumps an (optimized) asset to persistent storage, returns an URIAssetCollectionRendererInterface
: can render an asset collection into a render arrayAs said in #94: I've implemented this just for CSS to gather feedback first.
The goal of this patch/issue is to introduce clear separation of concerns and change as little as possible. However, in a few places, the lack of separation of concerns forced minor behavior changes:
CssCollectionOptimizer
. The consequence is this:drupal_aggregate_css()
used to do this: "Additionally, this functions aggregates inline content together, regardless of the site-wide aggregation setting." -> this is conflating different things; either aggregate, or don't. This is the only change in output: only perform this aggregation if the CSS aggregation setting is actually enabledCssCollectionGrouper
(which belongs to aggregation), which breaks separation of concerns, instead added very basic grouping support withinCssCollectionRenderer
just for this edge case; remove this when we support IE >=10PHPUnit unit tests for everything (though not every possible edge case yet), except
CssDumper
, which is tiny, identical to the original code and a pain to test.CssCollectionOptimizer
, because it calls into the application state and because it ties together the various aggregation classes. Advice on how to best test this is appreciated before I start mocking a bunch of things.In any case, the test coverage added by this patch is a massive step forward, because all complex parts are now properly tested.
P.S.: yes, that means there is test coverage for the IE 31 CSS files limit craziness!
Comment #97
sdboyer CreditAttribution: sdboyer commentedYAAAAAAAAAAAAAAAAAAY
here's a first-pass review; i'm assuming that the actual logic is more or less copy/paste from the old stuff (though with some reorganizations), so i'm looking more at the overall structure.
as i said in IRC, these'll need to be made into container services. but i know, we wanted buy-in, first :)
though sorta to that end, an architectural semi-nit: my preference would be that instead of using conditional logic that governs whether the optimizer is called at all, i think it might be better if we instead pass CssCollectionOptimizer some no-op groupers, optimizers, and dumpers, then call the $optimizer->optimize() logic unconditionally.
just a clarification for others - the plan Wim and i discussed in Portland is to have these methods take arrays initially, but in a subsequent patch have them take AssetBags instead.
this should be injected, but we can do it in a later patch.
s/inclussion/inclusion/
let's define an exception of our own, here.
not sure the @see on a non-phpdoc comment works.
but i'm also not sure that we can have the @see directive up in the phpdoc if we're using {@inheritdoc}...grrr.
but in this case, we can definitely move the @see up to the bottom of the phpdoc.
why are we using a static in a class context?
again, statics in an class context - just use an object property, i think.
Comment #99
Wim Leers#97:
Exactly.
I strongly disagree.
CssCollectionOptimizer
must by definition assume it has to optimize the passed in asset collection. Hence it also tries to be as lazy as possible, to make the optimization process as fast as possible. This inherently conflicts with "no-op". If you really want no-op groupers, optimizers and dumpers to be passed in, then we will also need a big, fat, ugly "IF no-op THEN return input ELSE return optimize(input)".Unless you can show me how to do this cleanly, I think your suggestion is a big step backwards, both in code clarity and in assumptions: if the "CSS aggregation" flag is disabled, then how does it make sense to apply it anyway, albeit with no-ops? It is counter to intuition.
RE: the rest of your review is all nitpicky details. The
@see
comments exists solely to point out where this chunk of code lived *before* this patch, to aid in reviewing. They are not intended to be in the final patch. The statics are within those functions because they also were before; getting rid of those is out of scope because it is against the "change as little as possible" principle that we're applying here.Rerolling to hopefully pass all tests…
Comment #100
catchI'd understand a no-op as an implementation that either does nothing, or returns exactly the same as it gets, in that case this ought to be simple enough to add those classes then use them based on the setting.
Haven't reviewed the patch yet but great to see this being worked on, and 100% agree with the course of action.
Comment #101
sdboyer CreditAttribution: sdboyer commentedyeah, most of my notes in #97 were nits, because i'm generally very happy with this. no surprise :)
@catch - yep, that's what i'm saying.
i'd have to look at it a little bit more to come up with a concretely good suggestion, but what suggesting is that rather than relying on conditionals in the code we're guaranteed to run to decide whether or not to access services, we instead guarantee that we always interact with the service in the same way, but rely on polymorphism in the service implementation to achieve the desired change in output.
it is a lower priority idea at this time in the cycle - it should NOT block this patch, nor should we even work on it here. it's a followup. but the more conditionals we pull out of mainline, the more control we deliver into pluggable services. and that's a Good Thing.
...actually, here's a concrete observation: right now, the logic for dealing with the IE 31 stylesheet maximum is, as the code comment says, a "light form of grouping." right now, it lives in
CssCollectionRenderer
because that's the only servicey thing that's run unconditionally. if, however, we were to always run the group/optimize/dump stack, then that logic could move into a grouper, where it more logically belongs, and that could be used instead of a true no-op grouping service in the case where the global css optimization flag is turned off. this would also enable sites that don't give two shits about supportingagain, i don't think it should block this patch - the important thing is getting the essential service established. these are things that can change in a follow-up. i do intend to do another review looking at it in context (not just in dreditor), and probably stepping through it as well.
Comment #102
catchIs there a use-case for optimizing files before they're aggregated - thinking of minimizing js which can be expensive, and you might want to do once per source-file instead of once per aggregate. On the other hand something like CSS preprocessing could potentially work on the whole file (removing overwritten rules etc.) so it'd either mean two steps or just not enabling that.
Comment #103
Wim Leers#101:
How does it make sense to always run a non-essential service just for the sake of being able to run the same things always? If it's an optional thing, it should not always execute. So it makes an optional service into a default/required service.
Furthermore, it will move the logic whether to *actually* apply the service into the default/required service. Instead of having a simple, stupid service that does what it's told, it's now an opinionated service… and whenever the decision to do it or not becomes more granular, we'll have to modify the service instead of the calling code.
I'm all for simplification, consistency, predictability and all that. I am. But in this case, I believe it achieves exactly the opposite. Precisely because it's optional. And because it is NOT just "output from step N is used as input for step N+1". This is what we agreed to do at DrupalCon, but that's now how it works right now, because we chose not to look at the details at DrupalCon. Please look at
CssCollectionOptimizer
and you'll see that "just plugging in no-ops" does not work in this particular implementation and I doubt it will work in any.Argh! NO! :P
I went through a lot of effort to do the exact opposite. What you describe is how it used to work. I even wrote specifically about this in #96:
In other words: grouper is for logical grouping, for bundling in some way that makes sense from a performance perspective. The grouping that needs to happen for IE is completely unrelated to performance, it's just to work around moronic browser limitations. Hence it belongs in the renderer service, and nowhere else, and once we don't need to support IE<=9 anymore, we can simply delete half of the renderer service…
At this point, I'm starting to feel like you guys are discussing ideals and not actual code. We've done that, now we need to talk actual code, not ideals :)
Ok, this is what I'll selectively remember then :) ;)
#102: The current architecture allows for that;
AssetCollectionOptimizerInterface
allows you to do exactly that.I've spent a lot of time thinking about what each way of doing things allows and prevents, I've looked at the advagg module to see what one might want to do, and so on. The goal of this issue is to provide the basic blueprint for asset optimization handling, so it allows for any conceivable logic to do just that.
This is excellent feedback though — please do try to think of more things you might want to do during CSS/JS aggregation/optimization/minification that you suspect won't work with the current approach :)
Comment #104
Owen Barton CreditAttribution: Owen Barton commentedFor some JS optimization tools that have eval code to "unpack themselves", optimizing individual files may not be very efficient as the unpacking logic has some overhead and would get included multiple times (there may also be efficiency gains compressing across larger files, as there is with gz/zip etc). For more direct JS optimizer tools, I think optimizing each file could work.
That said, if Drupal is generating lots of bundles frequently, that is a very strong indication that our aggregation strategy is broken (in the combinatorial "users get new large bundle every pageload breaking browser caching" sense we saw in D6) to the extent we might be better off just turning it off. If things are working right, there _should_ be only one set of bundles (split by media etc) per active role combination, and they should only change with code releases or rare configuration changes.
Comment #105
sdboyer CreditAttribution: sdboyer commentedyeah, clearly i didn't absorb that bit in my first pass :)
OK, i'll concede. i really don't have enough of a nitty-gritty feel for this logic yet to have anything like a strong opinion on whether or not it's a good idea, but i can certainly see the point that the global config setting aligns 1:1 with the domain of responsibility for the optimizer. so calling the service conditionally on the global setting's value is definitely not unreasonable.
good, that's what was important :)
this should go in ASAP once we get it green, so that we can move on to the next step.
Comment #106
pounardI couldn't agree more! Drupal 7 grouping and aggregation strategy is rotten to the grounds and in some case reveals itself to be very counter-productive and counter-performant, but I think the essentials about this patch is to turn all that logic into replaceable components, and it's probably for the better! I didn't review any of the patches thought, but anything that moves from hardcoded to replaceable is a huge win, even if the algorithmics grounds remain the same, they become replaceable by contrib at the very least!
Comment #107
Wim LeersWow, I had no idea it'd been more than a week already since I touched this. Time flies when you're coding against the clock…
#104 & #106: Yes, all this needs to become better and smarter. Let's get this issue done, so we can do that if necessary post-code freeze (July 1, 2013) because this issue will allow us to do that without changing APIs. If core doesn't do it, then contrib will be able to.
#104: note that this architecture is not really about (does not force) choosing to optimize individual assets over optimizing the aggregated end result; it's that an
AssetOptimizerInterface
implementation takes an asset and optimizes it — that could just as well be the aggregated end result! The point is that we have something that just takes a certain asset type, and optimizes it, and then an asset collection optimizer that decides how to do that: whether that's optimizing individual assets and then concatenating them, or first concatenating and then optimizing the whole: it doesn't matter, it's up to the implementer :)This is only about providing simple building blocks to have pluggability in multiple ways. One module might integrate with a webservice that analyzes common user navigation paths to do smarter grouping: this module would override the grouper. Another module might provide integration with UglifyJS (which cannot ship with Drupal core because it depends on node.js), and provide an UglifyJS JS asset optimizer. And yet another module might provide a smarter overall asset collection optimizer.
All tests now pass.
Apparently we did have test coverage for CSS aggregation in
CascadingStylesheetsUnitTest
! That is now removed and merged intoCssOptimizerUnitTest
!I had to make a few methods in
CssOptimizer
public becausecolor.module
was calling them directly. Yeah… I know … :/ But again, this issue is *not* about changing logic, it's about refactoring, so that's the only sensible thing to do.To run the unit tests:
cd core
, thenphp vendor/bin/phpunit --group Asset
.So, how to move forward here? It seems that overall, people like the proposed architecture (fancy word for "set of interfaces"). Shall I move on to applying the same to JS aggregation?
Comment #109
larowlanConflicted with #2005520: Remove remaining prefixed border-radius and box-shadow from core
Comment #110
sdboyer CreditAttribution: sdboyer commentedtagging
Comment #111
Wim LeersSo #109 has one unrelated fail and seems to have undefined index notices in the PHPUnit tests that I'm not seeing locally, probably because testbot is somehow running them more strictly. Infuriating that local results are different from the ones for testbot :/
Anyway, it's easy to make this green.
I still didn't get an answer to the key question in #107:
So, how to move forward here? It seems that overall, people like the proposed architecture (fancy word for "set of interfaces"). Shall I move on to applying the same to JS aggregation?
Comment #112
catchI'd be happy to get this in just for JS first (with the caveat I've not properly reviewed it yet), then open a follow-up to apply the same for CSS.
Comment #113
Wim Leerscatch: the above is for CSS only, not for JS only :) That's very nice to hear! The only reason I'd say that's not a great idea, is because without also implementing the JS half, we haven't fully validated the architecture yet.
Nobody gives me a firm "yes", but everybody is enthusiastic. So I'll get this done in the next few days, I definitely want to have it done by the end of the weekend.
Comment #114
catchIn agrcache the mechanism for swapping out CSS and JS aggregates is a completely different set of alters, but after implementing all those I was able to refactor the bits I was altering anyway to use much the same logic. That reminds me I should try to review this code for that particular use cases (not building the assets inline at all) to see if there's anything in particular stopping that.
There are some differences but most of the ones in core are accidental rather than necessary. Also if we need to tweak something after code freeze that should be fine in this case - there's no current API for this (except multiple levels of alters and re-implementing several functions), and only 3-4 modules try to swap things out at the moment using those (one of which I maintain, another of which doesn't have a stable 7.x release yet).
Comment #115
sdboyer CreditAttribution: sdboyer commented@Wim Leers - i'm comfortable giving a firm "yes" on continuing to JS. of course, i helped plan this architecture with you, so maybe you're looking for buy-in from someone else. but you have it from me. i wouldn't mind doing JS in a follow-up issue, if only because it'd mean that i could start on the next step more immediately for CSS, and time is...short.
@catch - when Wim and i came up with this approach initially, we designed it with the intention that there were a common set of interfaces that could be shared for each of the steps across CSS and JS, even if they don't share the exact same classes implementing those interfaces for each step. i think we've achieved that here. so imo, that means that it doesn't matter too much if the implementations are the same are not. at least, not right now - we just need to get the interfaces right. we can (and WILL) refactor and improve them later.
Comment #116
larowlanChases head.
Neither of those fails occur locally.
Comment #118
larowlanSo some strict warnings when browsers isn't set.
Hopefully green
Comment #120
larowlanThis time?
Comment #122
Wim Leers#118 & #120 fix the symptom, not the cause. The thing is,
drupal_add_css()
guarantees that the "browsers" key defaults toarray()
. It's the unit tests that were incorrect. Hence the changes in #118 & #120 only serve to make the tests pass.(The really silly thing here is of course the idiocy that PHPUnit tests do *not* fail on notices, then I'd have noticed this a long time ago. Pinged msonnabaum to fix that: https://twitter.com/wimleers/status/348809603984793601.)
Comment #124
Wim Leers#122: pluggable_preprocessing-352951-122.patch queued for re-testing.
Comment #126
Wim LeersTestbot is utterly borked:
As you can see, it did not even clean up after the previously borked attempt (which just aborted) to run the tests. I guess it's the aborting that caused this to happen. Sigh.
Comment #127
Wim LeersWhile rolling #122, I created #2026255: Make CascadingStylesheetsTest 3500% faster, to make
CascadingStylesheetsTest
a *lot* faster. (While working on this patch, I had to wait for that test for way too long way too often.)Now: the JS half of this patch.
Comment #128
Wim Leers#122: pluggable_preprocessing-352951-122.patch queued for re-testing.
Comment #130
Wim LeersNow the result is accurate, it's the same as #120. I'm pretty sure I know the fix.
In the mean time, I've been working on the JS half of this patch. While doing so, I also worked on making
JavaScriptTest
a *lot* faster: #2026349: Make JavaScriptTest 3200% faster.Comment #131
Wim LeersAnd voila, part two: pluggable JS aggregation!
Changes:
JsCollectionGrouper
,JsCollectionOptimizer
andJsCollectionRenderer
.CssDumper
toAssetDumper
(more generic), because 99% of the code was identical for the two.JavaScriptTest
actually has very decent test coverage. Also hardly modifiedJavaScriptTest
: only removed 3 places where it was callingdrupal_build_js_cache()
(which of course is removed).This was fairly easy, because 1) there is no advanced exotic edge case for JS handling like the 31-stylesheets-in-IE CSS handling, 2) better existing test coverage for JS, 3) the hard architecture work had already been done :)
This should also fix the last CSS unit test failure.
Comment #132
larowlanFirst up, this patch is awesome, great work.
Some nitpicks and some other general observations.
needs leading \
needs empty line before
We should be injecting the ConfigFactory
More than 80 chars
Needs blank line before
we shouldn't use top-level objects, instead \Drupal
remove ' object.'
Can we inject the state service?
two spaces?
Missing docblock
Can we get a todo here to move this to state once conversion occurs, don't want to add new variable_get call now but realise can't be avoided. We should also put these in methods on the object so that method can be mocked in unit testing.
Can we inject \Drupal\Component\Utility\String and use checkPlain, will help unit testing too
don't think this matches coding standards, should be in the docblock?
Is this the correct way to reference class methods? I'd have thought we'd need to reference the full object eg \Drupal\Core\Asset\CssOptimizer::loadFile()
Missing @params/@return from doc block
Out of scope: we really need an OO version of these
$this->container->get('config.factory')->get('system.performance')?
Missing @param/@return
Can we add a new method to CssRenderer called createUrl which includes the call to file_create_url and then mock that method in this test instead of this workaround. Same for variable_get. For check_plain we can inject the String utility (see above). Same for CSSOptimizer and file_uri_scheme.
Comment #133
Wim LeersYay, patch is green! Which means the nitpicking can now begin, as @larowlan already started doing :)
Those things should happen indeed, but IMHO are unrelated to the task at hand, which is making the whole pluggable. This is specifically about refactoring the way asset aggregation is handled, and specifically not about refactoring the logic. This is getting close to refactoring the logic.
… that's extremely ugly IMHO, and it'll also be extremely repetitive: pretty much every class in Drupal would need to do this kind of thing. Why is this necessary here, and not elsewhere? Should we really be worried about that too here? I want to keep this as focused as possible, and not worry about such details (which it is in the grand scheme of things).
I totally agree that what I currently have is extremely hacky and ugly, but once
file_create_url()
and others have been converted into OO code, this problem will go away automatically.These @see comments exists solely to point out where this chunk of code lived *before* this patch, to aid in reviewing. They can now be removed, and I have :)
Sadly, you're right. Bad DX once again. I think I've been doing this wrong for many months (and getting patches committed with this) without anybody every noticing though.
… but only the last of the three methods you mention is one of my making, the other two didn't have docs before. Chances are they will go away anyway. I don't want to waste time trying to clearly document what hacky existing code is doing.
So, only doc'd the last one.
Comment #134
dawehnerI guess it should be possible to convert this to a data provider, see http://phpunit.de/manual/3.7/en/appendixes.annotations.html#appendixes.a...
Comment #135
Wim Leers#2026255: Make CascadingStylesheetsTest 3500% faster got committed, so this will need a reroll.
#134: Thanks! I'm using data providers elsewhere, will apply that there too.
Comment #136
sdboyer CreditAttribution: sdboyer commentedi'm not quite gonna reassign this just yet, but...making a note that when we're basically happy with the core of what Wim's written, i'll convert the various components of the system over to container services, and introduce dependency injection as needed.
i've gotta review the JS side again first, though. sorry, couldn't get to it last night. WHY IS THERE NEVER ENOUGH TIME!?! once i've reviewed and am content, i'll reassign and go to town.
Comment #137
sdboyer CreditAttribution: sdboyer commentedok, reviewed this. i have a couple of nits, but rather than posting them here, i'm going to just fix them, because this is now on my plate to take care of the service-ifying and container-ifying of these various objects. i do have an inkling that, in addition to the services @larowlan mentioned for injection, i may refactor the renderer to take the optimizer as an injected service, as well.
so, reassigning, and i'm off to the races.
Comment #138
sdboyer CreditAttribution: sdboyer commentedfirst, just a quick reroll to catch up with #2026255: Make CascadingStylesheetsTest 3500% faster. no interdiff b/c the old patch no longer applied.
Comment #139
sdboyer CreditAttribution: sdboyer commentedfyi, the huge jump in patch size is because i forgot to reroll with -M; it's just the rename of all the stylesheet files.
Comment #140
sdboyer CreditAttribution: sdboyer commentedok, super-simple conversion of these systems from hardcoded object creation over to easily swappable services. they're all interface-ified already, so that'd be easy for contrib to do.
actually working on this patch made me want to tweak a few things about it, but they're relatively trivial details, so it's fine for followups. the only question i have - why do we optimize (minify) css, but not js?
i'd like to say that these will be all green, but i was seeing an absolutely bizarre test failure on
CascadingStylesheetsTest::testRenderInlinePreprocess()
- for some reason, a leading newline isn't making it into the output in the patch. that is:what's especially bizarre is that i've checked, and the render array returned both before and after this patch is EXACTLY THE GOD DAMN SAME - the newline is present in the suffix property in both cases. so i have NFI why this is happening. but whatever has changed with this interdiff, the change in behavior seems to be manifesting somewhere within
drupal_render()
. ugh. i really, really do not like returning render arrays from the renderer...but that, too, is most definitely a followup.Comment #142
Wim LeersBecause we never have done that, for fear of breaking crappy JS… :) Have fun reading the 172 comments in #119441: Compress JS aggregation.
Wow, wtf! I'll try to debug this sucker.
Review of #140's interdiff
The interdiff is much simpler than I expected :)
This means we'll have two instances in the container?
Comment #143
Wim Leers#140: pluggable_preprocessing-352951-140.patch queued for re-testing.
Comment #145
sdboyer CreditAttribution: sdboyer commented@Wim Leers - yep, i started very simple. it's a new habit for me. :P
yes, that means we'll have two instances of
AssetDumper
in the container. not a huge problem, but worth having the two separate addresses because that'd make it easier for contrib to just swap out the services at those addresses and have the collection optimizers pick them up.is it worth - perhaps in a followup - simply having a no-op
JsOptimizer
, notwithstanding #119441: Compress JS aggregation?Comment #146
sdboyer CreditAttribution: sdboyer commented#140: pluggable_preprocessing-352951-140.patch queued for re-testing.
Comment #148
Wim Leers#145:
RE:
AssetDumper
: D'oh, of course, that makes total sense.I don't see the point of having a no-op
JsOptimizer
. Except if … you mean that by having a no-opJsOptimizer
in core, we open the door very easily for contrib to swap it for another, one that actually does optimize JS. Which totally makes sense. Done.See
interdiff-noop_js_optimizer.txt
.I also searched and found the cause for the patch causing the installation to fail. The installer also relies on
common.inc
to render its CSS and consequently as of #140 it depends on the container. But the full container is not built for the installer, only truly necessary parts are registered. So I registered the CSS/JS renderers, and poof, problem solved :)I also found two other bugs that were introduced in #140 and fixed those (
$this->state;
being injected, but still being called like$this->state()
, which doesn't work; and … the optimized CSS/JS assets no longer being used at all :P).See
interdiff-fixes.txt
.With all that out of the way, this patch should now be be RTBC'able… :)
Comment #149
sdboyer CreditAttribution: sdboyer commentedhah. it's weird that i only saw the one test failure locally given the severity of those two oversights. oh well, green is beautiful!
Comment #150
JohnAlbinlol.
Comment #151
JohnAlbinI've edited the patch by hand to remove that one hunk. That way I can continue my review while the testbot works.
Comment #152
sdboyer CreditAttribution: sdboyer commentedoh man, that was me. wow, i really did a crappy first pass on this, sorry guys.
really just highlights to me how GREAT it'd be for us to get together a pull request-ish workflow...
Comment #153
Gábor HojtsyThis is of paramount importance for massive performance improvement on the frontend using a pluggable system. Although performance changes are the focus for this part of the release, this also makes minor API changes that are needed for that.
Comment #154
JohnAlbinSo I've kicked the tires on adding/removing/overriding/rearranging CSS and JS and it all seems to work the same way.
I've also verified that the fix for "IE9 limited to the first 31 stylesheet" is implemented, but in a slightly different way. I actually prefer the new way over the old way.
The difference only occurs when there are less then 31 stylesheets. This is how D8 loaded the stylesheets on the homepage with Bartik before the patch:
And here it is after the patch:
The latter (new) way makes more sense to me. And will to new people to Drupal. We should only start jumping through hoops after we reach 32 stylesheets, and not before.
I haven't tried to write my own implementation of CSS aggregation to test the pluggability of the new system, but… that's probably a bit ambitious for a patch review. :-) But I have looked at the interfaces for CssCollectionGrouper.php, CssCollectionOptimizer.php, core/lib/Drupal/Core/Asset/CssCollectionRenderer.php and I like the separation.
Nice work!
Comment #155
JohnAlbinCross-posted with Gabor.
Comment #156
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #157
catchOverall this looks great, the current implementation hasn't leaked into the API at all from what I can see.
Several nitpicks:
Should this describe the structure of a the $assets a bit more?
I didn't realise this was still a variable, not for here but do you know where the issue to convert this is?
Can't these be class properties? I'd expect we only have one class per-request. If for some reason it has to be static, it could be a static class variable with a @todo.
No documentation for the $matches parameter.
Looks like these two CSS files are removed but not put back?
Comment #158
Wim LeersI could, but the reason I didn't document this more explicitly is because the expectation is that we'll move towards using more well-defined objects when moving towards using Assetic etc. i.e. when #1762204: Introduce Assetic compatibility layer for core's internal handling of assets lands.
This is actually where the current implementation *does* leak through by necessity: the new APIs must be able to handle the in-memory representation of what a CSS/JS asset looks like, and that's exactly what's being passed here. So, it's an array of CSS/JS assets, just as we've been using it in Drupal 7 and before. I don't think it was well-documented before either, but if you think that should happen here, I'll do that.
I don't know.
They can be. But I just didn't want to change any logic where I didn't have to — this retains the exact same logic as before.
This is a (protected) helper function. There were no docs before either. I can add them if you want.
Correct. The tests for "unoptimized preprocessing" have been removed. Because that's a code path that is never used anyway; the only thing that matters is that the optimized output is correct. (Either CSS is completely unprocessed and hence the original files are served to the end user, or the CSS is preprocessed and the files are optimized. There's nothing in between.)
All of the feedback in #157 has been answered and I don't think any changes are necessary. Hence marking NR again to get feedback from catch. If the above is satisfactory, he can mark it RTBC again, otherwise he can mark it NW again so I can address his nitpicks.
Comment #159
catchI think the static should move to a class property here - the code is moved into a class, so a 'raw' static isn't necessary. It's not a logic change as such.
Also adding the documentation to the class method.
The rest fine with leaving as is for now.
Comment #160
Wim Leers#159: done.
Comment #161
catchThanks!
This breaks a couple of APIs, but they're ones we know that only a handful (literally) of modules make sure of - and those modules are the ones that will benefit from the API change since they're jumping through hoops at the moment.
Committed/pushed to 8.x. Will need a change notice.
Comment #162
Wim LeersEt voilà: https://drupal.org/node/2034675
Comment #163
Wim LeersI went through the Drupal core issue queue, looking for CSS/JS aggregation issues that can be closed thanks to this issue, as well as reactivating those that need rerolls. Below I list only those that have a fairly strong relation to this issue, I commented/closed many more. And forgot to list a bunch here probably.
We can now continue #1048316: [meta] CSS and Javascript aggregation improvement.
This patch fixed these issues indirectly:
#643534: separate module for CSS aggregation
#961518: drupal_add_css() preprocesses inline CSS even when the site-wide aggregation setting is off
#1033392: Script loader support in core (LABjs etc.)
Rerolled:
#2014851: Drupal CSS preprocessing breaks protocol-relative paths
Pointed out need for reroll:
#1761498: CSS aggregation fails when there's an unquoted url() with "/*" in it
#1488910: Incorporate media queries in aggregated CSS files, don't create a new group for each new media query
#678292: Add a development mode to allow changes to be picked up even when CSS/JS aggregation is enabled
#1014086: Stampedes and cold cache performance issues with css/js aggregation
Moved to D9:
#494498: Add CSS parsing capability to Drupal
Comment #164
hass CreditAttribution: hass commented#1686178: CSS compressor destroys valid css "content" attribute values
Comment #165
ParisLiakos CreditAttribution: ParisLiakos commentedso we got all those use statements in common.inc, but i cant find any usage there. should i open a followup to remove them, or they serve something?
Comment #166
Wim Leers#165: very good nitpick catch ;) That's a leftover from before #140, from before we were injecting these as services. Please open a new issue. Thanks! :)
Comment #167
ParisLiakos CreditAttribution: ParisLiakos commentedah, i never checked #140:)
opened #2035513: Remove unused "use" statements from common.inc
Comment #168.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #170
grendzy CreditAttribution: grendzy commentedCross-linking: This commit caused a regression, by removing the fix from #1961340: CSS aggregation breaks file URL rewriting because it abuses it.