Problem/Motivation

Currently both the JS and CSS preprocessors do not work for all use cases. Since drupal_build_js_cache() preforms the JavaScript preprocessing to roll all of the js files into one file, developers try to alleviate this with other methods such as the sf_cache module.

It has been proposed to make the JS and CSS preprocessors pluggable, possibly with the help of assetic. To be more specific Owen Barton suggested 2 interfaces, a bundler and a packager.

Proposed resolution

  1. We make the JS and CSS preprocessing pluggable with two separate interfaces, the bundler and the packager.
  2. We also integrate assetic into core as the packager.

Remaining tasks

  1. We need to reach a consensus on if we want to actually integrate assetic into core
  2. We also need to create a patch for making the JS and CSS preprocessors pluggable
  3. 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
CommentFileSizeAuthor
#160 pluggable_preprocessing-352951-160.patch205.94 KBWim Leers
#160 interdiff.txt3.88 KBWim Leers
#151 pluggable_preprocessing-352951-151.patch205.42 KBJohnAlbin
#148 pluggable_preprocessing-352951-148.patch206.73 KBWim Leers
#148 interdiff-fixes.txt3.31 KBWim Leers
#148 interdiff-noop_js_optimizer.txt4.68 KBWim Leers
#140 pluggable_preprocessing-352951-140.patch202.45 KBsdboyer
#140 interdiff.txt7.98 KBsdboyer
#138 pluggable_preprocessing-352951-138.patch330.87 KBsdboyer
#133 pluggable_preprocessing-352951-133.patch202.33 KBWim Leers
#133 interdiff.txt20.57 KBWim Leers
#131 pluggable_preprocessing-352951-131.patch201.67 KBWim Leers
#131 interdiff.txt38.15 KBWim Leers
#122 pluggable_preprocessing-352951-122.patch171.26 KBWim Leers
#120 pluggable-assets-352951.120.interdiff.txt680 byteslarowlan
#120 pluggable-assets-352951.120.patch169.49 KBlarowlan
#118 pluggable-assets-352951.118.interdiff.txt781 byteslarowlan
#118 pluggable-assets-352951.118.patch169.43 KBlarowlan
#116 pluggable-assets-352951.111.patch169.31 KBlarowlan
#109 pluggable-assets-352951.patch169.31 KBlarowlan
#107 pluggable_preprocessing-352951-107.patch171.37 KBWim Leers
#107 interdiff.txt90.08 KBWim Leers
#96 pluggable_preprocessing-352951-96.patch95.92 KBWim Leers
#85 pluggable-asset-management-352951-85.patch9.83 KBmcjim
#81 352951-js.patch14.27 KBOwen Barton
#81 352951-js-branch-do-not-test.patch7.9 KBOwen Barton
#76 pluggable-asset-management-352951-76.patch9.89 KBmcjim
#74 pluggable-asset-management-352951-74.patch8.6 KBmcjim
#71 pluggable-asset-management-352951-70.patch8.6 KBmcjim
#68 pluggable-asset-management-352951-68.patch7.98 KBmcjim
#66 Drupal-8-Asset-Workflow.jpg94.23 KBmcjim
#66 pluggable-asset-management-352951-66-do-not-test.patch9.27 KBmcjim
#25 352951-25.patch7.81 KBWim Leers
#23 plug-js-preprocess.patch7.47 KBmfer
#18 pluggable_js_preprocess-352951-18.patch12.07 KBmfer
#16 pluggable_js_preprocess-352951-16.patch10.52 KBmfer
#15 352951.patch9.95 KBRobLoach
#14 352951.patch9.5 KBRobLoach
#11 352951.patch9.57 KBRobLoach
#4 pluggable_js_preprocess-352951-4.patch6.38 KBmfer
#2 pluggable_js_preprocess-352951-2.patch6.31 KBmfer
#1 pluggable_js_preprocess-352951-1.patch6.29 KBmfer
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mfer’s picture

Status: Active » Needs review
FileSize
6.29 KB

