text:
Join as a member. There's a certificate with your name on it.
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
|Wim Leers
PASSED: [[SimpleTest]]: [MySQL] 57,160 pass(es). View
|Wim Leers
PASSED: [[SimpleTest]]: [MySQL] 56,687 pass(es). View
|Wim Leers
PASSED: [[SimpleTest]]: [MySQL] 56,273 pass(es). View
|Wim Leers
Comment #3drewish CreditAttribution: drewish commented
Looks pretty good to me. Some formatting issues:
+ *
+ * @param $files
+ * An array of JavaScript files.
+ *
+ * @return
+ * html for preprocessed JavaScript files.
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:
+ // Create the js/ within the files folder.
I'd say "Ensure there's a js directory with in the files directory that it is writable."
+ file_check_directory($jspath, FILE_CREATE_DIRECTORY);
I think you should also make it writable:
+ file_check_directory($jspath, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
Comment #7moshe weitzman CreditAttribution: moshe weitzman commented
Any chance you can name preprocessing to something else? The theme system has kinda taken over that name.
Comment #9RobLoach CreditAttribution: RobLoach commented
Stuck 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 #10RobLoach CreditAttribution: RobLoach commented
Had 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:
'name' => 'DrupalPreprocessJS',
'file' => 'includes/preprocess.inc'
....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 #11RobLoach CreditAttribution: RobLoach commented
....... Forgot the patch.
Comment #12moshe weitzman CreditAttribution: moshe weitzman commented
@rob - thats D6 style code. We no longer have to specify files, with the code registry.
Comment #14RobLoach CreditAttribution: RobLoach commented
Fixed the tests, but am having troubles with the registry. Assistance would be nice.
Comment #15RobLoach CreditAttribution: RobLoach commented
Ahhh, had to add common.test to the registry.
Comment #17RobLoach CreditAttribution: RobLoach commented
Is it appropriate for tests/common.test to be listed in simpletest.info?
I'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.
The layout of how we have an interface mixed between functions, is that appropriate? Or, should it be at the start or end of the file? Maybe someplace else?
I don't think it really matters, but we could move it to a different class if that's prefered.
Comment #19RobLoach CreditAttribution: RobLoach commented
|Needs review
|» Postponed
This 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 #20Wim Leers CreditAttribution: Wim Leers commented
Great work! The same should be applied to CSS preprocessing.
Comment #21sun CreditAttribution: sun commented
Comment #22andreiashu CreditAttribution: andreiashu commented
nice, subscribing
Comment #25Wim Leers CreditAttribution: Wim Leers commented
|Needs work
|» Needs review
Failed: 12749 passes, 7 fails, 15 exceptions View
Rerolled the patch. Didn't test it, just a straight re-roll.
Comment #27SeanBannister CreditAttribution: SeanBannister commented
Comment #28sun CreditAttribution: sun commented
|» 8.x-dev
Comment #29catch CreditAttribution: catch commented
Comment #30threading_signals CreditAttribution: threading_signals commented
Comment #31Owen Barton CreditAttribution: Owen Barton commented
|Make JS Preprocessing Pluggable
|» Make JS & CSS Preprocessing Pluggable
Agree with #20 that CSS should be included, so updating title
Comment #32dvessel CreditAttribution: dvessel commented
Comment #33mikey_p CreditAttribution: mikey_p commented
Comment #34rupl CreditAttribution: rupl commented
Comment #35RobLoach CreditAttribution: RobLoach commented
+++ includes/common.inc 31 Aug 2009 17:13:02 -0000
@@ -3401,44 +3401,68 @@ function drupal_add_tabledrag($table_id,
- * @param $filename
- * The name of the aggregate JS file.
- * @return
- * The name of the JS file.
+ * Returns a JavaScript preprocessing object that implements
+ * JSPreprocessingInterface.
Should be one line of comments rather than two.
+++ includes/common.inc 31 Aug 2009 17:13:02 -0000
@@ -3401,44 +3401,68 @@ function drupal_add_tabledrag($table_id,
- $contents .= file_get_contents($path) . ';';
+function drupal_js_preprocessor() {
+ static $instance;
+ if (empty($instance)) {
+ // Retrieve the preprocess system.
So excited to see what this spawns in contrib :-) .
+++ includes/common.inc 31 Aug 2009 17:13:02 -0000
@@ -3401,44 +3401,68 @@ function drupal_add_tabledrag($table_id,
+ $class = variable_get('preprocess_js_system', 'DrupalPreprocessJS');
+ // Allow for lazy loading of the class.
+ if (drupal_autoload_class($class)) {
+ $interfaces = class_implements($class);
+ if (isset($interfaces['JSPreprocessingInterface'])) {
+ $instance = new $class;
+ }
+ else {
+ throw new Exception(t('Class %class does not implement interface %interface', array('%class' => $class, '%interface' => 'JSPreprocessingInterface')));
Is it just me, or does DrupalPreprocessJS not exist in this patch?
+++ modules/simpletest/tests/common.test 31 Aug 2009 17:13:03 -0000
@@ -871,6 +874,23 @@
+ function testPluggablePreprocessor() {
+ // Switch the preprocessor.
+ variable_set('preprocess_js', TRUE);
+ variable_set('preprocess_js_system', 'testJSPreprocessing');
+ // Add some JavaScript to test the preprocessor system.
Some tab characters.
5 days to next Drupal core point release.
Comment #36skottler CreditAttribution: skottler commented
Comment #37ericduran CreditAttribution: ericduran commented
Comment #38RobLoach CreditAttribution: RobLoach commented
Not sure where I found this, but it definitely looks awesome: https://github.com/kriswallsmith/assetic
Comment #39catch CreditAttribution: catch commented
|» task
|» Major
#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.
Comment #40ericduran CreditAttribution: ericduran commented
After 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 #42nod_ CreditAttribution: nod_ commented
Comment #43RobLoach CreditAttribution: RobLoach commented
@Rob Loach it doesn't look like Assetic does aggregation of files. Is this the case?
It aggregates the files by default.
use Assetic\Asset\AssetCollection;
use Assetic\Asset\FileAsset;
use Assetic\Asset\GlobAsset;
$js = new AssetCollection(array(
new FileAsset('/path/to/another.js'),
new FileAsset('/path/to/different/file.js'),
// the code is aggregated when the asset is dumped:
echo $js->dump();
For our use, we would probably need to $js->dump() into the aggregated file.
Comment #45nod_ CreditAttribution: nod_ commented
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 #46Owen Barton CreditAttribution: Owen Barton commented
I 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 #47JohnAlbin CreditAttribution: JohnAlbin commented
I'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 #48Wim Leers CreditAttribution: Wim Leers commented
#46 is spot-on.
Comment #49ZenDoodles CreditAttribution: ZenDoodles commented
Tagging for issue summary
Comment #50catch CreditAttribution: catch commented
Yep, completely agreed with #46, they're two separate systems and should both be pluggable.
Comment #51Zgear CreditAttribution: Zgear commented
ah forgot to remove the issue summary initiative... done!
Comment #52klonos CreditAttribution: klonos commented
I 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.0klonos CreditAttribution: klonos commented
The issue required an issue summary to be moved forward
Comment #52.1klonos CreditAttribution: klonos commented
...made instances of "assetic" a link to the github project: https://github.com/kriswallsmith/assetic
Comment #53cosmicdreams CreditAttribution: cosmicdreams commented
If we're voting +1 for assetic
Comment #54effulgentsia CreditAttribution: effulgentsia commented
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
I'm wondering how to reconcile this statement with what's being proposed in #1542344: Use AMD for JS architecture
Comment #55nod_ CreditAttribution: nod_ commented
Comment #56nod_ CreditAttribution: nod_ commented
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 #57cosmicdreams CreditAttribution: cosmicdreams commented
Hey, nod_, If I can learn enough of the plugin system I want to help with this.
Comment #58nod_ CreditAttribution: nod_ commented
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 #59Owen Barton CreditAttribution: Owen Barton commented
I 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 #60cosmicdreams CreditAttribution: cosmicdreams commented
Had 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 #61neclimdul CreditAttribution: neclimdul commented
Since we're interested in using the plugin system, tagging for aggregation.
Comment #62neclimdul CreditAttribution: neclimdul commented
Also, 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 #63Kiphaas7 CreditAttribution: Kiphaas7 commented
Very interested in this. If I can help, I will.
Comment #64nod_ CreditAttribution: nod_ commented
Trying out an implementation with Assetic #1751602: Use Assetic to package JS and CSS files help welcome :)
Comment #65Owen Barton CreditAttribution: Owen Barton commented
Please 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 #66mcjim CreditAttribution: mcjim commented
I'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 #67Kiphaas7 CreditAttribution: Kiphaas7 commented
Awesome, keep up the good work!
Comment #68mcjim CreditAttribution: mcjim commented
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
Another 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 #69mcjim CreditAttribution: mcjim commented
|Needs work
|» Needs review
Comment #71mcjim CreditAttribution: mcjim commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 41,400 pass(es), 136 fail(s), and 90 exception(s). View
Registered the bundle in install.core.inc
Hopefully will work…
Comment #73mcjim CreditAttribution: mcjim commented
Looks like it's the call to
drupal_get_css() in
ajax_render() which is causing the fails.
Comment #74mcjim CreditAttribution: mcjim commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 41,566 pass(es), 145 fail(s), and 105 exception(s). View
Small amends to keep up-to-date with core and to (hopefully) fix the ajax_render() fails.
Comment #76mcjim CreditAttribution: mcjim commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 42,120 pass(es). View
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 #77mcjim CreditAttribution: mcjim commented
|Needs review
|» Needs work
After 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 #78kscheirer CreditAttribution: kscheirer commented
Comment #79RobLoach CreditAttribution: RobLoach commented
Comment #80mcjim CreditAttribution: mcjim commented
Comment #81Owen Barton CreditAttribution: Owen Barton commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 352951-js.patch. Unable to apply patch. See the log in the details link for more information. View
Took 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 #82neclimdul CreditAttribution: neclimdul commented
I don't think the JsAssetManager class is in the patch.
Comment #83attiks CreditAttribution: attiks commented
This is my first look in a long time to this, I like the idea, some small comments:
+++ b/core/includes/common.incundefined
@@ -3873,81 +3848,19 @@ function drupal_js_defaults($data = NULL) {
+ $javascript = drupal_container()->get('js_asset_manager');
+ return;
+ $javascript->set($js);
the return looks strange?
+++ b/core/lib/Drupal/Core/Asset/AssetManagerFactory.phpundefined
@@ -0,0 +1,55 @@
+ // Need to return other config, too, bundler and packager variables for each type. ¶
comment too long and trailing whitespace
Comment #85mcjim CreditAttribution: mcjim commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch pluggable-asset-management-352951-85.patch. Unable to apply patch. See the log in the details link for more information. View
Re-roll of patch in 76 to keep up with HEAD.
Now looking at a re-roll of 81.
Comment #86cweagans CreditAttribution: cweagans commented
So 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 #87Owen Barton CreditAttribution: Owen Barton commented
I 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 #88hass CreditAttribution: hass commented
I'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 #89YesCT CreditAttribution: YesCT commented
Comment #91YesCT CreditAttribution: YesCT commented
Comment #92Wim Leers CreditAttribution: Wim Leers commented
I 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 #93Owen Barton CreditAttribution: Owen Barton commented
Agreed. 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 #94Wim Leers CreditAttribution: Wim Leers commented
Status 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:
- Only refactor CSS aggregation, to get feedback on that before applying essentially the exact same pattern to JS aggregation.
- Ensure that the different phases are completely PHPUnit testable.
- Have as close to zero as possible changes to the actual logic; i.e. only move things around. This is not about making aggregation itself better, it's only about making the process of aggregation pluggable and cleanly overridable.
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 #95cosmicdreams CreditAttribution: cosmicdreams commented
excellent news!
Comment #96Wim Leers CreditAttribution: Wim Leers commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 57,136 pass(es), 17 fail(s), and 4 exception(s). View
The proposed structure:
AssetCollectionOptimizerInterface: optimizes the whole collection of assets, returns a (in our case: smaller because aggregated) set of assets
AssetCollectionGrouperInterface: used/called by
AssetCollectionOptimizerInterface, 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 by
AssetCollectionOptimizerInterface, optimizes an individual asset, returns optimized data as string
AssetDumperInterface: used/called by
AssetCollectionOptimizerInterface, dumps an (optimized) asset to persistent storage, returns an URI
AssetCollectionRendererInterface: can render an asset collection into a render array
As 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:
- When CSS aggregation is disabled, the original collection of CSS assets will be rendered, otherwise the smaller collection of CSS assets as returned by
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 enabled
- IE 31 limit handling: instead of always running
CssCollectionGrouper(which belongs to aggregation), which breaks separation of concerns, instead added very basic grouping support within
CssCollectionRendererjust for this edge case; remove this when we support IE >=10
- minor: groups used to contain an individual item (asset)'s basename. See CssGrouper::group().
PHPUnit 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 #97sdboyer CreditAttribution: sdboyer commented
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.
+++ b/core/includes/common.inc
@@ -2227,393 +2080,16 @@ function drupal_aggregate_css(&$css_groups) {
+ // Aggregate the CSS if necessary, but only during normal site operation.
+ if (!defined('MAINTENANCE_MODE') && config('system.performance')->get('css.preprocess')) {
+ $optimizer = new CssCollectionOptimizer(new CssCollectionGrouper(), new CssOptimizer(), new CssDumper());
+ $css_assets = $optimizer->optimize($css_assets);
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.
+++ b/core/lib/Drupal/Core/Asset/AssetCollectionGrouperInterface.php
@@ -0,0 +1,24 @@
+ */
+ public function group(array $assets);
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.
+++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.php
@@ -0,0 +1,173 @@
+ $query_string = variable_get('css_js_query_string', '0');
this should be injected, but we can do it in a later patch.
+++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.php
@@ -0,0 +1,173 @@
+ // below IE<10's limit of 31 total CSS inclussion tags, we handle this
+++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.php
@@ -0,0 +1,173 @@
+ throw new \Exception('Invalid CSS asset type.');
let's define an exception of our own, here.
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
@@ -0,0 +1,214 @@
+ /**
+ * {@inheritdoc}
+ */
+ // @see drupal_aggregate_css()
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.
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
@@ -0,0 +1,214 @@
+ * Contents of the stylesheet, including any resolved @import commands.
+ */
+ // @see drupal_load_stylesheet()
but in this case, we can definitely move the @see up to the bottom of the phpdoc.
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
@@ -0,0 +1,214 @@
+ protected function loadFile($file, $optimize = NULL, $reset_basepath = TRUE) {
+ // These statics are not cache variables, so we don't use drupal_static().
+ static $_optimize, $basepath;
+ if ($reset_basepath) {
+ $basepath = '';
+ }
why are we using a static in a class context?
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
@@ -0,0 +1,214 @@
+ protected function rewriteFileURI($matches, $base = NULL) {
+ static $_base;
+ // Store base path for preg_replace_callback.
+ if (isset($base)) {
+ $_base = $base;
+ }
again, statics in an class context - just use an object property, i think.
Comment #99Wim Leers CreditAttribution: Wim Leers commented
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)
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.
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 #100catch CreditAttribution: catch commented
I'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 #101sdboyer CreditAttribution: sdboyer commented
yeah, 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 supporting
again, 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 #102catch CreditAttribution: catch commented
Is 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 #103Wim Leers CreditAttribution: Wim Leers commented
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.
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.
...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 supporting
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:
IE 31 limit handling: instead of always running CssCollectionGrouper (which belongs to aggregation), which breaks separation of concerns, instead added very basic grouping support within CssCollectionRenderer just for this edge case; remove this when we support IE >=10
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 :)
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.
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 #104Owen Barton CreditAttribution: Owen Barton commented
For 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 #105sdboyer CreditAttribution: sdboyer commented
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
yeah, 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.
Ok, this is what I'll selectively remember then :) ;)
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 #106pounard CreditAttribution: pounard commented
That said, if Drupal is generating lots of bundles frequently, that is a very strong indication that our aggregation strategy is broken
I 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 #107Wim Leers CreditAttribution: Wim Leers commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch pluggable_preprocessing-352951-107.patch. Unable to apply patch. See the log in the details link for more information. View
Wow, 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 into
I had to make a few methods in
CssOptimizer public because
color.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, then
php 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 #109larowlan CreditAttribution: larowlan commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 58,093 pass(es), 9 fail(s), and 0 exception(s). View
Comment #110sdboyer CreditAttribution: sdboyer commented
Comment #111Wim Leers CreditAttribution: Wim Leers commented
So #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 #112catch CreditAttribution: catch commented
I'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 #113Wim Leers CreditAttribution: Wim Leers commented
catch: 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 #114catch CreditAttribution: catch commented
is because without also implementing the JS half, we haven't fully validated the architecture yet.
In 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 #115sdboyer 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 #116larowlan CreditAttribution: larowlan commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 56,819 pass(es), 8 fail(s), and 0 exception(s). View
Chases head.
Neither of those fails occur locally.
Comment #118larowlan CreditAttribution: larowlan commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 58,002 pass(es), 4 fail(s), and 0 exception(s). View
|781 bytes
So some strict warnings when browsers isn't set.
Hopefully green
Comment #120larowlan CreditAttribution: larowlan commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 58,642 pass(es), 1 fail(s), and 0 exception(s). View
|680 bytes
This time?
Comment #122Wim Leers CreditAttribution: Wim Leers commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 58,041 pass(es), 1 fail(s), and 0 exception(s). View
#118 & #120 fix the symptom, not the cause. The thing is,
drupal_add_css() guarantees that the "browsers" key defaults to
array(). 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 #124Wim Leers CreditAttribution: Wim Leers commented
|Needs work
|» Needs review
Comment #126Wim Leers CreditAttribution: Wim Leers commented
Testbot is utterly borked:
[08:48:35] Command [git apply --check -p1 /var/lib/drupaltestbot/sites/default/files/review/pluggable_preprocessing-352951-122.patch 2>&1] failed
Duration 0 seconds
Directory [/var/lib/drupaltestbot/sites/default/files/checkout]
Status [1]
Output: [error: patch failed: core/includes/common.inc:25
error: core/includes/common.inc: patch does not apply
error: core/lib/Drupal/Core/Asset/AssetCollectionGrouperInterface.php: already exists in working directory
error: core/lib/Drupal/Core/Asset/AssetCollectionOptimizerInterface.php: already exists in working directory
error: core/lib/Drupal/Core/Asset/AssetCollectionRendererInterface.php: already exists in working directory
error: core/lib/Drupal/Core/Asset/AssetDumperInterface.php: already exists in working directory
error: core/lib/Drupal/Core/Asset/AssetOptimizerInterface.php: already exists in working directory
error: core/lib/Drupal/Core/Asset/CssCollectionGrouper.php: already exists in working directory
error: core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php: already exists in working directory
error: core/lib/Drupal/Core/Asset/CssCollectionRenderer.php: already exists in working directory
error: core/lib/Drupal/Core/Asset/CssDumper.php: already exists in working directory
error: core/lib/Drupal/Core/Asset/CssOptimizer.php: already exists in working directory
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 #127Wim Leers CreditAttribution: Wim Leers commented
While 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 #128Wim Leers CreditAttribution: Wim Leers commented
|Needs work
|» Needs review
|Issue tags:
|-frontend performance, -Plugin system, -happy princess API
Comment #130Wim Leers CreditAttribution: Wim Leers commented
Now 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 #131Wim Leers CreditAttribution: Wim Leers commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 57,976 pass(es). View
And voila, part two: pluggable JS aggregation!
- New:
- Renamed
AssetDumper(more generic), because 99% of the code was identical for the two.
- No unit tests for JS, because
JavaScriptTestactually has very decent test coverage. Also hardly modified
JavaScriptTest: only removed 3 places where it was calling
drupal_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 #132larowlan CreditAttribution: larowlan commented
|Needs review
|» Needs work
First up, this patch is awesome, great work.
Some nitpicks and some other general observations.
+++ b/core/lib/Drupal/Core/Asset/AssetCollectionGrouperInterface.phpundefined
@@ -0,0 +1,24 @@
+ * Contains Drupal\Core\Asset\AssetCollectionGrouperInterface.
+++ b/core/lib/Drupal/Core/Asset/AssetCollectionOptimizerInterface.phpundefined
@@ -0,0 +1,24 @@
+ * Contains Drupal\Core\Asset\AssetCollectionOptimizerInterface.
+++ b/core/lib/Drupal/Core/Asset/AssetCollectionRendererInterface.phpundefined
@@ -0,0 +1,24 @@
+ * Contains Drupal\Core\Asset\AssetCollectionRendererInterface.
+++ b/core/lib/Drupal/Core/Asset/AssetDumperInterface.phpundefined
@@ -0,0 +1,26 @@
+ * Contains Drupal\Core\Asset\AssetDumperInterface.
+++ b/core/lib/Drupal/Core/Asset/AssetOptimizerInterface.phpundefined
@@ -0,0 +1,24 @@
+ * Contains Drupal\Core\Asset\AssetCollectionOptimizerInterface.
needs leading \
+++ b/core/lib/Drupal/Core/Asset/AssetCollectionGrouperInterface.phpundefined
@@ -0,0 +1,24 @@
+ * @return array
+++ b/core/lib/Drupal/Core/Asset/AssetCollectionOptimizerInterface.phpundefined
@@ -0,0 +1,24 @@
+ * @return array
+++ b/core/lib/Drupal/Core/Asset/AssetCollectionRendererInterface.phpundefined
@@ -0,0 +1,24 @@
+ * @return array
+++ b/core/lib/Drupal/Core/Asset/AssetDumperInterface.phpundefined
@@ -0,0 +1,26 @@
+ * @return string
+++ b/core/lib/Drupal/Core/Asset/AssetOptimizerInterface.phpundefined
@@ -0,0 +1,24 @@
+ * @return string
needs empty line before
+++ b/core/lib/Drupal/Core/Asset/AssetDumper.phpundefined
@@ -0,0 +1,52 @@
+ if (config('system.performance')->get($file_extension . '.gzip') && extension_loaded('zlib')) {
We should be injecting the ConfigFactory
+++ b/core/lib/Drupal/Core/Asset/CssCollectionGrouper.phpundefined
@@ -0,0 +1,95 @@
+ // items in the group must share the same information that would need to be
+++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.phpundefined
@@ -0,0 +1,161 @@
+ // Per the W3C specification at http://www.w3.org/TR/REC-CSS2/cascade.html#at-import,
+ // @import rules must proceed any other style, so we move those to the top.
+++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.phpundefined
@@ -0,0 +1,174 @@
+ // Loop through all CSS assets, by key, to allow for the special IE workaround.
+ // The dummy query string needs to be added to the URL to control
+ // In addition to IE's limit of 31 total CSS inclusion tags, it also
+ // (separated by \n) to be treated as a completely different string.
+ // This means that we can use ^ and $ on one line at a time, and not
+ // For inline CSS to validate as XHTML, all CSS containing XHTML needs to be
+ // wrapped in CDATA. To make that backwards compatible with HTML 4, we need to
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.phpundefined
@@ -0,0 +1,222 @@
+ // Anchor all paths in the CSS with its base URL, ignoring external and absolute paths.
+ // Remove multiple charset declarations for standards compliance (and fixing Safari problems).
+++ b/core/lib/Drupal/Core/Asset/JsCollectionGrouper.phpundefined
@@ -0,0 +1,80 @@
+ // the information that determines when a new group is needed, but the order
+ // of keys in the array doesn't matter, and we don't want a new group if all
+++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.phpundefined
@@ -0,0 +1,139 @@
+ // Append a ';' and a newline after each JS file to prevent them from running together.
More than 80 chars
+++ b/core/lib/Drupal/Core/Asset/CssCollectionGrouper.phpundefined
@@ -0,0 +1,95 @@
+ case 'inline':
+ case 'external':
Needs blank line before
+++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.phpundefined
@@ -0,0 +1,161 @@
+use Drupal;
+++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.phpundefined
@@ -0,0 +1,139 @@
+use Drupal;
we shouldn't use top-level objects, instead \Drupal
+++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.phpundefined
@@ -0,0 +1,161 @@
+ * @var \Drupal\Core\Asset\CssCollectionGrouper object.
+ * @var \Drupal\Core\Asset\CssOptimizer object.
+ * @var \Drupal\Core\Asset\AssetDumper object.
remove ' object.'
+++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.phpundefined
@@ -0,0 +1,161 @@
+ $map = Drupal::state()->get('drupal_css_cache_files') ?: array();
+ Drupal::state()->set('drupal_css_cache_files', $map);
+++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.phpundefined
@@ -0,0 +1,139 @@
+ $map = Drupal::state()->get('system.js_cache_files') ?: array();
+ Drupal::state()->set('system.js_cache_files', $map);
Can we inject the state service?
+++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.phpundefined
@@ -0,0 +1,161 @@
+ return hash('sha256', serialize($css_data));
two spaces?
+++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.phpundefined
@@ -0,0 +1,174 @@
+ *
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.phpundefined
@@ -0,0 +1,222 @@
+ *
Missing docblock
+++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.phpundefined
@@ -0,0 +1,174 @@
+ $query_string = variable_get('css_js_query_string', '0');
+++ b/core/lib/Drupal/Core/Asset/JsCollectionRenderer.phpundefined
@@ -0,0 +1,97 @@
+ $default_query_string = variable_get('css_js_query_string', '0');
+ $js_version_string = variable_get('drupal_js_version_query_string', 'v=');
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.
+++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.phpundefined
@@ -0,0 +1,174 @@
+ $import[] = '@import url("' . check_plain(file_create_url($next_css_asset['data']) . '?' . $query_string) . '");';
Can we inject \Drupal\Component\Utility\String and use checkPlain, will help unit testing too
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.phpundefined
@@ -0,0 +1,222 @@
+ // @see drupal_aggregate_css()
+ // @see drupal_build_css_cache()
+ // @see drupal_load_stylesheet()
+ // @see _drupal_load_stylesheet()
+ // @see drupal_load_stylesheet_content()
don't think this matches coding standards, should be in the docblock?
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.phpundefined
@@ -0,0 +1,222 @@
+ * @see loadFile()
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()
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.phpundefined
@@ -0,0 +1,222 @@
+ protected function loadNestedFile($matches) {
+ public function rewriteFileURI($matches, $base = NULL) {
+++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.phpundefined
@@ -0,0 +1,139 @@
+ protected function generateHash(array $js_group) {
Missing @params/@return from doc block
+++ b/core/lib/Drupal/Core/Asset/JsCollectionRenderer.phpundefined
@@ -0,0 +1,97 @@
+ $element['#value'] = 'var drupalSettings = ' . drupal_json_encode(drupal_merge_js_settings($js_asset['data'])) . ";";
Out of scope: we really need an OO version of these
+++ b/core/modules/system/lib/Drupal/system/Tests/Common/CascadingStylesheetsTest.phpundefined
@@ -99,8 +99,12 @@ function testRenderExternal() {
+ $config = config('system.performance');
+++ b/core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.phpundefined
@@ -562,4 +571,19 @@ function testAddJsFileWithQueryString() {
+ * Calculates the aggregated file name of a group of JavaScript assets.
Missing @param/@return
+++ b/core/tests/Drupal/Tests/Core/Asset/CssCollectionRendererUnitTest.phpundefined
@@ -0,0 +1,572 @@
+if (!function_exists('file_create_url')) {
+if (!function_exists('variable_get')) {
+if (!function_exists('check_plain')) {
+++ b/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.phpundefined
@@ -0,0 +1,227 @@
+ function file_create_url($uri) {
+ function file_uri_scheme($uri) {
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 #133Wim Leers CreditAttribution: Wim Leers commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 58,002 pass(es). View
Yay, patch is green! Which means the nitpicking can now begin, as @larowlan already started doing :)
We should be injecting the ConfigFactory
Can we inject the state service?
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.
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.
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.
… 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.
don't think this matches coding standards, should be in the docblock?
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 :)
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()
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.
Missing @params/@return from doc block
… 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 #134dawehner CreditAttribution: dawehner commented
+++ b/core/tests/Drupal/Tests/Core/Asset/CssCollectionGrouperUnitTest.phpundefined
@@ -0,0 +1,216 @@
+ function testGrouper() {
+ $css_assets = array(
I 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 #135Wim Leers CreditAttribution: Wim Leers commented
#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 #136sdboyer CreditAttribution: sdboyer commented
|Needs review
|» Needs work
i'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 #137sdboyer CreditAttribution: sdboyer commented
ok, 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 #138sdboyer CreditAttribution: sdboyer commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 57,997 pass(es). View
first, just a quick reroll to catch up with #2026255: Make CascadingStylesheetsTest 3500% faster. no interdiff b/c the old patch no longer applied.
Comment #139sdboyer CreditAttribution: sdboyer commented
fyi, 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 #140sdboyer CreditAttribution: sdboyer commented
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
ok, 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:
<style media="all">
/* ]]> */
<style media="all">
body { padding: 0px; }
/* ]]> */
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 #142Wim Leers CreditAttribution: Wim Leers commented
the only question i have - why do we optimize (minify) css, but not js?
Because we never have done that, for fear of breaking crappy JS… :) Have fun reading the 172 comments in #119441: Compress JS aggregation.
for some reason, a leading newline isn't making it into the output in the patch
Wow, wtf! I'll try to debug this sucker.
Review of #140's interdiff
The interdiff is much simpler than I expected :)
+++ b/core/core.services.ymlundefined
@@ -532,3 +532,23 @@ services:
+ asset.css.dumper:
+ class: Drupal\Core\Asset\AssetDumper
+++ b/core/core.services.ymlundefined
@@ -532,3 +532,23 @@ services:
+ asset.js.dumper:
+ class: Drupal\Core\Asset\AssetDumper
This means we'll have two instances in the container?
Comment #143Wim Leers CreditAttribution: Wim Leers commented
|Needs work
|» Needs review
|Issue tags:
|-frontend performance, -Plugin system, -happy princess API
Comment #145sdboyer 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 #146sdboyer CreditAttribution: sdboyer commented
|Needs work
|» Needs review
|Issue tags:
|-frontend performance, -Plugin system, -happy princess API
Comment #148Wim Leers CreditAttribution: Wim Leers commented
|» base system
|» Wim Leers
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 56,273 pass(es). View
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-op
JsOptimizer 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.
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).
With all that out of the way, this patch should now be be RTBC'able… :)
Comment #149sdboyer CreditAttribution: sdboyer commented
hah. 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 #150JohnAlbin CreditAttribution: JohnAlbin commented
|Needs review
|» Needs work
new file: core/modules/system/lib/Drupal/system/Tests/Common/CascadingStylesheetsTest.php.rej
Comment #151JohnAlbin CreditAttribution: JohnAlbin commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 56,687 pass(es). View
I've edited the patch by hand to remove that one hunk. That way I can continue my review while the testbot works.
Comment #152sdboyer CreditAttribution: sdboyer commented
oh 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 #153Gábor Hojtsy CreditAttribution: Gábor Hojtsy commented
|» Critical
This 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 #154JohnAlbin CreditAttribution: JohnAlbin commented
|» Major
|Needs review
|» Reviewed & tested by the community
So 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:
<style media="all">
@import url("http://drupal8x.dev/core/modules/system/css/system.module.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/field/css/field.module.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/user/css/user.module.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/system/css/system.theme.css?mp9lyp");
<style media="all">
@import url("http://drupal8x.dev/core/modules/views/css/views.module.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/contextual/css/contextual.module.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/contextual/css/contextual.theme.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/edit/css/edit.module.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/edit/css/edit.theme.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/edit/css/edit.icons.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/filter/css/filter.caption.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/shortcut/css/shortcut.module.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/shortcut/css/shortcut.theme.css?mp9lyp");
@import url("http://drupal8x.dev/core/assets/vendor/jquery.ui/themes/base/jquery.ui.core.css?mp9lyp");
@import url("http://drupal8x.dev/core/assets/vendor/jquery.ui/themes/base/jquery.ui.theme.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/contextual/css/contextual.toolbar.css?mp9lyp");
<style media="screen">
@import url("http://drupal8x.dev/core/modules/tour/css/joyride-2.0.3.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/tour/css/tour.module.css?mp9lyp");
<style media="all">
@import url("http://drupal8x.dev/core/modules/toolbar/css/toolbar.menu.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/toolbar/css/toolbar.module.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/toolbar/css/toolbar.theme.css?mp9lyp");
@import url("http://drupal8x.dev/core/modules/toolbar/css/toolbar.icons.css?mp9lyp");
<style media="all">
@import url("http://drupal8x.dev/core/themes/bartik/css/layout.css?mp9lyp");
@import url("http://drupal8x.dev/core/themes/bartik/css/style.css?mp9lyp");
@import url("http://drupal8x.dev/core/themes/bartik/css/colors.css?mp9lyp");
<style media="print">
@import url("http://drupal8x.dev/core/themes/bartik/css/print.css?mp9lyp");
And here it is after the patch:
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/system/css/system.module.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/field/css/field.module.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/user/css/user.module.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/system/css/system.theme.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/views/css/views.module.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/contextual/css/contextual.module.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/contextual/css/contextual.theme.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/edit/css/edit.module.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/edit/css/edit.theme.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/edit/css/edit.icons.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/filter/css/filter.caption.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/shortcut/css/shortcut.module.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/shortcut/css/shortcut.theme.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/assets/vendor/jquery.ui/themes/base/jquery.ui.core.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/assets/vendor/jquery.ui/themes/base/jquery.ui.theme.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/contextual/css/contextual.toolbar.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/tour/css/joyride-2.0.3.css?mp9huz" media="screen" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/tour/css/tour.module.css?mp9huz" media="screen" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/toolbar/css/toolbar.menu.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/toolbar/css/toolbar.module.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/toolbar/css/toolbar.theme.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/modules/toolbar/css/toolbar.icons.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/themes/bartik/css/layout.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/themes/bartik/css/style.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/themes/bartik/css/colors.css?mp9huz" media="all" />
<link rel="stylesheet" href="http://drupal8x.dev/core/themes/bartik/css/print.css?mp9huz" media="print" />
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 #155JohnAlbin CreditAttribution: JohnAlbin commented
|» Critical
Cross-posted with Gabor.
Comment #156YesCT CreditAttribution: YesCT commented
This issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #157catch CreditAttribution: catch commented
|Reviewed & tested by the community
|» Needs work
Overall this looks great, the current implementation hasn't leaked into the API at all from what I can see.
Several nitpicks:
+++ b/core/lib/Drupal/Core/Asset/AssetCollectionGrouperInterface.phpundefined
@@ -0,0 +1,25 @@
+ * @param array $assets
Should this describe the structure of a the $assets a bit more?
+++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.phpundefined
@@ -0,0 +1,177 @@
+ $query_string = variable_get('css_js_query_string', '0');
I didn't realise this was still a variable, not for here but do you know where the issue to convert this is?
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.phpundefined
@@ -0,0 +1,217 @@
+ // These statics are not cache variables, so we don't use drupal_static().
+ static $_optimize, $basepath;
+ if ($reset_basepath) {
+ $basepath = '';
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.
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.phpundefined
@@ -0,0 +1,217 @@
+ /**
+ * Loads stylesheets recursively and returns contents with corrected paths.
+ *
+ * This function is used for recursive loading of stylesheets and
+ * returns the stylesheet content with all url() paths corrected.
+ *
+ * @see Drupal\Core\Asset\AssetOptimizerInterface::loadFile()
+ */
No documentation for the $matches parameter.
+++ /dev/nullundefined
diff --git a/core/modules/simpletest/files/css_test_files/css_input_without_import.css.unoptimized.css b/core/modules/simpletest/files/css_test_files/css_input_without_import.css.unoptimized.css
deleted file mode 100644
index 118dfa4..0000000
index 118dfa4..0000000
--- a/core/modules/simpletest/files/css_test_files/css_input_without_import.css.unoptimized.css
--- a/core/modules/simpletest/files/css_test_files/css_input_without_import.css.unoptimized.css
+++ /dev/nullundefined
+++ /dev/nullundefined
+++ /dev/nullundefined
@@ -1,65 +0,0 @@
@@ -1,65 +0,0 @@
- * @file Basic css that does not use import
Looks like these two CSS files are removed but not put back?
Comment #158Wim Leers CreditAttribution: Wim Leers commented
|Needs work
|» Needs review
Should this describe the structure of a the $assets a bit more?
I 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 didn't realise this was still a variable, not for here but do you know where the issue to convert this is?
I don't know.
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.
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.
No documentation for the $matches parameter.
This is a (protected) helper function. There were no docs before either. I can add them if you want.
Looks like these two CSS files are removed but not put back?
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 #159catch CreditAttribution: catch commented
I 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 #160Wim Leers CreditAttribution: Wim Leers commented
|Needs review
|» Reviewed & tested by the community
PASSED: [[SimpleTest]]: [MySQL] 57,160 pass(es). View
#159: done.
Comment #161catch CreditAttribution: catch commented
|Make JS & CSS Preprocessing Pluggable
|» Change notice: Make JS & CSS Preprocessing Pluggable
|Reviewed & tested by the community
|» Active
|Issue tags:
|+Needs change record
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 #162Wim Leers CreditAttribution: Wim Leers commented
|Change notice: Make JS & CSS Preprocessing Pluggable
|» Make JS & CSS Preprocessing Pluggable
|» Fixed
|Issue tags:
|-Needs change record
Comment #163Wim Leers CreditAttribution: Wim Leers commented
I 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.)
#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: Make CSS and JS aggregation sensitive to file changes and be enabled by default
#1014086: Race conditions, stampedes and cold cache performance issues with css/js aggregation
Moved to D9:
#494498: Add CSS parsing capability to Drupal
Comment #164hass CreditAttribution: hass commented
Comment #165ParisLiakos CreditAttribution: ParisLiakos commented
+++ b/core/includes/common.incundefined
@@ -26,6 +26,15 @@
+use Drupal\Core\Asset\CssCollectionRenderer;
+use Drupal\Core\Asset\CssCollectionOptimizer;
+use Drupal\Core\Asset\CssCollectionGrouper;
+use Drupal\Core\Asset\CssOptimizer;
+use Drupal\Core\Asset\JsCollectionRenderer;
+use Drupal\Core\Asset\JsCollectionOptimizer;
+use Drupal\Core\Asset\JsCollectionGrouper;
+use Drupal\Core\Asset\AssetDumper;
so 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 #166Wim Leers CreditAttribution: Wim Leers commented
#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 #167ParisLiakos CreditAttribution: ParisLiakos commented
Comment #168.0(not verified) CreditAttribution: commented
Updated issue summary.
Comment #170grendzy CreditAttribution: grendzy commented
Cross-linking: This commit caused a regression, by removing the fix from #1961340: CSS aggregation breaks file URL rewriting because it abuses it.


text:
I'm not asking to be snarky, I'm honestly curious. I am 31 years old. When I was in school I did not know one person who was allergic to peanuts. There was a kid who was allergic to milk, so she got something else at lunch. Is there more of a "peanut allergy epidemic" nowadays?Ā
I understand that it can be life threatening in some cases, that's why I'm curious. What did the people with the peanut allergies do before it was banned? Did the USA have a number of deaths in schools due to peanuts? Is that why they are banned, or is it more of a preventative issue?

subset: commoncrawl


text:
Quick Look
Property brokered by
Get more listing details on brokerage site
Presented By:
Emma Price
(+34) 963 504 444
Email Agent
Emma Price
(+34) 963 504 444
Email Agent
Property DetailsStately-style apartment with a surface area of 170m2 conveniently distributed to be renovated according to the needs of the new owner. The building is very quiet with very few neighbors and the property is located on the second floor, in the heart of El Carmen district, next to a great variety of landmark monuments.
Area Detailed Information
Area Weather
Area Cost Index
Area Property Prices
Additional Property InformationThis condo listing located at El Carmen, Valencia offers six bedrooms and two bathrooms. The property is currently offered at $320,300.
Includes features like city views,walking distance to the centre,elevator,near to amenities,near to transport,near to airport.
This property is listed by Emma Price of Inmobiliaria Rimontgó headquartered in Spain.


text:
by Marina
My father took himself off to hospital with pains in the stomach in November 2010. The short story is that he died 2.5 months later, the longer version is of pain and more pain. After three failed major operations and a bacterial infection Septicema, he started talking about how we were to arrange his funeral. The week before he died he asked his grandkids to come and visit him. The day before he ate like a starving man. The hospital called late that night to let us know that he had a stroke and if we could come the next day as he didnt have long. I watched him go and I felt nothing. Three days later I was a mess. Two weeks later my 36 year old cousin died of unknown reasons (we thought suicide). Later after the autopsy we found he had bad lungs. My auntie followed her grandson (she died of liver cancer). My mother had been in a nursing home for 7 years with Dementia. She was the last to go 2 months after my father went. She couldnt live with the fact that he wouldnt be visiting her three times a week. My sister and I were trying to get over the other three deaths and we could only visit once a week. I say my mother go as well. My work has been my saviour and so have my children and their family. I do have a few good friends that do understand what I am going through, as they have also lost their parents. Everytime I see a film on someone dying, I cant stop crying. Its been worse in the last few weeks - too much to do in the garden - I dont have help. House is a mess - half renovated. I live with a dog and two cats and they are my life savers. I used to talk to my father like a friend and I miss him terribly. He was my life. My mother who I dont seem to miss that much and I feel awful that I dont. Too much to bear at times. I prefer to be alone with my pets rather then go out. I seem to visit my parents friends that they had and they want me to be a part of their lives. This also helps me to cope. I dont know where I am headed and not sure about what the future will hold for me. I am just happy working with the aged and disability and for now that is enough for me.
Comments for Four family members passed last year
Click here to add your own comments
Tap into the compassion, support and wisdom of the
Free Stress


text:
Master of Science in Analytics
Upcoming Events
There are currently no upcoming events. Please check back regularly.
Get Started
Spring 2018 Deadline
October 2, 2017
Autumn 2018 Deadline
April 2, 2018
MScA News
Today's organization leaders and decision-makers are faced with vast amounts of data as they confront complex issues and strategic challenges on a daily basis. The ability to make well-reasoned decisions hinges on the skillful interpretation of a broad range of available information.
The University of Chicago's Graham School Master of Science in Analytics provides students with the advanced training necessary to draw insights from real data and to apply analytical skills to solve real problems. The technical know-how, real-world insights and leadership edge gained in this program give students the capability to utilize data analytics to develop real solutions.
Structured as an in-person program, the MScA is a masters level analytics program that is fully classroom-based, providing a team-based learning approach and dynamic instruction from experts in the field. The program can be completed on a part- or full-time basis allowing students to progress at a pace that meets their needs.


text:
We help you find a Ingatestone euphonium tutor. Euphonium tuition are a brilliant method to begin an entertaining new hobby, pick up a new talent and attain confidence with euphonium. Try our service to find Ingatestone euphonium tutors and select which matches your needs so you can enjoy your euphonium lessons in Ingatestone today!
This is the best place to contact Ingatestone euphonium teachers in the UK, helping you find Ingatestone euphonium tuition close to you and Ingatestone tutors to assist with any instrument like piano, violin or guitar. We can help you find Ingatestone euphonium lessons or a Ingatestone euphonium teacher to prepare for euphonium grades and develop your euphonium skills! We order Ingatestone euphonium tutors in your area by their lesson prices and distance from you.
If you are a euphonium teacher wishing to offer euphonium tuition services in Ingatestone or anywhere else across the UK, please register with First Tutors: Music or learn about us here. Becoming a Ingatestone euphonium teacher is a great way to re-affirm your own skills through giving euphonium tuition while helping others, all in the Ingatestone area!


text:
The green/red bar may be misleading. Each side doesn't have a separate meaning. It's just 1 magic percentage which indicates how "good" a product is for selling. Thumbs up and down just helps indicate which end of the bar is good and which is bad. The number just helps read the specific ratio. You will notice the two numbers always add up to 100%.
Discover four of the most profitable niches on eBay and get details for the best suppliers for these niches. Enter your details below and we will send them instantly.
Your list of profitable markets and supplier details are on their way to your email inbox.
Be sure to add firstname.lastname@example.com to your safelist to ensure we can deliver the free suppliers details to you.
If you haven't received an email from us in 5 minutes, please check your spam folder or email us at email@example.com.


text:
OneStop Cal State Fullerton
by: Helix Mobile Marketing • 0
All Android applications categories
by: Helix Mobile Marketing • 0
This OneStop App is just that, your must-have one stop Deals and Student Needs App for Cal State Fullerton students and faculty.
Within this App, you will be provided with great deals on food and services, local job listings, local housing listings, financial aid, and much more at no cost to you. This App is your OneStop Directory for all of your needs!
Features include: Exclusive Deals at Local Businesses, Job Postings, Housing Information, Financial Aid Information, Campus Events, Break Room Videos, Find Movie Times and Purchase Tickets, QR Scanner, and more!
Download this powerful OneStop App today!
Download OneStop Cal State Fullerton
Free - V1.399 - 3M
Sorry ...
This app is no longer available.
Share this app


text:
I never set out to confer my toddlers chores to do. You may possibly be
thinking, "That's too young! Toddlers can't do chores!" That's
what I rumination too, thatability is until I was unremittingly swinging over
my boys piece provoking to do my own menage chores.
Let's facade it, toddlers deprivation to back mom, even once mom doesn't
want or need assist. But what better-quality way to edward teach your toddlers to
do chores than rental them assistance mom?
The digit of chores you pass your nestling to do will be on
your fry. One kids privation to give a hand much than others. At this
age, the far-reaching entity is to pirate them to unspotted up after
themselves, not to try to get them to spic the whole flat.
Double End Spotweld Cutter - 3/8 In - 3 Pk / Milwaukee Hand Trucks 33884 Aluminum Fold Up Hand Truck / BernzoMatic JT680 019110 Jumbo Flame Torch Head / Large Space Case Titanium Grinder + Cali Crusher Pollen / 6207-2RS Bearing 35x72x17 Sealed Ball Bearings / Custom Leathercraft 130L Subcontractor Flex Grip Work / Benchmade Pardue Design Axis Mini-Griptilian Knife / Case Cutlery 381 Case 316-5 Leather Hunter with Stainless / IIT 35100 Poly Rope Hoist - 2 Ton / Boardwalk Paper KFT3640765 765 Foot Length x 36 Inch / Joe Knows Electronics 5mm Clear Blue LED (25 Pack) HQ / Filmtec TW30-1812-50 TFC membrane / Carson LED Lighed LinenTest 11.5x Focusing Loupe (LT-10) / Rigol DS1052E 50MHz DSO / Neiko Professional Grade 6-Inch Bench Grinder with / OLFA Rotary Cutter RTY-2/G / AGPtek High Capacity Laptop Battery for HP Compaq 2710
One of my sons is not interested in helping me gather up toys. He
helps me for a infinitesimal afterwards moves on to thing other. However,
he loves small indefinite quantity me beside white goods and unloading the dish washer.
My other son has to have everything in the region of him tidy in a
certain way. Once we amass up his toys he makes convinced everything
is picked up and in its locate. One day once we were picking up
the family connections area he went into his bedchamber and picked up the entire
room. Once we saved him in here his breathing space was spotless, and he's
not even 2 time of life old!
Besides pick up toys off the flooring at the end of all day, I
don't compel my boys to do consistent chores, apart from as theyability want
to back me. Here are, however, a couple of chores they
especially savour.
Factory-Reconditioned Porter-Cable FR350AR Round Head / Mavea 1007930 Classic Fit 2-Pack Replacement Filter / 15pc. Assortment Motorcycle ATV Fuel Line Hose tube Tubing / API AC101P4 Mounting Clamps for Truck Caps / Camper Shells / DMT D8C 8-Inch Dia-Sharp Continuous Diamond Coarse / GearWrench 80550 57 Piece 3/8-Inch Drive 6 Point Socket / Opella 205.995.110 Swivel Ball, Chrome / Buck 103 Skinner Fixed Blade Knife / Real Bat in Acrylic Block - Medium / Spyderco Ladybug 3 H-1 Rust Free Serrated Edge Knife, / Kreg Pine Plugs - 200 count / Intellinet Cooling Fan 1123XSL- 115V AC 120mm , SP100A / Milwaukee 48-22-2113 11 In 1 ECX Multi tip / Gold Kapton Tape Polyamide High Temp 1/4" (6mm) x 36yds / WD-40 WD-40 Spray Applicator / Kreg SML-F125-100 1-1/4-Inch 7-Fine Washer-Head Pocket / Tower Manufacturing 30339015 15 amp GFCI 5-Outlet Adapter
I accomplished my boys were old ample to backing next to the household linen when
they started fetching their article of clothing out of the drier and running
into their liberty to put them into their drawers! So now I let
them put their own clothing away, and don't torment yourself about what
drawers theyability put them in. The crucial entity is thatability theyability are
helping and putting their own household linen away. They besides enjoy
putting their vesture into the work contraption. I issue turns
picking each juvenile person up so theyability can descend a number of wear into the
I had a dreadful instance wearisome to bread and butter my toddlers out of my
dishwasher until I accomplished theyability merely longed-for to back transport dishes
out of it. For for a while theyability handed me tableware to put away (I
take all of the heightened knives out archetypal), consequently theyability established that
they hot to put the tableware into the container. They can't
even see into the drawer, but theyability dribble it terminated the edge, into
the drawer, past I put the tableware where on earth it belongs.
I've had a lot of fun belongings my tyke boys backing next to household
chores. They don't see chores as work, theyability cogitate theyability are
hanging out beside and portion mom, which is what is primal to
them, and as theyability get elder theyability will learn thatability home chores
are a factor of home life.


text:
Aishwarya Rai Bachchan is all set to join Karan Johar and his team in Vienna who are eagerly waiting to work with her, in Ae Dil Hai Mushkil.
The actress was spotted at the airport heading for the shoot of her next film. Aishwarya as planned, finished her father-in-law's birthday celebrations and is now headed back to work.
The team of Ae Dil Hai Mushkil, along with the lead actors, Ranbir Kapoor, Anushka Sharma and Fawad Khan had been shooting the film in London, Paris and Vienna. Anushka and Aishwarya will be seen playing the role of two sisters, while Fawad Khan will be essaying the role of a DJ.
Amitabh Bachchan's Granddaughter Navya Naveli To Make Her Grand Debut
Fawad will be shooting for the film for 10 days, while his colleague from Pakistan, Imran Abbas Naqvi has finished shooting for his cameo role.
Karan Johar is very close to Aishwarya and so the director has gone the extra mile to accommodate her daughter, Aaradhya during her entire shooting schedule of the movie.
The actress has already won over the audience with her recently released film, Jazbaa and now in no time, we shall get to see her scorch the silver screen once again for Karan Johar.
Check out the actress getting spotted at the airport with fans...
Ash With Fans
Aishwarya Rai Bachchan was spotted at the airport by a number of fans who shared the pics on social media.
Aishwarya Rai is leaving for Vienna to join Karan Johar and the rest of the Ae Dil Hai Mushkil team.
Selfie With Mom
Aishwarya Rai's selfie with Aaradhya and her mom during the Jazbaa screening in Mumbai.
Ae Dil Hai Mushkil
Karan Johar, Anushka Sharma and Ranbir Kapoor spotted in London shooting for the film.
Jazbaa Promotions
Aishwarya Rai Bachchan along with Sanjay Gupta had promoted Jazbaa movie in Mumbai colleges.