Here is my first pass at a pluggable JavaScript preprocessor.

mfer’s picture

Updated the interface name to JSPreprocessingInterface for consistency and fixed some comments.

drewish’s picture

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);
mfer’s picture

Thanks for the feedback, drewish. Updated.

Status: Needs review » Needs work

The last submitted patch failed testing.

mfer’s picture

Status: Needs work » Needs review

Previous patch still applies with no issues for me.

moshe weitzman’s picture

Any chance you can name preprocessing to something else? The theme system has kinda taken over that name.

mfer’s picture

@moshe weitzman - I'm open to a name change. Drupal 6 already uses preprocess when refering to JavaScript so it would be a name change from an existing convention. Though, this topic is out of the scope of this patch which is just to take an existing system (with an existing name) and make it pluggable. It's primarily a re-factoring of code.

RobLoach’s picture

Status: Needs review » Needs work
Issue tags: +Patch Spotlight

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....

RobLoach’s picture

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:

array(
  '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.

RobLoach’s picture

FileSize
9.57 KB

....... Forgot the patch.

moshe weitzman’s picture

@rob - thats D6 style code. We no longer have to specify files, with the code registry.

mfer’s picture

Why are we telling it what file to load? The registry takes care of loading files for classes and interfaces. If it's in the includes folder it just finds it. If it's a module you define what module files to scan. See http://drupal.org/node/224333#registry and http://api.drupal.org/api/group/registry/7.

RobLoach’s picture

FileSize
9.5 KB

Fixed the tests, but am having troubles with the registry. Assistance would be nice.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
9.95 KB

Ahhh, had to add common.test to the registry.

mfer’s picture

I updated the test to remember the configured class for the JS Preprocessing and restore it after the test.

2 Questions:

  • Is it appropriate for tests/common.test to be listed in simpletest.info?
  • 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?
RobLoach’s picture

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.

mfer’s picture

The attached patch moves including common.test to a javascript_test module in the tests folder. The javascript_test.info file includes the module. The javascript_test.module file is a dummy file. If we change core to not require module files this can go away. The setup function for the javascript tests enables this module.

RobLoach’s picture

Status: 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...

Wim Leers’s picture

Great work! The same should be applied to CSS preprocessing.

Subscribing.

sun’s picture

Issue tags: +JavaScript
andreiashu’s picture

nice, subscribing

mfer’s picture

Status: Postponed » Needs review
FileSize
7.47 KB

I don't think the plugins/handlers system will make it into D7. So, here is an update to the previous series of patches that store the class in a variable.

This is the overall layout of the functionality but the tests could use some work.

Status: Needs review » Needs work

The last submitted patch failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.81 KB

Rerolled the patch. Didn't test it, just a straight re-roll.

Status: Needs review » Needs work

The last submitted patch failed testing.

SeanBannister’s picture

Sub

sun’s picture

Version: 7.x-dev » 8.x-dev
catch’s picture

subscribing.

threading_signals’s picture

Subscribing

Owen Barton’s picture

Title: Make JS Preprocessing Pluggable » Make JS & CSS Preprocessing Pluggable

Agree with #20 that CSS should be included, so updating title

dvessel’s picture

subscribing

mikey_p’s picture

Subscribing.

rupl’s picture

Subscribing.

RobLoach’s picture

+++ 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.

skottler’s picture

Subscribe

ericduran’s picture

sub.

RobLoach’s picture

Not sure where I found this, but it definitely looks awesome: https://github.com/kriswallsmith/assetic

catch’s picture

Category: feature » task
Priority: Normal » 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.

Also issues like #865536: drupal_add_js() is missing the 'browsers' option and the very different alters I had to do in http://drupal.org/project/agrcache show that our css and js handling are woefully out of sync. So bumping priority of this. Adding comments to JavaScript files should not be an (admittedly small) performance regression.

ericduran’s picture

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.

mfer’s picture

@Rob Loach it doesn't look like Assetic does aggregation of files. Is this the case?

nod_’s picture

This can finally go somewhere: #1497366: Introduce Plugin System to Core go nuts people! :)

RobLoach’s picture

@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.

mfer’s picture

@Rob Loach I see that. What do you think of this for a path forward.

  1. We make the current implementation pluggable. We use both the current implementation and Assetic as a basis to create the API. This way other systems could be used in contrib (e.g., advagg).
  2. We try to get Assetic in core.

If this sounds good we can start coding.

nod_’s picture

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:

drupal_add_js
drupal_get_js
drupal_sort_css_js
drupal_js_defaults
drupal_group_js
drupal_aggregate_js
drupal_build_js_cache
drupal_clear_js_cache

_drupal_flush_css_js

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?

Owen Barton’s picture

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.

JohnAlbin’s picture

Issue tags: +frontend performance

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. :-(

Wim Leers’s picture

#46 is spot-on.

ZenDoodles’s picture

Tagging for issue summary

catch’s picture

Yep, completely agreed with #46, they're two separate systems and should both be pluggable.

Zgear’s picture

ah forgot to remove the issue summary initiative... done!

klonos’s picture

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.

klonos’s picture

Issue summary: View changes

The issue required an issue summary to be moved forward

klonos’s picture

Issue summary: View changes

...made instances of "assetic" a link to the github project: https://github.com/kriswallsmith/assetic

cosmicdreams’s picture

If we're voting +1 for assetic

effulgentsia’s picture

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

nod_’s picture

nod_’s picture

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.

cosmicdreams’s picture

Hey, nod_, If I can learn enough of the plugin system I want to help with this.

nod_’s picture

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 :)

Owen Barton’s picture

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?

cosmicdreams’s picture

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.

neclimdul’s picture

Issue tags: +Plugin system

Since we're interested in using the plugin system, tagging for aggregation.

neclimdul’s picture

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 :)

Kiphaas7’s picture

Very interested in this. If I can help, I will.

nod_’s picture

Trying out an implementation with Assetic #1751602: Use Assetic to package JS and CSS files help welcome :)

Owen Barton’s picture

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!).

mcjim’s picture

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.

Asset flow

Kiphaas7’s picture

Awesome, keep up the good work!

mcjim’s picture

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.

mcjim’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, pluggable-asset-management-352951-68.patch, failed testing.

mcjim’s picture

Status: Needs work » Needs review
FileSize
8.6 KB

Registered the bundle in install.core.inc
Hopefully will work…

Status: Needs review » Needs work

The last submitted patch, pluggable-asset-management-352951-70.patch, failed testing.

mcjim’s picture

Looks like it's the call to drupal_get_css() in ajax_render() which is causing the fails.

mcjim’s picture

Status: Needs work » Needs review
FileSize
8.6 KB

Small amends to keep up-to-date with core and to (hopefully) fix the ajax_render() fails.

Status: Needs review » Needs work

The last submitted patch, pluggable-asset-management-352951-74.patch, failed testing.

mcjim’s picture

Status: Needs work » Needs review
FileSize
9.89 KB

Re-roll.
update.php and authorize.php weren't aware they needed to know about the css_asset_manager class, so I told them.
Fingers crossed.

mcjim’s picture

Status: 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.

kscheirer’s picture

Assetic was added to Drupal core here: #1784774: Remove Assetic component from core

RobLoach’s picture

Issue tags: +Assetic
mcjim’s picture

Owen Barton’s picture

Status: Needs work » Needs review
FileSize
7.9 KB
14.27 KB

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.

neclimdul’s picture

I don't think the JsAssetManager class is in the patch.

attiks’s picture

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

Status: Needs review » Needs work

The last submitted patch, 352951-js.patch, failed testing.

mcjim’s picture

Status: Needs work » Needs review
FileSize
9.83 KB

Re-roll of patch in 76 to keep up with HEAD.
Now looking at a re-roll of 81.

cweagans’s picture

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.

Owen Barton’s picture

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.

hass’s picture

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.

YesCT’s picture

Status: Needs review » Needs work
Issue tags: +JavaScript, +Patch Spotlight, +frontend performance, +Plugin system, +Assetic

The last submitted patch, pluggable-asset-management-352951-85.patch, failed testing.

YesCT’s picture

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Issue tags: -JavaScript, -Patch Spotlight, -Assetic

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…

Owen Barton’s picture

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.

Wim Leers’s picture

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:

  1. Only refactor CSS aggregation, to get feedback on that before applying essentially the exact same pattern to JS aggregation.
  2. Ensure that the different phases are completely PHPUnit testable.
  3. 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.

cosmicdreams’s picture

excellent news!

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
95.92 KB

The proposed structure:

  • AssetCollectionOptimizerInterface: optimizes the whole collection of assets, returns a (in our case: smaller because aggregated) set of assets
    1. 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)
    2. AssetOptimizerInterface: used/called by AssetCollectionOptimizerInterface, optimizes an individual asset, returns optimized data as string
    3. 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 CssCollectionRenderer just 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!

sdboyer’s picture

YAAAAAAAAAAAAAAAAAAY

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

s/inclussion/inclusion/

+++ 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.

Status: Needs review » Needs work

The last submitted patch, pluggable_preprocessing-352951-96.patch, failed testing.

Wim Leers’s picture

#97:

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)

Exactly.

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…

catch’s picture

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.

sdboyer’s picture

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.

catch’s picture

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.

Wim Leers’s picture

#101:

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 :)

Owen Barton’s picture

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.

sdboyer’s picture

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.

pounard’s picture

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!

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
90.08 KB
171.37 KB

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 CssOptimizerUnitTest!

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?

Status: Needs review » Needs work

The last submitted patch, pluggable_preprocessing-352951-107.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
169.31 KB
sdboyer’s picture

Status: Needs review » Needs work
Issue tags: +happy princess API

tagging

Wim Leers’s picture

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?

catch’s picture

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.

Wim Leers’s picture

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.

catch’s picture

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).

sdboyer’s picture

@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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
169.31 KB

Chases head.
Neither of those fails occur locally.

Status: Needs review » Needs work

The last submitted patch, pluggable-assets-352951.111.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
169.43 KB
781 bytes

So some strict warnings when browsers isn't set.
Hopefully green

Status: Needs review » Needs work

The last submitted patch, pluggable-assets-352951.118.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
169.49 KB
680 bytes

This time?

Status: Needs review » Needs work

The last submitted patch, pluggable-assets-352951.120.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
171.26 KB

#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.)

Status: Needs review » Needs work
Issue tags: -frontend performance, -Plugin system, -happy princess API

The last submitted patch, pluggable_preprocessing-352951-122.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +frontend performance, +Plugin system, +happy princess API

The last submitted patch, pluggable_preprocessing-352951-122.patch, failed testing.

Wim Leers’s picture

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.

Wim Leers’s picture

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.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -frontend performance, -Plugin system, -happy princess API

Status: Needs review » Needs work
Issue tags: +frontend performance, +Plugin system, +happy princess API

The last submitted patch, pluggable_preprocessing-352951-122.patch, failed testing.

Wim Leers’s picture

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
38.15 KB
201.67 KB

And voila, part two: pluggable JS aggregation!

Changes:

  • New: JsCollectionGrouper, JsCollectionOptimizer and JsCollectionRenderer.
  • Renamed CssDumper to AssetDumper (more generic), because 99% of the code was identical for the two.
  • No unit tests for JS, because JavaScriptTest actually 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.

larowlan’s picture

Status: 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 browsers for which the JavaScript item needs to be loaded is part of
+      // 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');

$this->container->get('config.factory')->get('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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
20.57 KB
202.33 KB

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.

dawehner’s picture

+++ 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...

Wim Leers’s picture

#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.

sdboyer’s picture

Status: 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.

sdboyer’s picture

Assigned: Wim Leers » sdboyer

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.

sdboyer’s picture

Status: Needs work » Needs review
FileSize
330.87 KB

first, just a quick reroll to catch up with #2026255: Make CascadingStylesheetsTest 3500% faster. no interdiff b/c the old patch no longer applied.

sdboyer’s picture

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.

sdboyer’s picture

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:


EXPECTED:
<style media="all">
/* <![CDATA[ */
body{padding:0px;}

/* ]]> */
</style>

 ACTUAL:
<style media="all">
/* <![CDATA[ */
body { padding: 0px; }
/* ]]> */
</style>

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.

Status: Needs review » Needs work

The last submitted patch, pluggable_preprocessing-352951-140.patch, failed testing.

Wim Leers’s picture

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?

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -frontend performance, -Plugin system, -happy princess API

Status: Needs review » Needs work
Issue tags: +frontend performance, +Plugin system, +happy princess API

The last submitted patch, pluggable_preprocessing-352951-140.patch, failed testing.

sdboyer’s picture

@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?

sdboyer’s picture

Status: Needs work » Needs review
Issue tags: -frontend performance, -Plugin system, -happy princess API

Status: Needs review » Needs work
Issue tags: +frontend performance, +Plugin system, +happy princess API

The last submitted patch, pluggable_preprocessing-352951-140.patch, failed testing.

Wim Leers’s picture

Component: javascript » base system
Assigned: sdboyer » Wim Leers
Status: Needs work » Needs review
FileSize
4.68 KB
3.31 KB
206.73 KB

#145:

RE: AssetDumper: D'oh, of course, that makes total sense.

I don't see the point of having a no-op JsOptimizer. Except if … you mean that by having a no-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.
See interdiff-noop_js_optimizer.txt.


I also searched and found the cause for the patch causing the installation to fail. The installer also relies on common.inc to render its CSS and consequently as of #140 it depends on the container. But the full container is not built for the installer, only truly necessary parts are registered. So I registered the CSS/JS renderers, and poof, problem solved :)
I also found two other bugs that were introduced in #140 and fixed those ($this->state; being injected, but still being called like $this->state(), which doesn't work; and … the optimized CSS/JS assets no longer being used at all :P).
See interdiff-fixes.txt.


With all that out of the way, this patch should now be be RTBC'able… :)

sdboyer’s picture

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!

JohnAlbin’s picture

Status: Needs review » Needs work
new file:   core/modules/system/lib/Drupal/system/Tests/Common/CascadingStylesheetsTest.php.rej

lol.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
205.42 KB

I've edited the patch by hand to remove that one hunk. That way I can continue my review while the testbot works.

sdboyer’s picture

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...

Gábor Hojtsy’s picture

Priority: Major » 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.

JohnAlbin’s picture

Priority: Critical » Major
Status: 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>
<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>
<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>
<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>
<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>
<style media="print">
@import url("http://drupal8x.dev/core/themes/bartik/css/print.css?mp9lyp");
</style>

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!

JohnAlbin’s picture

Priority: Major » Critical

Cross-posted with Gabor.

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

catch’s picture

Status: 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?

Wim Leers’s picture

Status: 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.

catch’s picture

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.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.88 KB
205.94 KB

#159: done.

catch’s picture

Title: Make JS & CSS Preprocessing Pluggable » Change notice: Make JS & CSS Preprocessing Pluggable
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Thanks!

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.

Wim Leers’s picture

Title: Change notice: Make JS & CSS Preprocessing Pluggable » Make JS & CSS Preprocessing Pluggable
Status: Active » Fixed
Issue tags: -Needs change record
Wim Leers’s picture

hass’s picture

ParisLiakos’s picture

+++ 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?

Wim Leers’s picture

#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! :)

ParisLiakos’s picture

Status: Fixed » Closed (fixed)
Issue tags: -frontend performance, -Plugin system, -happy princess API, -RTBC July 1

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

grendzy’s picture

Cross-linking: This commit caused a regression, by removing the fix from #1961340: CSS aggregation breaks file URL rewriting because it abuses it.