Problem/Motivation

(Work on this is being coordinated in the 1762204-assets branch in sdboyer's github fork)

The goal of this issue is to introduce a new base architecture for declaring and managing assets (CSS/JS), via a set of our own Assetic-derived classes. The architecture should, once completed, be able to completely replace the drupal_add_css/js/library() system, which is already scoped for deletion:

This change is necessary because the current system is reliant on globals (drupal_add_css/js()), which breaks page rendering encapsulation and can easily be cache-busty.

There are also some nice-to-haves that we can do with a proper approach to this problem.

  • Develop an asset grouping and aggregation strategy that isn't predestined to blow chunks. And is easily swapped out by contrib. (cf. #352951: Make JS & CSS Preprocessing Pluggable)
  • Utilize Assetic to help with the above. Yay! (#1751602: Use Assetic to package JS and CSS files)
  • Allow the declaration of conditional assets (e.g., 'always give X css to users in Y role') in a first-class, introspectable fashion. (NOT in scope for the initial patch)
  • Give us a clear point of control and reflection for, say, pushing javscript down into the footer.

This is a major hurdle on the path towards the goals of SCOTCH.

Proposed resolution

This patch slips in almost transparently underneath the existing #attached mechanism for declaring assets - meaning, almost NO API CHANGE.. More on the sole API change below.

We need a whole new approach to the declaration, ordering, aggregating, and final placement of assets into templates. Out of the box, Assetic can help with certain mechanics of manipulating asset content: minification, SASS compilation (though making that work correctly is NOT in scope for this patch), but it missing necessary things needed at declaration, and is totally orthogonal to sorting. So, the first major component of this patch is constructing a series of interfaces and classes on top of the base API provided by Assetic in order to accommodate our added requirements.

To assist with declaration, this patch introduces our own family of classes for representing the individual assets themselves. Here's an overview of the main concepts:

  • Asset - an individual asset, representing a local file, external file, or simply a string. There are classes representing each of those three, which share a base class, BaseAsset. These implement our AssetInterface, which is an extension of Assetic's AssetInterface.
  • Asset Metadata - these are classes that contain the sort of metadata we've historically set on assets - think every_page, browsers, etc. They're injected into AssetInterface objects, which need them to function, but they're kept in a separate class in order to keep AssetInterface's responsibility narrowly focused on representing, loading, dumping, etc., the asset itself.
  • Asset Collection - a container that holds assets (that is, things implementing our AssetInterface). These implement our AssetCollectionInterface, and have the sort of methods you'd expect from a collection. They do NOT represent a logical asset that is renderable; they are just a container. Drupal's libraries have always been this way - they contain a list of assets, but are not themselves an asset. Indeed, the new AssetLibrary class implements AssetCollectionInterface.
  • Asset Aggregates - these are also a container for AssetInterface objects, and have some collection-style methods, but unlike collections, they themselves implement AssetInterface, meaning that they represent a logical asset and ARE a renderable unit. So, naturally, they can contain only one type (css or javascript) of asset - unlike collections, which can contain both.

Note that the distinction between aggregates and collections is not one made by Assetic. It has only an AssetCollection, which is both a generalized container (like our collections) as well as a renderable (like our aggregates).

The other major player in this system is the AssetCollector. Think of it like a factory on wheels: its job is to travel around and create asset objects and inject them into an AssetCollectionInterface object that it holds. This lets us send the factory into code that we want to have declare assets, but prevents that code from having full access to the collection, which would allow it to change or remove assets already in the collection - not always appropriate.

The weight metadata on assets is going away. This is the sole API change mentioned earlier. This is intentional: reliance on a linear weight ordering is one of the primary reasons Drupal's aggregates suck. The new system replaces it with a first-class way of handling ordering. There are two mechanisms: assets can declare dependencies on libraries - conceptually, the same as now. Assets can also declare that they come before() or after() another asset (not a library - an individual asset); unlike a dependency, this does not guarantee that the other asset will be present, BUT IF it is, then that ordering is guaranteed. This explicit ordering information lets us use a graph to find an ordering that allows for a minimal, stable set of aggregates - something we could never do with a linear weight order.

Now, gluing it all together. For this patch, drupal_process_attached() uses an AssetCollector to transform all array-style assets into their object equivalents, and stores them in an AssetCollection. This still relies on drupal_static(), so a global, but that can go away with some other patches that are being developed.

When it comes time to render out the HTML for the assets, we're quickly into the stack in introduced in #352951: Make JS & CSS Preprocessing Pluggable - the set of services for aggregating, optimizing, dumping, and rendering assets. Pretty much all of those are changing to both take and return an AssetCollection. There is some rejiggering of things - the responsibility for doing the sort is encompassed by AssetGroupSorterInterface, which groupers (now known as aggregators) now call. Aggregators now produce an AssetCollection mostly full of AssetAggregates. The collection optimizer iterates over that, uses Assetic's mechanics for filtering, then passes them off to the dumper to write to disk.

Remaining tasks

Still a fair bit.

  • Grunt work: adding brief docblocks to all of the new unit tests.
  • In doing on the above, any plain Exceptions that are thrown should be converted to an appropriate more specific type. (some progress made)
  • FileAsset, StringAsset and ExternalAsset need unit tests.
  • ExternalAsset needs love. Right now it just throws exceptions if its load() or getLastModified() methods are called, but this is wrong - it needs to work, even if core doesn't use it.
  • Figure out a sane basis for StringAsset to return a value from getLastModified().
  • Deal with uniqueness in collections. That entails:
  • Make interfaces fluent, where possible and appropriate.
  • Dependency resolution needs to be handled. This is big, and probably involves reviving the AssetLibraryRepository.
  • AssetLibrary needs unit tests.
  • AssetLibrary maybe also needs some reconsideration of its metadata-as-exposed-get/set-methods interface. (followup?)
  • We should have a look at serialization of collections, aggregates, etc. to see how not-breaky we can make it. (followup?)
  • AssetAggregate has no test coverage; everything there needs to be written. load() and dump() can be skipped in a first pass, they are tricky.
  • AssetCollector needs an interface, and many of its methods still need to be documented.
  • AssetMetadataBag needs an interface.
  • AssetMetadataBag also really needs to be refactored. It was originally written to support a misunderstood use case - there's no need to keep track of "explicit" versus "default" values. The basic set of public methods on it are still probably useful, but the internals should be refactored to keep track of just a single array, rather than constantly coalescing between the two.
  • A factory should be introduced for making AssetMetadataBags, and AssetCollector should probably use it instead of directly calling new CssMetadataBag().
  • Some thought needs to be given to the different types of metadata we track for assets, with specific mind to how "intrinsic" the datum actually is to the asset. This could factor significantly in contrib efforts to do sitewide optimal asset groupings. (definite followup)
  • Refactor drupal_process_attached() to utilize an AssetCollector as described in the summary.
  • Make a subclass of AssetCollection that behaves like a priority queue using natural case sort; using this as the de facto collection everywhere should improve group ordering reliability. (Actually implemented by just adding sort methods to AssetCollectionInterface)
  • All of the JS-related classes from #352951: Make JS & CSS Preprocessing Pluggable need to be reimplemented with the new system.
    • Rewrite JsCollectionOptimizer. And tests.
    • Rewrite JsOptimizer. And tests.
    • Rewrite JsDumper. And tests.
    • Refactor JsCollectionGrouper into JsCollectionAggregator. And tests.
    • Write a JsGraphSorter. And tests.
  • Figure out how to separate out the two channels for header vs. footer js.
  • All of the CSS-related classes from #352951: Make JS & CSS Preprocessing Pluggable, except the grouper, need to reimplemented with the new system.
    • Rewrite CssCollectionOptimizer. And tests.
    • Rewrite CssOptimizer. And tests.
    • Rewrite CssDumper. And tests.
    • Refactor CssCollectionGrouper into CssCollectionAggregator. And tests.
    • Write a CssGraphSorter. And tests.
  • In doing the previous two items, we need to decide just how much we actually rely on Assetic's load() and dump() methods. We may let the behavior stay in there, but it's also an SRP violation, and can make for some awkward coupling.
  • Graph sorting needs to be moved out of CssCollectionGrouperNouveaux and into a standalone service, which the grouping classes utilize, but is also called directly when aggregation is disabled to prepare for rendering. Gotta sort there, too. (still needs a little tweaking, though)
  • Graph sorting needs functional tests. The AssetGraph has unit tests, but we need black box tests that ensure the expected output, given dependencies, is correct.
  • We need a new mechanism for injecting the rendered tags into the page - since we need to get rid of the global getters, too, template_preprocess_html() can no longer call the globals to grab it. They need to be injected as variables to the theme function, instead. Maybe they needn't even pass through drupal_render() at all...
  • Remove drupal_add_library().
  • Remove drupal_add_js().
  • Remove drupal_get_js().
  • Remove drupal_add_css().
  • Remove drupal_get_css().

Note - sdboyer reserves the right to pull the trigger on the last five. he's earned it :)

Further note - for now, we're punting on a thorny issue around supporting css/js preprocessors (so for SASS, LESS, CoffeeScript, etc.). It's not impossible, but it complicates the API a bit, has significant issues around best practices & third-party code, and we can readily do it later.

User interface changes

None.

API changes

None, except that weight doesn't do anything anymore on assets, and should not be used.

Files: 
CommentFileSizeAuthor
#176 assets-1762204-176.patch290.63 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-176.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#176 interdiff.txt68.24 KBsdboyer
#171 assets-1762204-171.patch243.11 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Component/ObjectState/FreezableTrait.php.
[ View ]
#161 interdiff.txt10.42 KBsdboyer
#161 assets-1762204-161.patch245.77 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#154 interdiff.txt12.27 KBsdboyer
#154 assets-1762204-154.patch243.84 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#152 interdiff.txt39.79 KBWim Leers
#152 assets-1762204-173.patch247.01 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-173.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#150 interdiff.txt52.65 KBsdboyer
#150 assets-1762204-150.patch244.24 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 58,329 pass(es), 266 fail(s), and 8 exception(s).
[ View ]
#141 assets-1762204-141.patch232.73 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 58,689 pass(es), 267 fail(s), and 8 exception(s).
[ View ]
#141 interdiff.txt1.38 KBsdboyer
#138 assets-1762204-138.patch231.35 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#138 interdiff.txt12.45 KBsdboyer
#138 broke-ass-css.png67.96 KBsdboyer
#135 assets-1762204-135.patch224.49 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#133 assets-1762204-133.patch224.42 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-133.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#129 interdiff.txt8.52 KBsdboyer
#129 assets-1762204-129.patch197.2 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 60,016 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#121 assets-1762204-121.patch197.24 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 59,170 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#121 interdiff.txt24.5 KBsdboyer
#119 assets-1762204-119.patch175.49 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 59,484 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#119 interdiff.txt93.29 KBsdboyer
#117 interdiff.txt24.19 KBsdboyer
#117 assets-1762204-117.patch157.06 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 59,583 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#114 interdiff.txt135.75 KBsdboyer
#114 assets-1762204-114.patch146.31 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 59,136 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#111 assets-1762204-111.patch197.19 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-111.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#111 interdiff.txt29.62 KBsdboyer
#109 assets-1762204-109.patch204.23 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-109.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#109 interdiff.txt38.81 KBsdboyer
#107 assets-1762204-107.patch197.98 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 58,836 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#107 interdiff.txt7.17 KBsdboyer
#106 assets-1762204-106.patch195.21 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 59,106 pass(es).
[ View ]
#106 interdiff.txt64.38 KBsdboyer
#105 assets-1762204-105.patch135.54 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 59,112 pass(es).
[ View ]
#105 interdiff.txt799 bytessdboyer
#99 assets-1762204-99.patch135.54 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Asset/CssCollectionOptimizerNouveaux.php.
[ View ]
#99 interdiff.txt25.57 KBsdboyer
#98 assets-1762204-98.patch130.95 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 59,044 pass(es).
[ View ]
#98 interdiff.txt24.05 KBsdboyer
#97 assets-1762204-97.patch122.5 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 58,657 pass(es).
[ View ]
#95 interdiff.txt105.3 KBsdboyer
#95 assets-1762204-95.patch191.37 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 58,793 pass(es).
[ View ]
#91 assets-1762204-91.patch188.78 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 58,723 pass(es).
[ View ]
#88 assets-1762204-88.patch183.75 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#88 interdiff.txt56.5 KBsdboyer
#86 interdiff.txt11.54 KBsdboyer
#86 assets-1762204-86.patch194.52 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 58,347 pass(es).
[ View ]
#84 assets-1762204-84.patch184.09 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 58,215 pass(es), 360 fail(s), and 9 exception(s).
[ View ]
#84 interdiff.txt1.18 KBsdboyer
#79 assets-1762204-79.patch184.04 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#77 assets-1762204-77.patch184.11 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-77.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#77 interdiff.txt41.32 KBsdboyer
#71 assets-1762204-71.patch179.43 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#68 interdiff.txt11.89 KBsdboyer
#68 assets-1762204-68.patch66.69 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 56,294 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#63 assets-1762204-63.patch72.14 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 56,525 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#63 interdiff.txt14.11 KBWim Leers
#60 interdiff.txt10.06 KBsdboyer
#60 assets-1762204-60.patch70.27 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 56,533 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#57 interdiff.txt3.26 KBsdboyer
#57 assets-1762204-57.patch67.98 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 56,561 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#54 assets-1762204-52.patch67.71 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#37 1762204-core_assets-20-35.interdiff.txt62.33 KBcam8001
#35 assets-1762204-201302270802.patch26.23 KBlunaris
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-201302270802.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#30 core_assets-1762204-29.patch84.08 KBclemens.tolboom
FAILED: [[SimpleTest]]: [MySQL] 49,945 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#29 interdiff-1762204-20-29.txt17.15 KBclemens.tolboom
#29 interdiff-1762204-20-29.txt17.15 KBclemens.tolboom
#20 interdiff.txt11.22 KBsdboyer
#20 core_assets-1762204-20.patch47.24 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 50,830 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#17 core-assets-1762204-17.patch41.91 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 50,718 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#15 interdiff.txt613 bytessdboyer
#15 assets-1762204-15.patch41.97 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 assets-1762204-14.patch42.74 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 50,699 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#14 interdiff.txt16.06 KBWim Leers
#13 assets-1762204-13.patch41.25 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 50,685 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#10 assets-1762204-10.patch41.27 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Issue tags:+API change, +API clean-up

tag

Title:API Clean up: drupal_add* functionsMake drupal_add_*() functions internal/private

removing all the aggregation steps and complicated setup currently in core

Sorry I don't think this is an option. It's a few months since I reviewed Assetic, and it looks like it has a nice API for preprocessing files and allows them to be stitched together - which would be better than the crappy regexp for CSS, or the simple concat we currently have for JavaScript (and would allow things to be pluggable without having to re-implement tonnes of logic).

However Assetic, unless I completely missed something, doesn't help at all for determining which files to bundle together, which is where the vast majority of our complex logic is around. That logic is there for a reason to allow files to be aggregated at all, while also preventing the Drupal 6 situation of a different huge aggregate being served on almost any page. Making both bundling and preprocessing independently pluggable would be great, but we can't just rip out bundling and then 'replace' it with Assetic which doesn't actually tackle that problem at all.

That's true, then again, we can make Assetic plugins with the code we have right now. That'll be just moving things around to a better place.

+1 on moving the bundling logic into a swappable/extendable function/class/assetic plugin. That should mean both bundling and preprocessing becomes pluggable, right?

Having everything being registered will help us to make a per site basis library usage and open the way of static/global aggregation of almost site wide used bundles, reducing drastically number of parallel http requests and leveraging to the maximum the client cache. +1 on making this pluggable, because what I told just before is something I want to explore, but that a lot of people don't.

Priority:Normal» Major

Bumping since it's that important.

Priority:Major» Normal
Issue tags:+Needs issue summary update

I fail to see how this is a blocker, can you expand the issue summary?
What about adding settings via JS?

Title:Make drupal_add_*() functions internal/privateMake drupal_add_css/js() functions internal/private

StatusFileSize
new41.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

ok, per #1871596-22: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox], i'm hijacking this issue to focus specifically on the assets architecture required to kill drupal_add_css/js().

up front note - this patch is maybe 50% of what it needs to eventually be, but i think it's figured out the hard parts of how to structure this whole thing so that everything can work out well.

note also that i've got a branch for this in the SCOTCH repo, called 1762204-assets.

this patch introduces a new Assetic-derived set of classes to encompass all of our different types of assets, as well as some containers for managing them. the basic reason for taking this approach is that Assetic is not designed to handle the case where assets are added NOT necessarily in the order in which they should be placed onto the page (nor is it particularly graceful about handling a mix of CSS and JS assets). however, the data we need to collect about assets is complex enough that we might as well just represent them with classes. and by representing them with classes that reuse Assetic's interfaces, we get to harvest the data we need but be immediately compatible with just about everything Assetic does, too.

the asset class family is:

  • Drupal\Core\Asset\AssetInterface - extends Assetic's AssetInterface with some additional methods for handling the metadata we need.
  • D\C\A\BaseAsset - extends Assetic's BaseAsset, and is a handy abstract base for all our asset classes.
  • D\C\A\BaseFileAsset - extends D\C\A\BaseAsset and copies logic from Assetic's FileAsset to serve as the abstract base class for all assets that are accessible via the local filesystem/a stream wrapper.
  • D\C\A\BaseExternalAsset - extends D\C\A\BaseAsset and copies logic from Assetic's HttpAsset to serve as the abstract base class for all externally-hosted assets.
  • D\C\A\BaseStringAsset - extends D\C\A\BaseAsset and copies logic from Assetic's StringAsset to serve as the abstract base class for all inline assets.
  • D\C\A\JavascriptAssetInterface - defines methods specific to Javascript assets.
  • D\C\A\StylesheetAssetInterface - defines methods specific to CSS assets.
  • D\C\A\JavascriptFileAsset, D\C\A\JavascriptExternalAsset, D\C\A\JavascriptStringAsset, D\C\A\StylesheetFileAsset, D\C\A\StylesheetExternalAsset, D\C\A\StylesheetStringAsset - all concrete assets, and are pretty much self-descriptive.

then, there's the container for these assets to conveniently pass them around - D\C\A\AssetBag, which implements D\C\A\AssetBagInterface. this is really the big differentiation point between Assetic and what we need, as none of their asset-group-managing classes (Collections, Managers, Factories) are designed to easily build up a list of overall assets, then massage them into correct order. that's what AssetBags are about.

finally, there's the least-worked-on part of this - D\C\A\AssetLibrary, alongside D\C\A\AssetLibraryReference and D\C\A\AssetLibraryManager. these are, collectively, the replacement for our current "libraries" system - they represent a versioned bundle of assets which can be declared as a dependency by other assets. D\C\A\AssetLibrary is the class that contains the individual assets in the library itself (which, of course, are all instances of things in our asset family tree, described above); D\C\A\AssetLibraryManager is a DIC service that keeps track of everything (essentially, the new repository for hook_library_info()), and D\C\A\AssetLibraryReference is the class that you create and attach to an asset you're declaring when you want that asset to depend on a particular library.

there's one test in there - AssetAssemblyTest - that should give a basic sense of how this is all supposed to be used. there's still QUITE a lot more to be added there, but i feel like it's enough that someone else can look at the test and begin to see where i'm going with it.

to get a sense of how this all fits together, read the issue summary (the bits on assets) of #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox]. i'll be moving the relevant bits of it over here presently (along with itemized TODOs - there are definitely still lots). the basic idea, though, is that we create AssetBags at various points in the request process, pass them back up the stack, and then add several compilation/aggregation stages as part of the main kernel request/response process.

Title:Make drupal_add_css/js() functions internal/privateIntroduce new asset declaration architecture to facilitate removal of global drupal_add_js/css()

better title, since i'm hijacking.

Status:Active» Needs review

StatusFileSize
new41.25 KB
FAILED: [[SimpleTest]]: [MySQL] 50,685 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Upon applying locally, I noticed that it doesn't apply anymore. A small bit has changed in CoreBundle.php. Otherwise identical.

StatusFileSize
new16.06 KB
new42.74 KB
FAILED: [[SimpleTest]]: [MySQL] 50,699 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

I like what I read in #10, but without it actually working, it's of course just a nice story :)


It was interesting to see that you've changed the default scope of JS files from header to footer. I applaud that, but it may require changes to some core JS as well (since some may rely on header being the default).

I was mostly wondering how this could possibly work:

+  public function testSortingAndDependencyResolution() {
+    $bag = new AssetBag();
+
+    $alm = new AssetLibraryManager();
+    $alm->set('jquery', $this->createJQueryAssetLibrary());
+    $dep = new AssetLibraryReference('jquery', $alm);
+
+    $css1 = new StylesheetFileAsset(DRUPAL_ROOT . '/core/misc/vertical-tabs.css');
+    $js1 = new JavascriptFileAsset(DRUPAL_ROOT . '/core/misc/ajax.js');
+    // $js1->addDependency($dep);
+
+    $bag->add($css1);
+    $bag->add($js1);
+
+    $this->assertEqual(array(new JavascriptFileAsset('core/misc/jquery.js'), $js1), $bag->getJs());
+  }

So I set out to get the tests to run. But it would fatal: Fatal error: Interface 'Assetic\Asset\AssetInterface' not found in … AssetInterface.php. While trying to get that to work, I failed miserably — class loading has been all-automagically for me so far and I have no knowledge about that part of Drupal 8. I got similar failures when I tried to use that interface from a .module file that I knew was being executed.

Hence, I'm also unable to do any meaningful further development, because I cannot actually run any of the new code.

For the same reason, the two above patches should fail — vendor class loading in D8 is currently broken, it seems (except for Symfony stuff)? I couldn't find a relevant issue. Somebody else hopefully knows?


In the mean time, here's a reroll with nitpicks fixed that will surely arise.

StatusFileSize
new41.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new613 bytes

ah, you beat me to being able to explain the issue with the classloading - very frustrating, that error. it's because our classloader decides it's a PEAR-style library rather than being properly namespaced & PSR0. woohoo. it would probably be fixed by #1658720: Use ClassLoader instead of UniversalClassLoader (thanks for the tip, @dawehner), but in the meantime, i've rolled an updated patch including the two-character fix that makes it work on my local. *facepalm*

It was interesting to see that you've changed the default scope of JS files from header to footer. I applaud that, but it may require changes to some core JS as well (since some may rely on header being the default).

that's a slightly decrepit part of the code, but yeah, i've talked with rupl and Snugug a LOT about that. my feeling from them is that we ought to at least give it a shot as we go along here. there's a fair bit to be thought about with respect to how we track these default values for assets, anyway - one of the complaints i've heard about the system right now is that it's impossible to tell whether the scope that's been set was done so by the original asset declarer, or someone further down the line. ensuring we have a a few entry points that keep those data separate would solve that problem.

with respect to setting defaults, though, there's an entire avenue here that i haven't yet explored: AssetFactory. Assetic provides one of these, and we could easily write our own. it COULD make asset declaration more DX friendly, but it's also really the best way of controlling the defaults that an asset is initialized with: we pass a factory configured with certain defaults to the hook for the system css files, then change those defaults and pass the factory along to themes...etc.

in any case, for the things that really need to be in the header - html5shiv, modernizr, and probably jquery - it's easy to set that when the asset is declared in the library.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-15.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new41.91 KB
FAILED: [[SimpleTest]]: [MySQL] 50,718 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Rerolled #14 with changes from #15

Status:Needs review» Needs work

The last submitted patch, core-assets-1762204-17.patch, failed testing.

Failing for the right reasons at least.

StatusFileSize
new47.24 KB
FAILED: [[SimpleTest]]: [MySQL] 50,830 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new11.22 KB

OK, new patch with two additions, and one removal:

  • i added a test that tests assembling multiple bags together, ensuring that the raw sequencing is correct.
  • i added a host of methods (to the interface) for declaring dependencies and sequencing. i didn't write implementations for them, but if someone else takes a stab before i can return to it, keep in mind that the calculations should all be done during AssetBagInterface::sort(), NOT when the assets themselves are added.
  • i've removed everything related to weight. it is my belief that this system should allow us to accomplish everything we need WITHOUT explicit weights; rather, we rely on a combination of the sequencing logic and the tight control we have over the order in which various contributors can add to the bags, then resolve it all into a graph. doing this should allow us to be more flexible & smarter when creating bundles, at least with js.

there's at least one conceptual hole introduced here - dependencies are wrapped in the AssetLibraryManager and have a referencing class, but mere sequencing does not. i'm pondering the best way to resolve this, or if it really even needs resolution: all that really matters is that we're able to uniquely identify a particular asset when doing comparisons. the AssetLibraryManager approach is designed to do that comparison by ensuring we only ever create a single object instance, but it's equally valid to generate a unique string for individual assets in the sequencing case (and will probably work better).

i'm now working on updating the issue summary.

Status:Needs work» Needs review

this will fail (intentionally), but let's change status so other people can see where those failures are :)

Status:Needs review» Needs work

The last submitted patch, core_assets-1762204-20.patch, failed testing.

+1 to removing weights (if we can) - what we dealing with is dependencies, and we should treat them as such. Need to dig into the patch some more, but from the comments I like the direction.

ok, issue summary is updated. i need to let this sit for a little while, as i have to shift my focus over to the Response stuff, as well as filing an initial issue dealing with master displays. i'm hoping somebody else will be interested in picking up at least some of what's here...i'm more than happy to answer any questions as needed :)

Issue summary:View changes

Revamp after hijacking.

@sdboyer: Regarding the remaining task

Drupal\Core\AssetInterface::sort()

I'm curious about that code effort.

I know Graph.inc suggests too much as it allows for #896698: Circular dependency references between field and field_sql_storage implying it allows for cycles :(

I had code lying around in #310898: Making module_enable and module_disable API functions which never made it. In Graph API #1344648-4: Reincarnate graph class? we have Graph, DirectedGraph and AcyclicDirectedGraph classes

For Drupal\Core\AssetInterface::sort() we can use AcyclicDirectedGraph::getTSL() (code).

Should/Could we upgrade Graph.inc to mentioned classes as we have feature freeze?

@clemens.tolboom - ah, i didn't even realize #896698 was a problem. yeah, definitely not OK here.

upgrading, replacing, or just complementing the existing graph API is definitely something we need here. i started briefly looking at the problem, but didn't get that far into it.

what really makes the difference with this case is that the vertices are objects. that makes it a little trickier, though not hugely - you can convert the objects to unique strings with spl_object_hash(), do the graph calculation, then reconvert them to objects when spitting out results.

@sdboyer I'm working on #1344648: Reincarnate graph class? first to make Unittest for it. Next I'll try this issue. Then Graph.inc

fantastic. ping me when you have something that i can see about dropping into the sort() method. in the meantime, i've made a DX request in that other issue.

Status:Needs work» Needs review
StatusFileSize
new17.15 KB
new17.15 KB

@sdboyer I did not succeed in moving fast enough :(

Here is my small effort :(

<?php
 
protected function sort() {
    if (!
$this->needsSorting) {
      return;
    }
   
$this->sorted['js'] = $this->sorted['css'] = array();
   
$this->sorted['js'] = $this->_sort($this->getUnsortedJs());
   
$this->sorted['css'] = $this->_sort($this->getUnsortedCss());
   
$all = array();
   
$this->needsSorting = FALSE;
  }
  protected function
_sort($items) {
   
$g = new DirectedAcyclicGraph();
    foreach (
$items as $item) {
     
$from_id = spl_object_hash($item);
     
$g->addNode($from_id, $item);
     
$deps = $js->getDependencies();
      foreach (
$deps as $dep) {
       
$to_id = spl_object_hash($dep);
       
$g->addNode($to_id, $dep);
       
$g->addLink($from_id, $to_id);
      }
    }
   
$tsl = $g->getTSL();
   
$result = arrau();
    foreach(
$tsl as $id) {
     
$result[] = $g->getNodeData($id);
    }
    return
$result;
  }
?>

The patch adds a new vendor https://github.com/clemens-tolboom/graph as I think we really need real graph util for Drupal and doing it through GraphAPI contrib module takes way too much time I can handle.

An alternative function _sort() {} would be allowing for cycles thus use a DirectedGraph then convert it to a DirectedAcyclicGraph allowing for errors like

<?php
 
protected function _sort($items) {
   
$g = new DirectedGraph();
    foreach (
$items as $item) {
     
$from_id = spl_object_hash($item);
     
$g->addNode($from_id, $item);
     
$deps = $js->getDependencies();
      foreach (
$deps as $dep) {
       
$to_id = spl_object_hash($dep);
       
$g->addNode($to_id, $dep);
       
$g->addLink($from_id, $to_id);
      }
    }
   
// We need to ignore cycles?
   
$dag = DirectedAcyclicGraph::fromDirectedGraph($g, TRUE);
   
$tsl = $dag->getTSL();
   
$result = arrau();
    foreach(
$tsl as $id) {
     
$result[] = $g->getNodeData($id);
    }
    return
$result;
  }
?>

Feel free to copy the vendor/graph into Drupal namespace but I'll focus on the github lib + Graph API.

StatusFileSize
new84.08 KB
FAILED: [[SimpleTest]]: [MySQL] 49,945 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

With patch :p

Status:Needs review» Needs work

The last submitted patch, core_assets-1762204-29.patch, failed testing.

Hmmm ... test will fail as the classes are not yet registered by the autoloader. That lead to https://packagist.org/packages/submit to submit https://github.com/clemens-tolboom/graph (which I hesitate to do) to discover https://github.com/clue/graph has way more code let alone tests. It lacks the ability to attach data.

@sdboyer: sorry. I for me am out of ideas :(

[edit: fixed wrong link to https://packagist.org/packages/zetacomponents/graph /edit]

Status:Needs work» Needs review

From the log

Fatal error: Class 'Graph\Component\Graph\DirectedAcyclicGraph' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Asset/AssetBag.php on line 183
FATAL Drupal\system\Tests\Asset\AssetAssemblyTest: test runner returned a non-zero error code (255).

@sdboyer I filed a bug report #1915308: Make the graph component input an edge list and add topological sort

Your sort algoritme should look something like below adding a TOP vertices

<?php
 
protected function _sort($items) {
   
$core_id = '__CORE__';
   
$g = array();
    foreach (
$items as $item) {
     
$from_id = spl_object_hash($item);
     
$g[$core_id]['edges'][$from_id] = 1;
     
$deps = $js->getDependencies();
      foreach (
$deps as $dep) {
       
$to_id = spl_object_hash($dep);
       
$g[$from_id]['edges'][$to_id];
        if (!
is_array($to_id)) {
         
$g[$to_id]['edges'] = array();
        }
      }
    }
   
$graph = new Graph($g);
   
$g = $graph->searchAndSort();
    return
$g[$core_id]['paths'];
  }
?>

but that will fail due to the HEAD to TAIL bug mentioned but not fixed in #1915308: Make the graph component input an edge list and add topological sort.

StatusFileSize
new26.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-201302270802.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok; a little movement on this after lots of useful discussions with sdboyer in IRC.

  • I've stripped back the class/interface hierarchy to begin with while we get a handle on what the producer/consumer interface is going to look like. Currently we therefore just have AssetInterface, BaseAsset, FileAsset, AssetGraph (more in a second) and AssetBag. It's likely that for the DIC we'll need an AssetBagInterface or similar (as originally proposed by sdboyer) but this is very much a work in progress.
  • With Drupal\Component\Graph\Graph not satisfying our needs, I've knocked up a specialised AssetGraph class that (as well as producing a TSL) tries to be smart when considering sequencing (i.e. $asset->after($before)) as well as dependencies ($asset->dependsOn($dependency)).
  • AssetBag is currently a thin wrapper around AssetGraph, though as sdboyer's original patch suggests it may need to be aware of CSS/JS components it contains and the like.
  • PartialResponse (currently on loan through the latest patch in #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox]) now has an AssetBag property, as well as some dumb mutators/accessors (to be fleshed out/made more robust).
  • I'd originally been working in the sandbox, but found it difficult to separate out the large number of commits from 8.x when producing a patch, so I've effectively re-rolled -- the attached patch should apply cleanly to HEAD.
  • Some terrible tests.

The next step I'm toying with is hacking BlockInterface::build to return a PartialResponse so that we can begin looking at how asset aggregation (i.e. bagging) should work -- thoughts?

Apologies if this cut-down patch feels like a step in the wrong direction, but hopefully it's better than nothing.

lunaris

thanks for the review, sorry for my delayed response. and that i'm further delaying a real response until later this week :(

in the meantime, though, any chance you could put up an interdiff vs. #20? i can keep the sandbox updated that way, at least.

StatusFileSize
new62.33 KB

Does this help?

About the DAG, we might get away without. In any case everything works when all weight keys are removed from scripts #1945262: Replace custom weights with dependencies in library declarations.

Title:Introduce new asset declaration architecture to facilitate removal of global drupal_add_js/css()Introduce new asset declaration architecture to facilitate removal of global drupal_add_js/css() [Moved to sandbox]
Status:Needs review» Postponed

per the discussion in #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox], marking this postponed - we're working on it in the SCOTCH sandbox.

@nod_ - it's possible, but i still think it's optimal. i'm aiming to get back to this this week (in the sandbox), at which point i'll revisit the DAG as well. honestly, i don't think it'll be that bad - the DAG, while somewhat complex to understand, is not terribly complex to implement and is a very well-defined problem space. and it definitely gives us a robust system to fall back on that, i think, folks will end up finding very intuitive.

Removing the issue summary initiative tag, since sdboyer updated the summary several times since the tag was added.

I haven't read the patches yet, and probably won't until the sandbox is further along, but the benefits listed in the issue summary are cool.

The one thing I question though is:

This change is necessary because the current system is reliant on globals (drupal_add_css/js())

If this system were truly necessary to make ESI work, it would warrant setting this issue's priority to major. However, we ought to be able to remove the *public* use of drupal_(add|set)_*() without any of this. We already have the #attached key of render arrays, and need to fix all places where code calls the global functions to use #attached instead. The issue for doing so is #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff and can happen in parallel with this one. Once that issue is done, we can tweak the internal implementation of drupal_process_attached(), whether by simply adding an "_" in front of the drupal_add_*() functions to make clear they are not public API functions, or by integrating with the more powerful/pluggable stuff being worked on here.

Issue tags:+D8 cacheability

Issue tags:-D8 cacheability

Per #40, removing the "D8 cacheability" tag. I'd like that tag to be focused on improving D8's caching via what's already available within existing core architecture (e.g., #attached and Drupal render cache). This issue remains a cool idea though on its own merits.

In short, I'm worried about the state of this. Using Assetic is what will bring us better CSS minification, proper JS minification, other optimizations, and possibly most importantly: the ability to swap any Assetic filter (minifiers, mostly) for another one, in contrib.


No commits have been made to the 1762204-assets branch of the SCOTCH sandbox in 3 months (last one on January 21, 2013).

#35 seems to do a very nice job of moving this forward.

#38 says the DAG is non-essential.


Let's get this issue moving forward again!

#40 suggests to me that we could go for an intermediary step where we don't let this depend on SCOTCH, but instead let drupal_process_attached() instantiate all the Assetic-powered goodness that's being worked on in this issue. In a next step, we can then get rid of #attached and move that over to the PartialResponse stuff that's being worked on over at #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox].

Thoughts?

all new commits are in the princess branch, i merged the 1762204-assets branch into princess back in March.

#35 scoots us along in some respects, but also changes a lot of things that I'm not entirely sure about.

i don't think we'll be able to get away without a graph in the long term, but we definitely don't need it right away.

i do believe we can rely on #attached for the moment, then bolt SCOTCH back in on top of this later if we can get those pieces sorted out.

Status:Postponed» Needs review
Issue tags:-JavaScript, -API change, -API clean-up, -JavaScript clean-up

#35: assets-1762204-201302270802.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+JavaScript, +API change, +API clean-up, +JavaScript clean-up

The last submitted patch, assets-1762204-201302270802.patch, failed testing.

Status:Needs work» Postponed

...as per #39.

Unless of course you intent to work on this Robin. (?)

@klonos I'm currently trying some things in D8 with Assetic, can this issue stays open?

Status:Postponed» Active

...of course it can. It's simply that after your action to retest #35, you also changed the state from "postponed" to NW but never mentioned that you'd work on this. Some people simply do that out of curiosity if the patch still applies, but never intent to work on the issue. That's why I asked. Please also go ahead and assign to yourself while working on it. Thanx and sorry.

Assigned:Unassigned» robinvdvleuten

For anybody interested, I've applied the patch to the latest version of core and moved everything to this sandbox. I'll keep you guys updated about how things are going.

@robinvdvleuten - per #44 you probably want to branch off @sdboyer's princess branch - it sounded like there had been further commits there since the last patch.

@owen The princess branch seems to be broken. I get an error about a missing Timer class. Does anyone know how to fix this?

yeah - ping me in irc, #drupal-scotch will work well. Wim is also working elsewhere on a set of base services related to Assetic now, and we have a whole incremental plan for pushing things into core. but there's definitely work to be done on various parts of the guts of what's been done here.

a missing 'Timer' class is weird, though...it installs clean for me, and for others.

Assigned:robinvdvleuten» sdboyer
Status:Active» Needs review
StatusFileSize
new67.71 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

rebooting this a bit. this patch takes care of a lot of what's described in the issue summary, and punts some of it, too. it leaves some significant pieces undone, as well, unfortunately. i've spent a long time here and really have to focus on other things today, so i'm pivoting over to those. plus, my brain's just blown out on this for the moment.

i'll return here this week, or maybe later today/tomorrow, depending on what else i can get done. so...whatever that means.

here's the substantive changes in this patch:

  • i've refactored the whole AssetBase hierarchy. it's still pretty dirty, but it's much more functional. probably most important is that i've set up ArrayAccess-based interaction with all the metadata we have historically stored on assets. that should actually make slotting this code in with existing stuff MUCH easier.
  • the library system is very nearly internally complete - probably more than any other single piece here. it still needs integration into the system itself.
  • i've introduced a pseudopattern that i'm currently calling 'Collectors.' they're half factory, and half kinda-sorta-repository. they're passed around to things needing to produce a certain kind of object (here, an asset object), help with that process (i.e., be a factory), then stick the results into a collection (i.e., a repository). this can be used with inversion of control to facilitate an info hook-like pattern, or be injected directly into some other code for object collection. there are two collectors in this patch: AssetLibraryCollector, which is intended for use in a replacement to hook_library_info(), and AssetCollector, which is intended to be injected into a block's declareAssets() method (also added by this patch).
  • i've added a ton of unit tests, though primarily for AssetCollector and AssetLibrary in this iteration. these also inherently test certain segments of AssetBase.
  • AssetBag is no longer responsible for sorting. that should be taken care of by a separate layer. this simplifies its interface and responsibilities considerably - it is now, clearly and unambiguously, a set of discrete asset collections.

absolutely necessary TODOs:

  • many, many, many docblocks are missing.
  • some things in the AssetBase stack are still relying on Assetic utilities. these need to be replaced with Drupally equivalents. could really use some help from someone who knows file/resource handling well...
  • the library system needs to be plugged in to transparently exist alongside (and eventually replace) the existing hook_library_info().
  • AssetLibraryManager needs finalizing - a couple more methods. not big stuff.
  • Unit tests for AssetBag, parts of the AssetBase hierarchy, AssetLibraryManager, AssetLibraryCollector.
  • implement the (weight, then graph) asset sorter. most important is dependency resolution.
  • i need to reach in to drupal_add_js() and drupal_add_css() and convert all incoming assets to their appropriate types in this system. we can then rewrite the interfaces being worked on in #352951: Make JS & CSS Preprocessing Pluggable to expect these object types.

more nice-to-have TODOs:

  • AssetBag probably ought to return a collection object from getCss() and getJs(), rather than an array. this'll be the thing that a sorter can later work with.
  • i haven't dealt with separated names/ids for the assets. Assetic doesn't have them, and we don't currently *really* either (we just use the guaranteed-unique path to the asset), but it might be nice to have. #35 introduces such an id; i haven't used it here, but we could.

and things that are definitively followups:

  • i'm pretty unhappy happy with how 'defaults' are implemented in the AssetBase hierarchy - but as long as it works for now and can live behind that interface, we can refactor it later.
  • We've done nothing about creating a FilterManager, but we'll want and need to do that and have it behave very similarly to how the AssetLibraryManager does - a global/container-encapsulated listing of available filters.
  • the fact that the Collector pattern is incapable of returning the collection/bag object it fills, and instead relies on the injector of the bag to retain somewhat magical access to the filled bag by reference, is pretty cheaty.

also, this is effectively blocked on #1308152: Add module://, theme:// and profile:// stream wrappers to access system files, as without it it's impossible to write dependencies in a portable way (without explicit ids). but also because it's just 122% uglier without it.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-52.patch, failed testing.

Status:Needs work» Needs review

oh, one thing worth emphasizing on its own: this can go in, once the TODOs are finished, without having to change the way that *anything* currently interacts with any of the asset APIs. everything can be done with a BC/conversion layer that we can gradually phase out. or not.

StatusFileSize
new67.98 KB
FAILED: [[SimpleTest]]: [MySQL] 56,561 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new3.26 KB

oh FFS testbot...

yeah, that's phpunit blowing up if tests don't have a test method in them. i did that as placeholder reminders of the unit tests we need to write. so i added a stub method to each of the three.

btw, there are known failures on AssetMetadataTest and AssetAssemblyTest, the latter of which is decrepit and needs to go away completely, anyway. i fixed a couple small things in the former to at least prevent fatal errors due to calling methods that had been refactored out. that test is from a long time ago and i hadn't been paying close attention to it.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-57.patch, failed testing.

Title:Introduce new asset declaration architecture to facilitate removal of global drupal_add_js/css() [Moved to sandbox]Introduce new asset declaration architecture to facilitate removal of global drupal_add_js/css()

oh yeah, we're not in sandbox anymore.

Status:Needs work» Active
StatusFileSize
new70.27 KB
FAILED: [[SimpleTest]]: [MySQL] 56,533 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new10.06 KB

...well now, that was easier than i thought.

this interdiff changes AssetLibraryManager to AssetLibraryRepository, which is a more accurate description of its role, though still not quite right. more importantly, it adds a really dirty implementation that completely populates the ALR with all the assets declared in hook_library_info(). wowzers.

now, it's doing a full load of all library declarations, rather than an as-needed module-by-module load like drupal_get_library() currently does. and the code is HORRIBLY dirty that does it. but, it's got all the library data, in the new form.

onward and upward!

Status:Active» Needs review

Status:Needs review» Needs work
Issue tags:-JavaScript, -API clean-up, -JavaScript clean-up

The last submitted patch, assets-1762204-60.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.11 KB
new72.14 KB
FAILED: [[SimpleTest]]: [MySQL] 56,525 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

First: it works well :)


Review

I had intended to review everything and to add docs wherever they were missing. It turns out that AFAICT this patch is not yet "nearly RTBC". It still needs a fair amount of work, both in terms of cleaning up docs & loose ends, but also in terms of architectural unclarities. The review below does cover the majority of the patch though.

Overall remark: IMHO any and all Assetic Filters stuff should be removed from this patch; only once #352951: Make JS & CSS Preprocessing Pluggable lands and the code introduced there is converted to use the new interfaces introduced here, we can start using filters. Filters belong in the preprocessing/aggregation/minification/optimization phase, not in the declaration ("I need this asset to be loaded") phase.
In case I misinterpreted this and these filters were not intended for optimization purposes, but rather to support the likes of SASS, CoffeeScript and so on: I think we should not aim to get that in Drupal core as part of an initial patch, especially because there are sound reasons to not have that explicitly supported in Drupal core at all. I'd hate to see this go down over a bikeshed over that aspect.

I removed the following classes after chatting with sdboyer and confirming that they're no longer needed/used by the patch: core/lib/Drupal/Core/Asset/AssetGrouper.php, core/lib/Drupal/Core/Asset/AssetGroupingInterface.php and core/lib/Drupal/Core/Asset/AssetLibraryReference.php.

+++ b/core/lib/Drupal/Core/Asset/AssetBagInterface.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Asset/AssetCollector.phpundefined

The whole point of AssetCollector is not very clear.

I think it would feel more natural to have all these methods on AssetBags instead. I suspect you made this split to keep AssetBags stupid?

+++ b/core/lib/Drupal/Core/Asset/AssetCollector.phpundefined
@@ -0,0 +1,232 @@
+  public function create($asset_type, $source_type, $data, $options = array(), $filters = array()) {
+    return call_user_func(array($this, $this->methodMap[$asset_type][$source_type]), $data, $options, $filters);

This method is only being used in AssetLibraryRepository and in tests. Doesn't that mean this method is not truly necessary? Along with all the create*Asset methods?

+++ b/core/lib/Drupal/Core/Asset/AssetDependencyInterface.phpundefined
@@ -0,0 +1,17 @@
+ * Describes an asset or asset-like object that can declare dependencies.

What would an "asset-like" object be?

+++ b/core/lib/Drupal/Core/Asset/AssetInterface.phpundefined
@@ -0,0 +1,53 @@
+   * Sets default metadata to be used for this asset.

So is this "metadata" or "options"?

(Assuming AssetCollector::create()'s "options" parameter is intended to receive this.)

In any case, I think it's better to call this setDefault(Options|Metadata), to make it explicit *which* defaults you're setting.

+++ b/core/lib/Drupal/Core/Asset/AssetLibrary.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Asset/AssetLibraryCollector.phpundefined

This is only being used in AssetLibraryRepository AFAICT, which suggests to me that it might be better to have the logic to manipulate AssetBags in there, rather than have a separate class (AssetLibraryCollector)?

This is closely related to my remarks on AssetCollector.

+++ b/core/lib/Drupal/Core/Asset/BaseExternalAsset.phpundefined
@@ -0,0 +1,74 @@
+  public function getLastModified() {
+    if (false !== @file_get_contents($this->sourceUrl, false, stream_context_create(array('http' => array('method' => 'HEAD'))))) {
+      foreach ($http_response_header as $header) {
+        if (0 === stripos($header, 'Last-Modified: ')) {
+          list(, $mtime) = explode(':', $header, 2);
+
+          return strtotime(trim($mtime));
+        }
+      }
+    }

I applaud the intention, but this is unacceptable to be the default behavior AFAIK.

Retrieving and using this sort of metadata about an asset belongs in the asset optimization phase, i.e. #352951: Make JS & CSS Preprocessing Pluggable.

+++ b/core/lib/Drupal/Core/Asset/BaseFileAsset.phpundefined
@@ -0,0 +1,56 @@
+  public function getLastModified() {
+    if (!is_file($this->source)) {
+      throw new \RuntimeException(sprintf('The source file "%s" does not exist.', $this->source));
+    }
+
+    return filemtime($this->source);

Same.

+++ b/core/lib/Drupal/Core/Asset/BaseStringAsset.phpundefined
@@ -0,0 +1,35 @@
+  public function __construct($content, $options = array(), $filters = array()) {
+    $this->content = $content;
+    $this->lastModified = REQUEST_TIME;
+
+    parent::__construct($options, $filters);
+  }
+
+  public function setLastModified($last_modified) {
+    $this->lastModified = $last_modified;
+  }

Same. And in this case, the information is not even reliable: what if the inline asset has remained unchanged? It will still use the request time as the last modified stamp, potentially leading to unnecessary cache invalidations.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-63.patch, failed testing.

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

removing API change tag, because we can actually implement this without changing how any existing code interacts with the assets system.

and adding happy princess API, because princess needs this.

sticking with needs review just to hopefully get some more eyes on this; we all know it needs work.

responding to #63:

It turns out that AFAICT this patch is not yet "nearly RTBC".

yep. sorry if i gave a different impression. this is decidedly WIP.

Overall remark: IMHO any and all Assetic Filters stuff should be removed from this patch; only once #352951: Make JS & CSS Preprocessing Pluggable lands and the code introduced there is converted to use the new interfaces introduced here, we can start using filters. Filters belong in the preprocessing/aggregation/minification/optimization phase, not in the declaration ("I need this asset to be loaded") phase.
In case I misinterpreted this and these filters were not intended for optimization purposes, but rather to support the likes of SASS, CoffeeScript and so on: I think we should not aim to get that in Drupal core as part of an initial patch, especially because there are sound reasons to not have that explicitly supported in Drupal core at all. I'd hate to see this go down over a bikeshed over that aspect.

i'd like to keep this simpler, too, and i don't disagree with any of this. but i'm also trying to retain compatibility with Assetic's AssetInterface. that's the cause of a lot of the complication and bloat in the Asset hierarchy represented in this patch.

in Assetic, there is no structural distinction between a load filter (which would be where one implements SASS, CoffeeScript, etc. compilation) and a dump filter (which is for optimization). there are different methods that are called, but all filters must implement both; filters that don't care about one or the other simply have an empty method implementation. yeah, this is no bueno.

however, filters are not actually RUN until the load() or dump() method is called on the asset. which means that, even though they could be declared by any asset producer in a Drupal codebase, they wouldn't be run until something calls load() or dump(). and that, i agree, should be entirely the domain of the services we conceived in the other patch.

thus far, i've felt OK about just ignoring filters. and that OK feeling stems largely from the fact that, since we're only looking at a backwards-compatibility layer to the current API right now for libraries, and there are currently no blocks that produce assets, there is *no way* that Drupal asset producers could declare filters. for me, that's good enough to punt it to a followup.

The whole point of AssetCollector is not very clear.

I think it would feel more natural to have all these methods on AssetBags instead. I suspect you made this split to keep AssetBags stupid?

yes, i don't want AssetBags to be factories, primarily because of the granular control over defaults that needs to be there.

Collectors are essentially an objectification of a pattern we often use with info hooks - invoke a hook, get back an array of data, caller modifies it in some way. their goal is to:

  • bring this pattern to objects that have more rules around state, and control over when/by whom certain values can be set, than simple arrays.
  • assist code implementing such hooks (so, responders) with a factory-like interface that makes it easier to respond.
  • assist code invoking such hooks by allowing more granular, even responder-by-responder control over the way the factory creates objects.

the intention is that these collectors can travel around to the responders, and implicitly record the results. that's why the various lock()-related logic exists - so that anything other than the factory functionality of the collector can be shut down while it's traveling.

in truth, i probably shouldn't have spent as many hours as i did with them on Friday, but it seemed pretty clear while i was doing it, so i went for it. i was trying to deal with the minor clusterfuck that is options & defaults options on assets. initially i was trying to do object properties for them, which just gave me a headache, so i tucked them all into a single array and added a method for externally controlling it, which i handed to the factories. initially i'd intended to make those 'defaults' set in the constructor, and immutable thereafter, which would more thoroughly justify the need for a factory-controlled instantiation process.

This method is only being used in AssetLibraryRepository and in tests. Doesn't that mean this method is not truly necessary? Along with all the create*Asset methods?

well, this hasn't been hooked substantively into the system yet, but AssetLibraryRepository's use case, while dirty, is legitimate.

but i agree partway - we can get rid of either the generic create(), or the specific ones, but not both. remove them, and it's not a factory anymore, so there's no point at all to the collector (per above). the specific ones are what should go, they're terrible copy/paste code.

i wrote the collector when i thought there would be variation across the constructor signatures for different assets. variations that would mean a generic factory method might not be adequate for some, but not all, instantiation cases, depending on what data needed to be passed. so i wrote the specific methods, with the idea that they could vary based on the needs of the specific asset. subsequent to that, i normalized the constructors, so the issue became moot. i just didn't get around to removing the specific methods.

What would an "asset-like" object be?

an AssetLibrary. that's the other thing that implements AssetDependencyInterface.

This is only being used in AssetLibraryRepository AFAICT, which suggests to me that it might be better to have the logic to manipulate AssetBags in there, rather than have a separate class (AssetLibraryCollector)?

This is closely related to my remarks on AssetCollector.

yep, and i have roughly the same response: i do not want to pass the AssetLibraryRepository around in the info hook phase. or to a block's reportAssets() method. without a separated object for collection, we'd have no choice but to do that. that's what an alter phase is for. in effect, i want the collection to be append-only.

collectors could be rewritten to produce a bag that implementors/responders are expected to return, and then the calling code merges that in with the existing bag. that is more explicit and definitely better from a clear interface perspective, but also more complex, since we're merging collections/collection-like objects.

I applaud the intention, but this is unacceptable to be the default behavior AFAIK.

Retrieving and using this sort of metadata about an asset belongs in the asset optimization phase, i.e. #352951: Make JS & CSS Preprocessing Pluggable

hehe. yep, this is code pretty much directly copied over from Assetic. remember how @kriswallsmith said he wants assets to be inert in v2? this is a good example of one place that they're not, and how that can be problematic. so, i don't disagree. but if we're going to retain consistency with Assetic's v1 API (and filters), this has to be there, and it's what filters and such will interact with.

exact same story for your next comment on the same method for file assets, as well (no surprise).

Same. And in this case, the information is not even reliable: what if the inline asset has remained unchanged? It will still use the request time as the last modified stamp, potentially leading to unnecessary cache invalidations.

i can't even remember if i copy/pasted that or not, but that's basically throwaway code at this point - i have NFI what the best idea is for a last modified time on a string asset. all that occurred to me was setting it based on mtime of the file that declared the string asset, which is still kinda batshit crazy, not to mention kinda complicated to implement reliably, even with a Collector.

oh ffs, firefox

StatusFileSize
new66.69 KB
FAILED: [[SimpleTest]]: [MySQL] 56,294 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new11.89 KB

Refactored AssetCollector to remove all the type-specific factory methods. should be a little clearer now.

continuing on to other things. i'm prioritizing getting it working over making it clean. we can refactor once we've covered the use cases, or even in a followup.

i am, though, considering removing the use of Assetic's AssetInterface (at least for now) at the base of our hierarchy. we lose filters, but we get a cleaner set of things to implement on our assets, which we kinda do want to be inert.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-68.patch, failed testing.

Title:Introduce new asset declaration architecture to facilitate removal of global drupal_add_js/css()Introduce Assetic compatibility layer for core's internal handling of assets
Status:Needs work» Needs review

updating to reflect the fact that this patch isn't pushing for a change to existing public APIs.

StatusFileSize
new179.43 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

and you thought this patch was dead. hah!

we're not done yet, but we are so, so very much closer. however, given that there are still major holes and this patch just tripled in size (and that's entirely additions), let me first direct attention to the areas that definitely bear some discussion right now:

  • this brings in, via composer, a standalone library that i wrote. hopefully no one has complaints about my choice to write this as a separate library rather than putting it in Component because...well, because this is what's happening. but if people want to bellyache, let's get all up in arms around that bikeshed sooner rather than later, plz.
  • now, that thurr library is for doing fun things with graphs. like, what's being done in CssCollectionGrouperNouveaux (the goal being to swap that in for CssCollectionGrouper). that is the meat of this whole thing, and what could *really* make all of this worth it: a graph traversal algorithm that finds the optimum minimal grouping for assets. that means, during asset aggregation, we generate the minimum number of groups possible while still respecting all the dependency/sequencing requirements of each asset. while i'm not entirely happy with the algorithm that i've devised (for example, it is VERY sensitive to starting vertex - hence CssCollectionGrouperNouveaux::createSourceQueue()) i'm pretty i'm on the right track. feedback there would be invaluable. it'll definitely take a bit of grokking to understand how Gliph does its traversal, but basically, we overlay a graph representing the absolute minimum grouping (ish - if it were an actual graph, it would mean (V-1)^2 visits, only (V-1) of which could ever be fruitful) on top of the dependency/ordering graph.
  • i'm using a much lighter set of asset metadata attributes than what core currently uses to determine group membership. some of this should be noncontroversial - for example, media is no longer a qualifying property, because we can (rather, must - #1488910-7: Incorporate media queries in aggregated CSS files, don't create a new group for each new media query) embed that data in the body of the css itself. however, i also pulled out group. i'm pretty sure that's not correct in the long term, but i'd rather we justify the reasons for having that property in the first place than just assume it should stick around. i do have an idea about how we should use it, but i'm gonna bite my tongue for now. i'd also love to kill every_page, but that's something i can't picture doing sanely without princess...so, contrib.

as i said, there's still a fair bit left to do, not the least of which is about 100 unit tests, but i think this patch represents a nearly-complete foundation (the glaring exception is that i need to update AssetBag, which hasn't really changed since april or so). but because the rest is probably still a bit confusing, let's please have reviews focus on the above three listed things until i can get a more complete patch up. i'm out of town this weekend, but i'm on this now, and i'll be continuing to work feverishly on it next week, so more is coming soon.

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

The last submitted patch, assets-1762204-71.patch, failed testing.

Status:Needs work» Needs review

#71: assets-1762204-71.patch queued for re-testing.

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

The last submitted patch, assets-1762204-71.patch, failed testing.

I'd like to know what's wrong with https://github.com/clue/graph mentioned in #32. That lib has a lot algorithms implemented already. So I don't understand creating an new one :-/

@clemens.tolboom - simplest answer: i'd totally forgotten your earlier response noting that there was another library out there.

from a cursory look through, though, their implementation seems rather verbose, and i'm not sure how much benefit there is to the added layers. they seem to place certain requirements on the structure of the vertices, whereas i designed gliph to require *only* that the vertices are objects. gliph never, ever mutates the internal structure of objects (though you can certainly do so yourself by extending it), and has no requirements of the objects (e.g., that library seems to require that its vertices have unique string identifiers).

actually, to be more precise, it seems that library requires that its vertices be strings OR ints. which means writing a mapping layer if you're doing real OO, which is the way i've built the rest of this system.

my library is ~750 LoC, including tests; theirs is 10x as big. of course, nothing precludes us from using both, if there's another use case for which their system is more appropriate - that's a wonderful thing about composer.

most important for our purposes, though, is how efficiently and tersely the algorithm i designed could be implemented in that library (assuming its correctness), as it is a custom depth-first traversal.

Status:Needs work» Needs review
StatusFileSize
new41.32 KB
new184.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-77.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

updated patch. hopefully this one will run, and pass tests. doing so is not yet a cause for jubilation, as i've not actually swapped in the new logic - just started adding tests that run through the swathe of new logic i've added. those tests, which are exclusively unit tests, DO pass. yay :)

i've also finished up most of the core classes, esp. refactoring AssetBag to be where it needs to go.

however, i've also pretty much come to the conclusion that the separation into two separate class hierarchies for js and css is overkill, making the system unnecessarily verbose. getting rid of that distinction could remove at least 12 classes/interfaces, and we could still probably do the same enforcement we'd otherwise do. it could also obviate the need for the whole AssetBag hierarchy, as that could be rolled in entirely with collections. so, significant upsides. not quite sure i want to do it yet, but it's seeming increasingly likely.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-77.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new184.04 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Had to reroll after #2088651: Update symfony components to latest 2.3.* in order to re-reconcile gliph; no interdiff. this should apply and pass.

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

The last submitted patch, assets-1762204-79.patch, failed testing.

Status:Needs work» Needs review

#79: assets-1762204-79.patch queued for re-testing.

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

The last submitted patch, assets-1762204-79.patch, failed testing.

ugh. lemme track that down...

Status:Needs work» Needs review
StatusFileSize
new1.18 KB
new184.09 KB
FAILED: [[SimpleTest]]: [MySQL] 58,215 pass(es), 360 fail(s), and 9 exception(s).
[ View ]

ok, hopefully that finally does it - i was a little premature in when my hacks into drupal_add_js() were firing into the collector, and it was choking.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-84.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new194.52 KB
PASSED: [[SimpleTest]]: [MySQL] 58,347 pass(es).
[ View ]
new11.54 KB

ok, the issue there was that in the current system we have 'inline' assets, and in the new system we have 'string' assets. i'd forgotten about this difference, but it actually kinda points to something awkward in our current paradigm: two of the asset types refer to the type of source, but the third refers to a manner of use. that's gross.

in any case, i've introduced a normalization (with a TODO) in AssetCollector that should fix those failures for now.

this patch also includes a full suite of rather crucial tests for AssetGraph, as well as some tweaks and fleshings-out of its internals. those revolve around the primary functionality of the AssetGraph over and above its parent, which is to work directly with the sequencing data that can be declared on assets in producing the full set of edges. i think i like the separation of concerns, there.

i'm now in the process of gutting the type hierarchy-level differentiation between js and css. things are still failing hard locally - i'll have it green tomorrow, or perhaps while i'm on the plane to Prague. there are a couple niggling annoyances with it, which were the reason i went down the verbose path in the first place. but the reduction in complexity in the type tree is enough five times over to offset those issues.

AssetLibraryRepository needs significant refactoring. if someone wanted to step up to do it, that'd be cool - it basically needs to be made to look like a plugin manager (caching + collection of data from an info hook), though i am flatly opposed to it actually BEING in that hierarchy :)

StatusFileSize
new56.5 KB
new183.75 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

wow, that actually wasn't as hard as i thought. probably not quite done, i'm sure i've missed a thing or two, but i've refactored out the type-level representations of CSS & JS, moving it down into mere identifying methods. 12 classes, gone, and about 600 fewer lines in total.

i should be able to wrap up the internal bits of this at the sprint tomorrow (though could use help on tests, to be sure...), at which point we can deal with the harder, more interesting questions of how we attach it to the main system.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-88.patch, failed testing.

Some tiny remarks:

Is the issue summary up to date with the latest patches? Esp. regarding the Glyph library.

+++ b/core/lib/Drupal/Core/Asset/Aggregate/AssetAggregateInterface.php
@@ -0,0 +1,53 @@
\ No newline at end of file

A lot files have No newline at end of file

Status:Needs work» Needs review
StatusFileSize
new188.78 KB
PASSED: [[SimpleTest]]: [MySQL] 58,723 pass(es).
[ View ]

yeah, issue summary isn't even close to up to date :) i've been neglecting it until i finish the basic internals, but i'm nearly there with that. once i do, i'll update the summary.

this patch should fix the installer error, and also adds full test coverage for AssetMetadataBag and the JS/CSS subtypes.

no interdiff because of an oddity in my local merging stuff. c'est la vie. note that i have, though, tossed this up on github for other folks' ease of following along/contributing. PRs there, if needed, kthx.

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

The last submitted patch, assets-1762204-91.patch, failed testing.

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

#91: assets-1762204-91.patch queued for re-testing.

This is exceptionally awesome.

StatusFileSize
new191.37 KB
PASSED: [[SimpleTest]]: [MySQL] 58,793 pass(es).
[ View ]
new105.3 KB

@RobLoach - always nice to hear, thanks :)

ok, there's still a lot to do, but i gotta pivot to other stuff for a little while. i'll gonna update the issue summary, including a TODO list, and it'd be great if i could start getting other people helping out on this. forking my gh repo should probably be the easiest way to do that, but you can roll patches if you really want :)

this latest patch does the following:

  • Adds 100% unit test coverage for BaseAsset, AssetCollection, and AssetCollector.
  • Our BaseAsset now descends from Assetic's BaseAsset (again). This probably won't be an issue, but could be a little awkward because all of their base's properties are private.
  • Updates gliph to 0.1.3 - all i really did in that version was document the shit out of everything and add formal interfaces.
  • Introduces a mechanism in AssetCollector that optionally automatically creates ordering relationships between the CSS assets it creates. This will probably be really important for getting peoples' implicit expectations about how the ordering of their declared CSS should work.
  • Stability was introduced into asset ids, removing the need for AssetCollectionBasicInterface::reindex(), so that was removed from everywhere.
  • Completely removes AssetLibraryRepository and AssetLibraryCollector. I may want to bring these back later, but they were the worst code in the system, and we can get away with not having them by never putting a library reference straight into an AssetCollection - that would mean resolving and building the library as it's being worked through within drupal_process_attached(). This is probably not the end of the world, and it got rid of bad code, sooo...

Issue summary:View changes

add repo info

Issue summary:View changes

home stretch! totally rewrite the summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Added todo for drupal_process_attached()

no takers, eh? :(

i'll return to this next week.

StatusFileSize
new122.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,657 pass(es).
[ View ]

OK, picking this back up again. first, a big reroll that will just remove the addition of gliph, since (yay!) #2098375: Add gliph to composer went in. patch size dropped by 70k, woohoo!

StatusFileSize
new24.05 KB
new130.95 KB
PASSED: [[SimpleTest]]: [MySQL] 59,044 pass(es).
[ View ]

ok, knocked out the AssetCollectorInterface and some accompanying exception improvements. also put in the skeleton for BaseAssetAggregateTest - that's gonna suck to write :/ but, that's on deck for later tonight.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

ticked off AssetCollector interface, couple other minor points.

StatusFileSize
new25.57 KB
new135.54 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Asset/CssCollectionOptimizerNouveaux.php.
[ View ]

ok, this introduces AssetGroupSorterInterface, and separates the graph sorting out into its own discrete class. i'm not entirely happy with the interface there, nor have i written tests for it yet. however, we've known this is a prerequisite, as shifting to this system necessitates that we run the sort whether the optimizer is run or not.

these ended up feeling like prerequisites to really defining BaseAggregateAsset enough to unit test it (i'm still unhappy with some of the wobbliness there), so i tackled them first.

also, it's seeming pretty clear that AssetLibraryRepository will need to return - it's the only sane way to keep track of dependencies.

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

The last submitted patch, assets-1762204-99.patch, failed testing.

Status:Needs work» Needs review

#99: assets-1762204-99.patch queued for re-testing.

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

The last submitted patch, assets-1762204-99.patch, failed testing.

ok, i don't know wtf is going on there, there are no syntax errors in that file on my local.

if (empty($uri || !file_exists($uri))) {

You can't have empty() on function calls in php5.3, I think that's a 5.5 thing?

And even then, shouldn't it be
if (empty($uri) || !file_exists($uri)) {

Status:Needs work» Needs review
StatusFileSize
new799 bytes
new135.54 KB
PASSED: [[SimpleTest]]: [MySQL] 59,112 pass(es).
[ View ]

yep, that was it. thanks, @tim.plunkett. typo on my end, because yes, that's what the logic should have been. i've now set my phpstorm validation down to 5.3 from 5.5 on that project...

Issue summary:View changes

Updated issue summary.

StatusFileSize
new64.38 KB
new195.21 KB
PASSED: [[SimpleTest]]: [MySQL] 59,106 pass(es).
[ View ]

this adds sort methods to AssetCollection, which actually makes a few things much simpler - we need rigid ordering in some cases where we're using a collection, but not in others. this lets that happen without having to create a separate class. i'll update the issue summary accordingly soon.

hat tip to @msonnabaum for helping me think that one through.

also updates Assetic and its dependency, symfony/process, as we're gonna need that presently. one of my next/soon patches will pull in Assetic's own dependencies, which it lists under require-dev for reasons i suspect are specific to how symfony expects to interact with it.

the non-vendor change in this interdiff is very small, and visible here - https://github.com/sdboyer/drupal/commit/ec650d3e34d7bf77653500dcf641952...

Issue summary:View changes

Marked exception thing complete - it's good enough, and the remainder will be fixed by other refactors.

StatusFileSize
new7.17 KB
new197.98 KB
FAILED: [[SimpleTest]]: [MySQL] 58,836 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

ok, first pass at the tests for BaseAssetAggregate. there's definitely some kinks in that class that i'd like to work out a bit, mostly pertaining to how we conceive of the typing. but, it progresses.

this patch will fail, as i also added shell tests for other methods on BaseAssetAggregate that still need to be filled in, and their body is just $this->fail(). i'll see if i can't add similar shell tests in my next patch for other things, as well - better a (more) truthful red than a lying green.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-107.patch, failed testing.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

strikethrough on separated graph sorter

Issue summary:View changes

interface for AssetMetadataBag is done; add todo/notes about ids in collections.

Status:Needs work» Needs review
StatusFileSize
new38.81 KB
new204.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-109.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

next pass. this does the following:

  • divides AssetOrderingInterface into two interfaces - one for dependencies (DependencyInterface), and one for pure positioning (RelativePositionInterface).
  • introduces a MetadataFactoryInterface - a factory for asset metadata - and makes the AssetCollectorInterface utilize it.
  • minor docs fixups.

should still have the same 6 fails.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-109.patch, failed testing.

StatusFileSize
new29.62 KB
new197.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-111.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

two changes on this pass:

  • refactored all the clumsy default handling stuff out of AssetMetadataBag, knocking off another big TODO item, and pulling out a bunch of code - the metadata bags now just descend from generic ParameterBags.
  • further articulated the metadata factory by telling it about the source type and 'data' (path for 'file' or 'external', body of the asset for 'string').

this will fail again because of composer conflicts. i'll reroll again for that later.

Issue summary:View changes

checked off creation of asset metadata factory.

Status:Needs work» Needs review

For Testing.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-111.patch, failed testing.

Issue summary:View changes

mark finished the step to simplify AssetMetadataBag

Issue summary:View changes

Buncha new todos

Priority:Normal» Major
StatusFileSize
new146.31 KB
FAILED: [[SimpleTest]]: [MySQL] 59,136 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
new135.75 KB

1.5 things in this pass:

  • Unit tests for FileAsset, StringAsset, and ExternalAsset. Ideally, we could maybe even get rid of our one-off methods here and just rely on Assetic...but whatever, for now, they're there. There are 3 new expected failures for the ExternalAsset methods that throw exceptions where there should be real logic; there are TODOs there to fix those things. Note that fixing those would be an ideal place for someone to jump in and help me out a bit, here.
  • I reverted the changes to composer, and opened a separate issue for it: #2114165: Add mrclay/minify vendor library to core.

also, bumping this to major. maybe it should even be critical. from a frontend performance perspective, it rather is.

so, for clarity, we're now up to 9 known test failures.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, assets-1762204-114.patch, failed testing.

Issue summary:View changes

touchup, plus one more item

Issue summary:View changes

Mark off unit tests for base asset types

Status:Needs work» Needs review
StatusFileSize
new157.06 KB
FAILED: [[SimpleTest]]: [MySQL] 59,583 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new24.19 KB

fair bit more code to be focused on relatively small niggling details, but, here's what we got.

  • adds tests for most of BaseAggregateAsset, excluding load() and dump().
  • fixes some implementation issues in both aggregates and collections around handling uniqueness, and improves the tests accordingly.
  • it also improves test annotations for BaseAggregateAssetTest and AssetCollectionTest.
  • adds an additional test failure on BaseAssetAggregateTest::isEmpty(); that method should return empty if the only contents are empty aggregates. Not too hard to fix.

believe it or not, we're actually getting close here. now at 6 expected test failures.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-117.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new93.29 KB
new175.49 KB
FAILED: [[SimpleTest]]: [MySQL] 59,484 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

this pass does a ton.

  • implements BaseAssetAggregateTest::isEmpty() correctly, fixing the new failure from the last patch.
  • fixes recursion in the removeLeaf(), replaceLeaf() methods to work correctly with grace mode (recursing calls should always operate with grace, otherwise exceptions are thrown prematurely).
  • introduces iterator methods onto AssetAggregateInterface - each() and eachLeaf(). these return about what you'd expect - the former iterates across all contained assets, and the latter across only terminal/non-collection assets.
  • all() moved up to AssetCollectionBasicInterface.
  • AssetCollectionBasicInterface now extends \Countable.
  • MOST significantly, the whole class tree around collections got refactored. previously, BaseAggregateAsset was extending off the leaf asset hierarchy, ultimately having Assetic's BaseAsset as its parent. but as i was working on all this, i realized that truly enforcing the collection requirements was enough complex code that it would be better to have it done once in a parent (well, really, in a trait, but...), and then have all the collections (aggregate, collection, library) extend that. this also helps with the fact that the set of methods appropriate for collections is slightly disjoint with that of libraries. so, there is now a BasicAssetCollection that implements AssetCollectionBasicInterface which is the parent for aggregates and collections, and will be for libraries once i get back round to them. that parent class is 100% unit tested, which is really great, as that's where all the complex collection-handling logic lies - nesting, searching, iterating, etc.
  • renamed BaseAggregateAsset to AssetAggregate, and got rid of CssAggregate and JsAggregate. because...
  • implemented asset typing restrictions within AssetAggregate. this means that a css aggregate will aggressively ensure that it only contains css assets - no need for a subclass, as that's the metadata's purview, anyway.

for a general sense of scope, this patch took the asset test numbers from 118 tests and 316 assertions to 148 tests and 579 assertions (that's 100% phpunit). some of that is redundancy from a test inheritance scheme that mirrors the BasicAssetCollection family tree, but still - lots. i'm now pretty confident that the collections behaviors are settled. that's a big win.

Issue summary:View changes

mark "basic" unit tests for BaseAssetAggregate as complete.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-119.patch, failed testing.

StatusFileSize
new24.5 KB
new197.24 KB
FAILED: [[SimpleTest]]: [MySQL] 59,170 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

quick pass this time.

  • reintroduces AssetLibraryRepository and AssetLibraryCollector, the latter of which was renamed to AssetLibraryFactory. these two are massively cleaned up from their ugly state before; they're halfway decent now.
  • i got rid of the two-part string BS we use for libraries right now (e.g., system, jquery). it's still in two parts, it's just joined together around a colon.
  • (re)introduces the tests for both of these, as well. the former's tests are failing until the class is fixed (yay TDD), the latter is passing with 100% coverage.

i didn't tackle the dependency resolution step here, but that's next. once that's done, well...

HOLY SHIT!

i'll actually be able to wire something real into drupal_process_attached(), which means this puppy can start to take flight.

Status:Needs work» Needs review

Issue summary:View changes

mark of plan for colletion ids - that's now in hand, even if it means reindexing

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

The last submitted patch, assets-1762204-121.patch, failed testing.

Status:Needs work» Needs review

#121: assets-1762204-121.patch queued for re-testing.

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

The last submitted patch, assets-1762204-121.patch, failed testing.

sounds exciting :) any chance we can use a / instead of a colon for separating the lib reference? just thinking ahead for AMD and #1996238: Replace hook_library_info() by *.libraries.yml file.

ahhhh, a comment other than me!! :)

yep, i see no reason that slash would be an issue. i'll change it in my next reroll.

A word of encouragement

See what they're planning for HTTP 2.0: https://docs.google.com/presentation/d/1eqae3OBCxwWswOsaWMAWRpqnmrVVrAfP...

In case that link doesn't take you to slide 15, jump to there.

As you can see from the slide, Http 2.0 will allow you to prioritize the delivery of assets. Which means we need a system-level process for assigning priority to assets. Which means we need this patch.

StatusFileSize
new197.2 KB
FAILED: [[SimpleTest]]: [MySQL] 60,016 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new8.52 KB

@cosmicdreams - yep, this patch will help with being able to take advantage of HTTP 2.0, for sure. the real kicker, though, will be a well-structured page model. with that, we could know ahead of time what the assets we'd need for a given page are (as they'd be contained within a cacheable object).

i'm quite new to SPDY/HTTP 2.0, but if i understand the model properly, then at the start of request processing we could immediately begin pipelining out assets, keeping the client busy with downloading those while we do normal serverside request processing. once we have HTML ready to send, we send that back down in a separate stream than any of the individual assets - some/most/all of which the browser has already loaded. the browser should know to prioritize that stream higher than others, so it'll complete receiving the HTML first, then return to receiving any (remaining) assets. hopefully, though, it'll have received most or all of the assets by the time we send the HTML, so that there'll be no additional latency beyond the initial page response time.

while this sending process is more complex, if we have a well-structured page, that really shouldn't be THAT much of a problem. and we can then completely get rid of aggregation as a concept, because (as Ilya points out in that presentation) it's a crappy hack in the first place. yay.

back to the patch. this pass isn't too big:

  • switches the library key delimiter from a colon to a slash, per #126.
  • gets all existing tests passing for AssetLibraryRepository. this is only about 50% coverage though, so there's more to go.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, assets-1762204-129.patch, failed testing.

Completely irrelevant to any actual work done here, but I want to just join #94, #126 and #128 by saying that this is indeed exceptionally awesome work :).

Status:Needs work» Needs review
StatusFileSize
new224.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-133.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

#132: =)

even though the issue has been dormant for a week, the code has not - quite evident from the github repo. this is a rather massive update that brings us within spitting distance of having the new system take care of CSS. here's the rundown:

  • AssetLibraryRepository::resolveDependencies() now works, with exhaustive tests. srsly, the first unit test (yes, unit) i wrote for this was 125 lines long. yay, mocks.
  • AssetCollectionBasicInterface::getById() renamed to ::find(). shorter, and more in line with convention for this sort of behavior.
  • AssetCollectionInterface::sort() renamed to ::uksort(), in keeping with the general pattern. fwiw, i don't actually like basing this on php's naming (that naming is not great, and it exposes implementation details). i think the naming should be based on a more domain-specific concept, but i haven't settled on the right thing yet.
  • AssetCollectionInterface has added several methods for handling declaration of libraries directly on the collection itself (as opposed to being attached to a particular asset): ::addUnresolvedLibrary(), ::hasUnresolvedLibraries(), ::clearUnresolvedLibraries(), ::getUnresolvedLibraries(). this is distinct from DependencyInterface because the collection itself does not have dependencies, as collections themselves are not [logical] assets.
  • updated core.services.yml with a number of additional services.
  • there is now an icky-and-hacky-but-functional layer in drupal_process_attached() and the various drupal_add*() functions that sends declared assets to a global collection/collector combo, so that we can use them in the new system.
  • dots are now allowed in library names, as they should always have been.
  • AssetLibraryFactory has a little out to avoid having to deal with js settings - we're deferring all the js stuff till after CSS is working.
  • added AssetCollectionInterface::reverse(), which reverses the current order of assets in the collection.
  • lots of verification around correct handling and propagation of dependencies.
  • refactored AssetGraph::transpose(); it was traversing using the wrong graph method, causing it to incorrectly drop vertices with degree of 0.
  • collection freezing was made more robust, especially the errors (the exception now identifies by name the disallowed method that was called).
  • AssetCollector was incorrectly including js assets in its auto-after(). this is now fixed.
  • THERE ARE NOW INTEGRATION TESTS FOR MY GRAPH GROUPSORTING ALGORITHM. this is a really big deal, because it suggests that the algorithm i designed is correct (enough). ...and because, for the last week or so, it was giving me enough trouble that i was starting to think i'd messed it up somewhere. of course, this does not rise to the level of a true proof.
  • a note - in CssGraphSorter, if we could get the vertex reach data we need without doing transposition, we could probably save some cycles by not having to transpose at all and performing the sort on the original graph, and not having to reverse() the result.

as of right now, the new system is capturing 35 of the 36 CSS assets that normally appear. all that's missing is core/modules/edit/css/edit.css, which is added via a hook_library_info_alter() call, indicating that i likely just forgot to invoke the alter back in AssetLibraryFactory.

once that declaration vector is captured correctly, we just need #2114165: Add mrclay/minify vendor library to core to be able to take CSS *ALL* the way. hold on to your butts, people.

no interdiff b/c this included a conflict resolution from a core merge.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-133.patch, failed testing.

Issue summary:View changes

reintroduced AssetLibraryRepository

Status:Needs work» Needs review
StatusFileSize
new224.49 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

goddamn core.services.yml reconciliation. testbot also complained about common.inc, but i didn't see any issues with that. hopefully this will work.

Status:Needs review» Needs work

The last submitted patch, assets-1762204-135.patch, failed testing.

hmmm...well, that's better than a failure in applying the patch.

i guess this makes some sense, i was seeing some BS on my local installer around failures like this, too. i'll tackle it later.

fwiw, if anybody is intending to review this (which it could really, really use at this point), then that failure should not deter you - you should be able to apply it to an existing, running site and have things run fine on a container rebuild (which will be needed). more importantly, though, just look at the tests. they're all unit tests, anyway.

Status:Needs work» Needs review
StatusFileSize
new67.96 KB
new12.45 KB
new231.35 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

next pass. four things done, here:

  • hopefully it fixes the testbot installation failures from before. i don't know if it will, though, as i wasn't seeing failures on my local.
  • invoke the alter from AssetLibraryFactory, per the note at the end of #133.
  • brings CssCollectionAggregator up to date (it had an outdated class reference).
  • introdue CssCollectionRendererNouveaux, and put it on the non-optimizing CSS rendering path. yep, that's right - this is the first patch that officially outputs something through the new system. HOT DIGGITY DAMN. there aren't tests for this just yet - i just wanted to get it up.

now, there are immediately visible, obvious problems with the output. i'm (sorta) hoping that they stem from incorrectly declared CSS dependencies, as that'd mean just cleaning those things up elsewhere, rather than it being a problem here. but here's where i could REALLY start using other peoples' input - assuming the system i've built is operating correctly, then help would be very much appreciated around figuring out what's declared incorrectly.

for anyone wanting to help, here's a starting point. when logged in as uid 1 and hitting the frontpage on a plain D8 site, this is the set of assets and their order produced by the old system:

Array (
     [0] => normalize.css
     [1] => system.module.css
     [2] => field.module.css
     [3] => user.module.css
     [4] => system.theme.css
     [5] => views.module.css
     [6] => contextual.module.css
     [7] => contextual.theme.css
     [8] => contextual.icons.css
     [9] => jquery.ui.core.css
     [10] => jquery.ui.theme.css
     [11] => jquery.ui.button.css
     [12] => jquery.ui.resizable.css
     [13] => jquery.ui.dialog.css
     [14] => edit.module.css
     [15] => edit.theme.css
     [16] => edit.icons.css
     [17] => edit.css
     [18] => filter.caption.css
     [19] => shortcut.module.css
     [20] => shortcut.theme.css
     [21] => shortcut.icons.css
     [22] => contextual.toolbar.css
     [23] => joyride-2.0.3.css
     [24] => tour.module.css
     [25] => user.icons.css
     [26] => toolbar.menu.css
     [27] => toolbar.module.css
     [28] => toolbar.theme.css
     [29] => toolbar.icons.css
     [30] => dialog.theme.css
     [31] => layout.css
     [32] => style.css
     [33] => colors.css
     [34] => print.css
)

and this is the set produced by the new system. yes, we have full paths in the new system.

Array (
    [0] => core/modules/field/css/field.module.css
    [1] => core/modules/system/css/system.module.css
    [2] => core/modules/system/css/system.theme.css
    [3] => core/modules/user/css/user.module.css
    [4] => core/assets/vendor/normalize-css/normalize.css
    [5] => core/themes/bartik/css/colors.css
    [6] => core/themes/bartik/css/print.css
    [7] => core/themes/bartik/css/layout.css
    [8] => core/themes/bartik/css/style.css
    [9] => core/assets/vendor/jquery.ui/themes/base/jquery.ui.core.css
    [10] => core/assets/vendor/jquery.ui/themes/base/jquery.ui.button.css
    [11] => core/assets/vendor/jquery.ui/themes/base/jquery.ui.dialog.css
    [12] => core/misc/dialog.theme.css
    [13] => core/modules/edit/css/edit.icons.css
    [14] => core/themes/seven/edit.css
    [15] => core/assets/vendor/jquery.ui/themes/base/jquery.ui.resizable.css
    [16] => core/assets/vendor/jquery.ui/themes/base/jquery.ui.theme.css
    [17] => core/modules/contextual/css/contextual.icons.css
    [18] => core/modules/contextual/css/contextual.module.css
    [19] => core/modules/contextual/css/contextual.theme.css
    [20] => core/modules/contextual/css/contextual.toolbar.css
    [21] => core/modules/edit/css/edit.module.css
    [22] => core/modules/edit/css/edit.theme.css
    [23] => core/modules/filter/css/filter.caption.css
    [24] => core/modules/shortcut/css/shortcut.icons.css
    [25] => core/modules/shortcut/css/shortcut.module.css
    [26] => core/modules/shortcut/css/shortcut.theme.css
    [27] => core/modules/toolbar/css/toolbar.icons.css
    [28] => core/modules/toolbar/css/toolbar.menu.css
    [29] => core/modules/toolbar/css/toolbar.module.css
    [30] => core/modules/toolbar/css/toolbar.theme.css
    [31] => core/modules/tour/css/joyride-2.0.3.css
    [32] => core/modules/tour/css/tour.module.css
    [33] => core/modules/user/css/user.icons.css
    [34] => core/modules/views/css/views.module.css
)

and, for the lookie-loo, here's a screenie of the homepage when rendered through this:
broke-ass-css.png

Status:Needs review» Needs work

The last submitted patch, assets-1762204-138.patch, failed testing.

turns out, the new system is causing some errors for the web installer. of course, i have no idea what the errors are because we're throwing an exception from within the __toString(). fucking drupal|php. time to track THAT down...

StatusFileSize
new1.38 KB
new232.73 KB
FAILED: [[SimpleTest]]: [MySQL] 58,689 pass(es), 267 fail(s), and 8 exception(s).
[ View ]

found it. needed to update the installer to include manual registration for some of these new services - the graph sorter, the new css renderer, the asset library factory, and the asset library repository.

we should be back to 5 phpunit failures, now...let's hope?

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, assets-1762204-141.patch, failed testing.

Status:Needs work» Needs review

oooh, ACTUAL failures.

well, JUST glancing through the failures list (not looking at the tests), there's a subset of those that we might actually need to care about now:

  • ColorTest that checks for the presence of its stylesheets.
  • why something weird is getting passed to htmlspecialchars() in DbLogTest.
  • EditorLoadingTest is failing to find some of its expected assets.
  • Ajax\FrameworkTest has some failures around CSS being present and ordered correctly.
  • Ajax\DialogTest points to, i think, something that i may have critically missed about adding CSS assets on subsequent page requests. i think this might share a root cause with the failing EditLoadingTest, and ViewAjaxTest.
  • ThemeInfoStylesTest is just plain definitely broken.

again, i just glanced through. this is far from a complete list - i'm just hoping it's enough that people could maybe hop in and investigate.

note that, for a lot of these, i'd hope we could remove the tests entirely and replace with unit tests. the answer in each of these cases may not be easy, depending on what's being done; it might require me doing some 'splaining/figuring to decide how these things fit in with the new system.

Priority:Major» Critical
Issue tags:+frontend performance

and...bumping to critical, on the basis that we need to get this to the point where we can really make performance benefit comparisons.

Issue summary:View changes

ticked off several major todos. woot.

Issue summary:View changes

Issue summary:View changes

Issue summary:View changes

Issue summary:View changes

oook, another pass. this was mostly internals, again.

  • introduced fluency into the interfaces all over the place - basically, anything that was returning void is now returning $this.
  • cleaved AssetAggregateTest and AssetCollectionTest from the shared parent BasicAssetCollectionTest. it was a lot of redundancy for very little value.
  • a full pass over DependencyInterface and RelativePositionInterface and their implementations (libraries and base assets); cleaned up a lot, and added a ton of proper tests.
  • also made DependencyInterface no longer inherit from RelativePositionInterface. interface segregation principle aside, the two really are distinct.
  • brought test coverage for AssetLibrary up to >95%.

Status:Needs review» Needs work

The last submitted patch, 150: assets-1762204-150.patch, failed testing.

StatusFileSize
new247.01 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-173.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new39.79 KB

Wow. Awesome progress here! Thank you, sdboyer!


Reasons for this patch

I re-read the entire issue, to come up with a list of reasons why this patch is changing our asset handling almost entirely:

  1. Get rid of globals.
  2. Major SCOTCH blocker.
  3. Much better CSS and JS aggregation & minification become possible: Assetic has many filters, and we provide better metadata. (And we can finally ship with JS minification enabled!)
  4. #71: "optimum minimal grouping".
  5. #128: "HTTP 2.0"
  6. Points 3 and 4 can lead to significantly improved front-end performance. Point 5 as well, but that's not yet a market reality.

All that, with no API changes, besides 'weight' becoming ignored.

High-level questions

After re-reading the entire issue, I came up with the following six questions (i.e. without having seen the code):

  1. Why no before()/after() for libraries? That can also be necessary, even though it'd usually only apply to its JS.
  2. What's the before()/after() syntax for #attached?
  3. Renaming "groupers" to "aggregators" seems bad. "Aggregation" is a very overloaded term in the asset world: some see it solely as (the logical) "grouping", some see it as generating the aggregates themselves, and hence consider minification a part of it.
  4. What's the performance of this new system? Have you done any profiling, or do you have plans to do so? I'm refering to PHP CPU performance and memory consumption.
  5. You now use full paths to identify assets, but at some point in time, for reasons I don't know, it was decided that just the filename of a CSS file would uniquely identify it in ajaxPageState. So why this change (besides being more logical)? How do you plan to update ajaxPageState?
  6. How does this affect render cache? Right now, render cache stores array('#markup' => '<p>Hi!</p>', '#attached' => array(…)) and runs drupal_process_attached(), which works because it uses a global. I'm sure it's possible to make it work, but it might need some moving around of things to get it all to work.

And while doing the code review below, I came up with two more questions:

  1. How much of Assetic do we leverage? We're reimplementing a lot due to the overly simplistic/inappropriate assumptions that Assetic makes. It seems like almost the only thing we use from Assetic is its filters?
    Plus, it looks like e.g. Assetic's CSS minifier is barely worthwhile to use (#2114165-10: Add mrclay/minify vendor library to core).
    Does that mean we should reconsider using Assetic at all?
  2. Overall: how can we make the architecture simpler? Or does the architecture that you came up with simply reflect the inherent complexity of a modular system's asset handling?
    My gut feeling tells me we shouldn't use Assetic (see above), but I hope that's wrong.

Code review

I've tried to focus on the CSS aspect, because that is the part that is closest to completion. In doing so, I of course also reviewed the parts that are the same for CSS/JS.

  1. +  $rendered = \Drupal::service('asset.css.collection_renderer_nv')->render($sorted_css);
    +
       // Aggregate the CSS if necessary, but only during normal site operation.
       if (!defined('MAINTENANCE_MODE') && \Drupal::config('system.performance')->get('css.preprocess')) {
         $css_assets = \Drupal::service('asset.css.collection_optimizer')->optimize($css_assets);
    +    return \Drupal::service('asset.css.collection_renderer')->render($css_assets);
       }
    -  return \Drupal::service('asset.css.collection_renderer')->render($css_assets);
    +
    +  return $rendered;
    }

    I think it's better to follow the original logic that was there. That was done intentionally: it avoids rendering twice — in the current patch, when aggregation is enabled, it will first render the non-aggregated assets (but never use that) and then render the aggregated assets.
  2. Isn't AssetInterface::getAssetType() overly verbose? Wouldn't AssetInterface::getType() suffice?
  3. /**
    * Describes assets that can declare dependencies on asset libraries.
    */
    interface DependencyInterface {

    BaseAsset implements DependencyInterface, which means that any individual CSS/JS asset can declare a dependency on a library. That is definitely an API addition. But then again, I suspect that it in practice does not cause an API addition because this cannot be used from #attached.
    Nevertheless: why? Is it even intentional? My best guess is that this is necessary for when an asset library is expanded into its individual assets, so that it's possible to track the combined dependencies of individual assets, which in turn would allow for more precise/informed grouping? Or rather, it is necessary to be able to create a graph out of all assets at all, so we can do the proper dependency tracking — but in that case, I'd think dependencies on other specific assets would be tracked rather than on asset libraries.
    I think it should be explained here why individual assets can depend on asset libraries, to remove all doubt.
    In general, it would be conceptually simpler if asset libraries could only depend on other asset libraries and if individual assets can only depend on other individual assets.
  4. Minor: RelativePositionInterface's before() and after()'s parameter flexibility are unfortunate because it prevents us from typehinting:

       * @param string|\Drupal\Core\Asset\AssetInterface $asset

    Is the string ID variant truly necessary?
  5. There is a terminology discrepancy between the "ordering" verb that is used almost everywhere in the code and the RelativePositionInterface that provides that ordering functionality. I think that discrepancy should be fixed. I think you're in the best position to judge whether it should be "order" or "relative position", sdboyer.
  6. Why doesn't AssetInterface simply extend not only AsseticAssetInterface, but also DependencyInterface and RelativePositionInterface?
  7. Why is an asset's type ('js' or 'css) stored both in AssetInterface and the AssetMetadataInterface that every object implementing AssetInterface must hold?
  8. Why not demand that a string asset not only is a string but is also non-empty? That way, you don't have to generate a random ID if it's empty. I think it's only reasonable to ensure that a string asset is non-empty?
      public function __construct(AssetMetadataInterface $metadata, $content, $filters = array()) {
        if (!is_string($content)) {
          throw new \InvalidArgumentException('StringAsset requires a string for its content.');
        }
        $this->id= empty($content) ? Crypt::randomBytes(32) : hash('sha256', $content);
        …
  9. public function load(FilterInterface $additionalFilter = NULL) {
        // TODO very wrong. decide how to do this right.
        throw new UnsupportedAsseticBehaviorException('Drupal does not support the retrieval or manipulation of remote assets.');
      }

    So Assetic's HttpAsset just retrieves the remote file and aggregates it. Interesting, particularly because that means it's super easy for sites to fail once that external asset goes offline. Very bad. That's why Drupal doesn't do it, and you rightfully throw an exception.
    I think what you have there right now is totally fine. I think ExternalAsset should simply hardcode isPreprocessable() to returning FALSE. Drupal will listen to that and not call load(), and if it does, then an exception is appropriate.
    (I think this would be a cleaner implementation than what we currently have in CssCollectionGrouper::group() too, where the grouper needs to special case external assets, rather than the asset itself providing the appropriate hint/instruction/metadata.)
  10. The getLastModified() methods that are yet to be implemented for FileAsset and StringAsset … are difficult to solve. I doubt they are solvable problems, unless we start to maintain a (potentially massive & expensive) history of previous values.
    The only reason we have that in this patch, is because Assetic has it. Assetic uses it to 1) build a cache key (see AssetCache::getCacheKey()), 2) do cache busting (see CacheBustingWorker::getHash())). Never does it use it for time purposes. It's effectively used in the same way as we would use an MD5 hash. So, I'd consider this a pretty big design flaw in Assetic, which we can't solve, unless we don't (and won't ever) use either of those two classes, in which case we can just ignore it.
  11. AssetCollector:
    • Appears to be very complex.
    • Its restoreDefaults() is no longer an accurate name AFAICT.
    • It's not clear to me why getMetadataDefaults() is public.
    • Its locking functionality is not used at all AFAICT.
    • clearLastCss() feels like a leaky abstraction? Shouldn't it be the responsibility of the caller of AssetCollector::create() to pass the previous asset, if any?
  12. In AssetCollectionBasicInterface:
       * @param AsseticAssetInterface $asset
       *   The asset to add. Note that, despite the type requirements, it must
       *   conform to Drupal's AssetInterface.

    Then why not simply use Drupal's AssetInterface, rather than Assetic's?
  13. The whole collection (heh…) of "collection" interfaces & classes is… incredibly complex. And then the aggregate classes use that too. To even begin to understand it, I had to draw a diagram, which I tried to make an ASCII diagram of to help others :D That diagram is here:
                                                                     +--------------------------+  extends  +-----------------------+  implements  +------------------+
                                  +--------------------------------->| AssetCollectionInterface +---------->| AsseticAssetInterface |<------------ + AsseticBaseAsset |
       ASSETIC                    |                                  +--------------------------+           +-----------------------+              +------------------+
                                  |                                                                                             ^                         ^
    +-----------------------------|---------------------------------------------------------------------------------------------|-------------------------|-----------------------------+
    +-----------------------------|---------------------------------------------------------------------------------------------|-------------------------|-----------------------------+
                                  |                                                                                             |                         |
       DRUPAL                     |                                                                                             |                         |
                                  | extends                                                                                     |                         |
                                  | (to use Assetic's filters)                                                                  | extends                 | extends
                                  |                                                                                             |                         | (to disable stuff)
                                  |                                                                                             |                         |
      +---------------------------|--------+                                                                                    |                         |
      |         ASSET AGGREGATES  |        |                                                                     +--------------|-------------------------|--------------+
      |---------------------------|--------|                                                                     |              |   INDIVIDUAL ASSETS     |              |
      |                           |        |                                                                     |--------------|-------------------------|--------------|
      |                           |        |                                                                     |              |                         |              |
      |    +----------------------+--+     |                         extends                                     |              |                         |              |
      |    | AssetAggregateInterface +-------------------------------------------------------------------------------->+--------+-------+     +-----------+---------+    |
      |    ++------------------------+     |                                                                     |     | AssetInterface |     | AsseticAdapterAsset |    |
      |     |          ^                   |   +---------------------------------------------------------------------->+----------------+     +---------------------+    |
      |     |          |                   |   |                    implements                                   |       ^                                ^              |
      |     |          |                   |   |                                                                 |       |                                |              |
      |     |          | implements        |   |                                                                 |       |                                | extends      |
      |     |          |                   |   |                                                                 |       |                                |              |
      |     |          |                   |   |                                                                 |       |                                |              |
      |     |          |                   |   |                                                                 |       |                                |              |
      |     |  +-------+--------+          |   |                                                                 |       |                                |              |
      |     |  | AssetAggregate +--------------+                                                                 |       |  implements  +-----------+     |              |
      |     |  +-------+--------+          |              +------------------------------+-------------------------------+--------------+ BaseAsset +-----+              |
      |     |          |                   |              |                              |                       |                      +-----------+                    |
      |     |          |                   |              |                              |                       |                       ^         ^                     |
      +-----|----------|-------------------+              v                              v                       |    +---------------+  | extends |   +-----------+     |
            |          |                        ---------------------+     +---------------------------+         |    | ExternalAsset +--+         +--+| FileAsset |     |
            |          |                       | DependencyInterface |     | RelativePositionInterface |         |    +---------------+                +-----------+     |
            |          |                       +---------------------+     +---------------------------+         |                                                       |
            |          |                                  ^                              ^                       +-------------------------------------------------------+
            |          |                                  |                              |
            |          |                                  |                              |
            |          |                                  +------------------------------+-----------------------------------------------------------------------------------+
            |          |                                                                                                                                                     |
            |          |                                                                                                                                                     |
            |          |                                                                                                                                                     |
            |          |                                                                                                                                                     |
            |          |                                                                                                                                                     |
            |          |                                                                               +-----------------------------------------------------------------+   |
            |          |                                                                               |                       ASSET COLLECTIONS                         |   |
            |          |                                                                               |-----------------------------------------------------------------|   |
            |          |                                                                               |                                                                 |   |
            |          |                                                                               |                                                                 |   |
            |          |                                   implements                                  |                +-------------------------------+                |   |
            +----------|----------------------------------------------------------------------------------------------->| AssetCollectionBasicInterface |                |   |
                       |                                                                               |                +-------------------------------+                |   |
                       |                                                                               |                       ^               ^                         |   |
                       |                                                                               |            implements |               | extends                 |   |
                       |                                                                               |                       |               |                         |   |
                       |                                    extends                                    |     +----------------------+   +--------------------------+     |   |
                       +------------------------------------------------------------------------------------>| BasicAssetCollection |   | AssetCollectionInterface |     |   |
                                                                                                       |     +----------------------+   +--------------------------+     |   |
                                                                                                       |               ^   (abstract)                  ^                 |   |
                                                                                                       |               |                               |                 |   |
                                                                                                       |       extends |                               | implements      |   |
                                                                                                       |               |       +-----------------+     |                 |   |
                                                                                                       |               +-------+ AssetCollection +-----+                 |   |
                                                                                                       |                       +-----------------+                       |   |
                                                                                                       |                                ^                                |   |
                                                                                                       |                                |                                |   |
                                                                                                       |                                | extends                        |   |
                                                                                                       |                                |                                |   |
                                                                                                       |                        +-------+------+      implements         |   |
                                                                                                       |                        | AssetLibrary +-----------------------------+
                                                                                                       |                        +--------------+                         |
                                                                                                       |                                                                 |
                                                                                                       |                                                                 |
                                                                                                       +-----------------------------------------------------------------+

    The "individual assets" stuff in the top-right makes sense. The "asset collections" stuff in the bottom right less so: the distinction between a basic and a regular asset collection feels bizarre. An asset library is a regular asset collection, but with ordering and dependency information. Okay. (That's why RelativePositionInterface mentions it's "for assets and asset-like objects", an asset library is "asset-like".)
    But then when you add the "asset aggregates" stuff on the left, things become extremely difficult to grok IMHO: the AssetAggregateInterface extends Drupal's AssetInterface, Drupal's AssetCollectionBasicInterface (making it both an individual asset and an asset collection from a Drupal POV!) and Assetic's AssetCollectionInterface. Similar things then logically also apply to AssetAggregate.
    Why can't a simple AssetCollection fulfill the purpose of AssetAggregate? AFAICT that's because to use Assetic's filters, it must implement Assetic's AssetCollectionInterface. But couldn't that be solved much more simply, by letting Drupal's AssetCollectionInterface extend Assetic's identically named interface? Or does that bring downsides that I'm missing?

  14. There should be a AssetLibraryRepositoryInterface?
  15. In AssetLibraryRepository::resolveDependencies():
            // Only bother attaching if operating on an asset.
            if ($attach && $asset instanceof RelativePositionInterface) {
              foreach ($library as $libasset) {
                if ($asset instanceof AssetInterface &&
                    $asset->getAssetType() !== $libasset->getAssetType()) {
                  continue;
                }
                $asset->after($libasset);
              }
            }
    • The docs say "if operating on an asset", then why should we check "if instanceof RelativePositionInterface"?
    • The docs probably should probably say "if operating on an asset of the same type".
    • It's calling ->after(), which is about order/relative position, not dependencies, so shouldn't that be a call to addDependency() instead?
  16. CssGraphSorter::groupAndSort() receives "the full asset collection", rather than just the CSS assets, it retrieves just the CSS files itself by calling ->getCss() on the received collection. Wouldn't it be easier for the reader of the code to do
    $sorted_css = $sorter->groupAndSort($collection->getCss());
    rather than
    $sorted_css = $sorter->groupAndSort($collection);
  17. CssGraphSorter::getGroupingKey() should check the asset's isPreprocessable() return value, and if it's FALSE, exclude it. This would help you to not have to special case ExternalAsset there.
    Furthermore, I think we should strive to not have any instanceof SomeTypeOfAsset logic in there; we should be able to get metadata that we want to use for the grouping key, and if some metadata is missing, we just don't use that. That would simplify this code a lot, and it opens the door for potentially adding new asset types later on… (though I can't think of a use case for that).
    This would also allow you to move groupAndSort() to abstract AssetGraphSorter and reuse it for the JsGraphSorter, further reducing code duplication.
  18. Can you rephrase this? It's hard to understand, at least to a non-native speaker:
    * The impact of a dependency can be calculated myopically (without knowledge of
    * the full set), as a dependency inherently guarantees the presence of the
    * other vertex in the set.
  19. Shouldn't AssetGraph::transpose() also remove the original directed edges? Right now, AFAICT, it is effectively turning a directed graph into an undirected one?
  20. Inside CssGraphSorter::groupAndSort(), the docs imply that the grouping key serves to achieve optimal groups. This is true to some extent, but e.g. in the case of the "browsers" metadata that also ends up in the grouping key, it is a matter of correctness, not optimality.
  21. AssetGraph adds directed edges in the "depends on" direction, i.e. if asset B depends on asset A, then there will be a directed edge from B to A. There's no problem with that. But if you'd invert that, couldn't you then avoid the transponse() operation in CssGraphSorter::groupAndSort() altogether?
    AFAICT that's the only place where we'll use AssetGraph, so I think it's fine to invert the direction and make it "is a dependency of".
  22. I think in places such as CssGraphSorter::groupAndSort(), we could all benefit from some ASCII art :) :D I think things like
        // Create a queue of start vertices to prime the traversal.
        $queue = $this->createSourceQueue($transpose);
        // Now, create the visitor and walk the graph to get an optimal TSL.
        $visitor = new OptimallyGroupedTSLVisitor($optimal, $optimal_lookup);
        DepthFirst::traverse($transpose, $visitor, $queue);
        $final = $visitor->getTSL();
        $final->reverse();
        return $final;
    will be incomprehensible to most people. Inline ASCII art would do wonders in explaining this. I already started to add some stuff, but please improve and extend that.

Manual testing

I reinstalled Drupal with this patch applied. Installation worked fine. I got to the "Welcome to this site" frontpage while anonymous after the installation just fine — with the same rendering problems (missing/incorrectly ordered CSS) as you've shown in #138.

But then, when I log in, I get this:

( ! ) Fatal error: Maximum function nesting level of '100' reached, aborting! in /Users/wim.leers/Work/drupal-tres/core/vendor/sdboyer/gliph/src/Gliph/Traversal/DepthFirst.php on line 58

Reroll

I fixed a whole bunch of nitpicks in this reroll. I also added some docs (that you should double-check) and a few TODO's (mostly about incomplete docs).

ROFL — in the patch I mark it as being associated with comment 173, because that is what d.o told me in the preview. Turns out that was … quite a bit off! :P

Status:Needs work» Needs review
StatusFileSize
new243.84 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new12.27 KB

WimLeers+++++++++++++++++++++++++++++++++++++++++

diving right in. i'm gonna break this up a tad in order to accelerate my rate of response, and make it seem less monolithic for other people.

  1. Why no before()/after() for libraries? That can also be necessary, even though it'd usually only apply to its JS.
  2. What's the before()/after() syntax for #attached?

we talked about this a bit in IRC, but until the patch i've posted just here, it WAS possible to do before() and after() on libraries. this was wrong.

apparently i pointed this out somewhere in #66, but to explain this properly, let's step back for a sec. there's a really key conceptual point here that needs to be made more front and center. the central asset system introduced by this patch has two types of things:

  • Assets - things that are directly renderable to a SINGLE HTML element. so, a link tag, a style tag, or if we're accommodating IE9, an @import inside a style tag. this includes the obvious - actual files - but also includes aggregates. aggregates contain multiple other assets, but are ultimately concatenated and dumped to disk as a single file - and are therefore renderable to a single HTML tag. again, the absolute, non-negotiable requirement here is that, from a renderable HTML perspective, these are a SINGLE thing. Things that are assets in this system:
    • BaseAsset
    • FileAsset
    • ExternalAsset
    • StringAsset
    • AssetAggregate
  • Asset Collections - collections are sets of assets. it is possible that some of them may ultimately be renderable as a single HTML tag - if they have only one contained asset, or if they have a few assets of the same type with the same metadata that allows them to be made into an aggregate. but this is not NECESSARILY the case for collections, as it is with individual assets.Things that are collections in this system:
    • AssetCollection
    • AssetLibrary

back to the original question: libraries are collections, and dependencies (as expressed via DependencyInterface) operate at either the collection or individual asset level. RelativePositionInterface, which defines after() and before(), expresses a relationship between individual assets, and individual assets only.

now, having done all that, i tried to write up a really nice semantic modelling distinction between why we have positioning at the individual asset level, but not the collection level. but i couldn't. maybe that points to a problem. but what it comes down to is this: the only thing that is expected to use the positioning data is a sorter, and it keeps things simpler if we are only working on individual assets by the time we make it into the graph sorter.

plus, all of the after()/before() stuff is translated onto libraries' contained assets, anyway. keeping it on the library is redundant.

Renaming "groupers" to "aggregators" seems bad. "Aggregation" is a very overloaded term in the asset world: some see it solely as (the logical) "grouping", some see it as generating the aggregates themselves, and hence consider minification a part of it.

yeah, i churned on this for a while. but i went with what i did because this actually *isn't* a renaming. there are three things that happen, or need to have happened, with this step:

  • sorting the assets
  • grouping the assets
  • creating aggregate representations of the grouped assets

in the old system, sorting was automatic and implicit. grouping and creating the aggregate representations were a conjoined operation.

in the new system, sorting is not automatic. sorting and grouping are actually the same operation. and creating the aggregates is done by iterating across the linear list created by the group sorter.

i balked at naming the thing that was creating aggregates a 'Grouper' because it wasn't actually doing any of the work of grouping anymore. it was just taking a list produced by the graph sorter and turning it into aggregates. sure, it was calling the group sorter (delegating), and so composition dictates we could still call it a grouper. but what it does there is trivial; we could actually eliminate that step by having the tsl visitor simply create the aggregates directly and return put those in the AssetCollection it creates, instead of just injecting them in one by one.

so - i wouldn't mind going back to the Grouper naming, but if we do then i'd prefer it to be on something that's actually *doing* grouping work. i have a thought on how we could do that and probably reduce some code duplication, too.

What's the performance of this new system? Have you done any profiling, or do you have plans to do so? I'm refering to PHP CPU performance and memory consumption.

i've done some basic profiling (and talked with catch and msonnabaum about it), but it's not yet in a position where it's easy to make comparisons against the current. i'm concerned about this, of course, but am still operating in the "make it right" mode, with the assumption that we can optimize later.

You now use full paths to identify assets, but at some point in time, for reasons I don't know, it was decided that just the filename of a CSS file would uniquely identify it in ajaxPageState. So why this change (besides being more logical)? How do you plan to update ajaxPageState?

tbh, i didn't even know this was a change until recently. it seemed...uh, obvious that we'd use the whole path as the id. but i went that route initially because it was in closest keeping with Assetic's API.

i don't have clear plans for ajaxPageState right now, mostly because i don't understand it (to the necessary degree of precision). that'd be an excellent thing for us to discuss know, and knowledge for me to gain so that we can put the plan together.

i'd be pretty surprised if it's hard to solve.

How does this affect render cache? Right now, render cache stores array('#markup' => '

Hi!

', '#attached' => array(…)) and runs drupal_process_attached(), which works because it uses a global. I'm sure it's possible to make it work, but it might need some moving around of things to get it all to work.

this was a before-you-read-the-code question, but the answer for the current patch is, "we dodge it." we have our own globals that we set up.

the longer-term fix for drupal_process_attached() is what we discussed in Prague - something that defers the actual processing of attached data until later, continuing to hand it back up the stack in its array form, and we designate a single point in the page rendering process where we snap all that up into the collector.

as for render cache, either we store a serialized version of an AssetCollection (making this easy is in my TODOs), or we store the unprocessed #attached array. i haven't thought about it too far beyond that, as i haven't really spent enough time with render caching to have a strong intuition.

How much of Assetic do we leverage? We're reimplementing a lot due to the overly simplistic/inappropriate assumptions that Assetic makes. It seems like almost the only thing we use from Assetic is its filters?
Plus, it looks like e.g. Assetic's CSS minifier is barely worthwhile to use (#2114165: Add mrclay/minify vendor library to core). Does that mean we should reconsider using Assetic at all?

right now, we use little to none of Assetic. the linked patch is what will allow me to utilize more of it - its filter system. and a follow-up that potentially lets us handle SASS, CoffeeScript, LESS, etc., would utilize more of those filters.

to be fair, though, that's not actually Assetic's CSS minifier; that's an external project. all Assetic has ever provided is simple filter implementations that connect with other projects, in PHP or otherwise.

the real argument to keep with Assetic, i think, is that i'm hoping a lot of what i've done here can serve as a basis for Assetic 2.0 - Kris indicated his interest in making assets inert and using more services to do the work. and we have *definitely* moved in that direction.

Overall: how can we make the architecture simpler? Or does the architecture that you came up with simply reflect the inherent complexity of a modular system's asset handling?
My gut feeling tells me we shouldn't use Assetic (see above), but I hope that's wrong.

well, there's a lot of parts to this architecture. removing Assetic would make AssetInterface simpler, but it wouldn't change the collections at all, and wouldn't change the services much.

i know that there's a lot of complexity here, but at the end of the day, i think this models the domain pretty much correctly. we could take artificial measures: combining interfaces, relaxing restrictions on certain collection behaviors - but doing so would violate SRP and interface segregation.

maybe we could even combining aggregates and collections together - and while it would work, there would be all sorts of things you COULD do that just wouldn't be correct. that would create a situation where the system drifts, over time, to allow murky hackiness in the system. i'm sure there's still some of that in what i've written, but i think making the rules clear and explicit, it could stand the test of time.

mostly, i think what this needs is a good diagram, and good docs. again, there's complexity here, but i think it's the minimum amount required to correctly model the domain, given a multi-actor system like Drupal.

i'll get to the code review items in my next response.

Status:Needs review» Needs work

The last submitted patch, 154: assets-1762204-154.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 154: assets-1762204-154.patch, failed testing.

1. I think it's better to follow the original logic that was there. That was done intentionally: it avoids rendering twice — in the current patch, when aggregation is enabled, it will first render the non-aggregated assets (but never use that) and then render the aggregated assets.

oh, heh. that's totally fine - that code is completely transitional, anyway, since it uses the old system for the optimized path and the new system for the optimized path. you're completely right; let's just chalk that up to that bit still being PoC.

2. Isn't AssetInterface::getAssetType() overly verbose? Wouldn't AssetInterface::getType() suffice?

possibly. i have it the way that it is though, because 'type' could reasonably be construed as the source type (file, external, string) by a naive developer. i'd be fine with changing it, though, if you think that's not an issue.

3. BaseAsset implements DependencyInterface, which means that any individual CSS/JS asset can declare a dependency on a library. That is definitely an API addition. But then again, I suspect that it in practice does not cause an API addition because this cannot be used from #attached.
...

IIINTERESTING. i hadn't thought of it this way, at all.

the problem (...is it?) with it is that it makes it impossible for, exactly as you say, an asset to have a dependency on a library. so in order to use jQuery, you have to create a library with your js asset named inside it, name the library, then declare from somewhere that you want that library to be included. that's a lot of steps, and i suppose i'd been operating under the assumption that for simple cases, it would be fine to allow directly declaring assets with library dependencies on them.

or, for a less convenience-oriented case - do we allow themes to declare libraries via the info hook? (can themes implement hooks yet?) if not, how would they express a dependency once the global functions go away?

honestly, i think that the potential for setting id-based before/after relationships directly on assets partially obviates the needs for libraries. not entirely - libraries are still useful in that they allow for the declaration of dependencies on a package, where that package includes a different type of asset (or some js settings). they also allow for those string-based lookups to actually work correctly in the case of actually having a hard *dependency* (as opposed to an optional one, which is how before/after currently and kinda have to work). but, still...food for thought.

4. Minor: RelativePositionInterface's before() and after()'s parameter flexibility are unfortunate because it prevents us from typehinting:

ah, you say minor, and yet, that's quite an intentional, major feature of how i've built this system :)

since we have a multi-actor system, and there is no global registry of objects representing the assets available to the system (only for libraries - not individual assets) there is no way that disconnected segments of code will have access to the same object instance for representing a particular asset. consequently, we cannot rely on object identity as the sole way of identifying an asset; string ids *must* be the primary way we traffic them.

i was disappointed, too. at first, i thought we could insulate the system and rely just on object identity for a lot of things.

of course, based on my answer to the previous question, it's possible we may want to discuss all this a bit...

5. There is a terminology discrepancy between the "ordering" verb that is used almost everywhere in the code and the RelativePositionInterface that provides that ordering functionality. I think that discrepancy should be fixed. I think you're in the best position to judge whether it should be "order" or "relative position", sdboyer.

yup, this highlights a recent refactoring; the logs tell me Oct. 15. DependencyInterface and RelativePositionInterface used to be one interface: AssetOrderingInterface. i was using 'ordering' as the general term then, and that was when i wrote a lot of the docs. the docs need another full pass to update them to utilize the "positioning" terminology instead, though i still think i like "ordering" as the general term that encompasses both interfaces. totally open to input there.

6. Why doesn't AssetInterface simply extend not only AsseticAssetInterface, but also DependencyInterface and RelativePositionInterface?

because they're different. even though the concrete implementation (BaseAsset) implements all three, i'd say that what makes a Drupal asset inherently different vs. an Assetic asset is a) the way that it fulfills Assetic's interface (functions used to grab content, how it handles load/dump, whether it can use vars, values, etc.). it is not intrinsic to a Drupal asset that it can declare positioning data or dependencies, and so it is not part of the interface.

there's a current use for this, and a possible one as well: right now, AssetAggregate does not allow for any kind of ordering relationship. this works well for the system as it is currently conceived, because aggregates should be created only by the system itself AFTER all of the dep resolution and sorting has been done. this may not actually be correct, though, if we want to permit the declaration of aggregates by the wider system. (...though i don't see a reason for doing so - let something else do your aggregate, then tell drupal it's a FileAsset - which is why i didn't support those methods on aggregates).

the hypothetical case is js settings. we haven't dealt with this at all yet, but i have a general plan for it - js settings are, when you think about it, really just a specialized type of StringAsset with known, specific metadata. we can add some methods to collections that allow them to carry around a single JsSettingsAsset, adding values to it incrementally, and that along to other collections it's merged into. there is no reason that JsSettingsAsset should be able to declare positioning or dependencies; it inherently can't have dependencies, and positioning should be determined by the system.

7. Why is an asset's type ('js' or 'css) stored both in AssetInterface and the AssetMetadataInterface that every object implementing AssetInterface must hold?

errr...it isn't? well, in the implementation, AssetInterface objects ask their metadata what their type is. returning to JsSettingsAsset, that's a case where it needn't ask the metadata, as it's clearly inherent to the asset class.

it's possible that this is a poor way of arranging things, but i think it makes more sense if you consider AssetInterface::getAssetType() to be akin to AssetInterface::isPreprocessable(): it's just a convenience for the caller to only have to type out one method instead of two.

now, if it seems weird that i'm comparing preprocessability (a Drupal concept) vs. asset type (clearly not a Drupal concept), yes, i still agree. but that's actually the tail of a larger problem, for which i have an outstanding TODO in the list:

Some thought needs to be given to the different types of metadata we track for assets, with specific mind to how "intrinsic" the datum actually is to the asset. This could factor significantly in contrib efforts to do sitewide optimal asset groupings. (definite followup)

so yeah, i'd prefer to answer that question in that wider context :)

8. Why not demand that a string asset not only is a string but is also non-empty? That way, you don't have to generate a random ID if it's empty. I think it's only reasonable to ensure that a string asset is non-empty?

good call. i've made the change locally, will push it up later.

9. So Assetic's HttpAsset just retrieves the remote file and aggregates it. Interesting, particularly because that means it's super easy for sites to fail once that external asset goes offline. Very bad. That's why Drupal doesn't do it, and you rightfully throw an exception.
I think what you have there right now is totally fine. I think ExternalAsset should simply hardcode isPreprocessable() to returning FALSE. Drupal will listen to that and not call load(), and if it does, then an exception is appropriate.

hmm, so i'm halfway in agreement with you here. isPreprocessable() should be hardcoded to FALSE in ExternalAsset. but it doesn't make sense to me to throw the exception; perhaps there are some circumstances where some particular site actually DOES want to retrieve external assets at this stage. i don't see a reason to hamstring those use cases by forcing them to actually swap out the class that's being used for something that can retrieve remote assets. we should be happy with just hardcoding the isPreprocessable() to FALSE, and ensuring that our core implementation therefore does not call those methods. and that the phpdoc on them warns people with appropriately scary language.

this is particularly true if we want to make the experience of utilizing other Assetic filters smooth - those are quite likely to call such methods. i'm already worried about the vars/values methods being overridden with exceptions for this reason.

10. The getLastModified() methods that are yet to be implemented for FileAsset and StringAsset … are difficult to solve. I doubt they are solvable problems, unless we start to maintain a (potentially massive & expensive) history of previous values.
The only reason we have that in this patch, is because Assetic has it. Assetic uses it to 1) build a cache key (see AssetCache::getCacheKey()), 2) do cache busting (see CacheBustingWorker::getHash())). Never does it use it for time purposes. It's effectively used in the same way as we would use an MD5 hash. So, I'd consider this a pretty big design flaw in Assetic, which we can't solve, unless we don't (and won't ever) use either of those two classes, in which case we can just ignore it.

i'm gonna assume you meant ExternalAsset, not FileAsset. to that end, i'll make basically the same argument as i did to the last question - it's at least *doable*, because we can make an HTTP request and use the Last-modified header.

string assets are what scare me, for sure. big time. looking back over the history of this issue, i see we discussed this nearly 100 comments back, too :) honestly, i don't have good ideas there, apart from something like recording a "time of last cache clear" and using that for all string assets. we certainly can't expose that information readily via the current API...ugh, maybe with a hack to the way we parse string assets in the collector...so, yeah, tricky.

so, maybe we can just ignore it. but i'd prefer not to - i think that its primary utility will be for non-request-time operations; the bigger, sitewide optimizing passes that we've occasionally talked about in hushed, hopeful tones.

11. AssetCollector:

i'll take these one at a time...

  • Appears to be very complex.

not quite sure how to quantify this - it is an object with at least two different kinds of state, but that's what you get with a traveling factory.

  • Its restoreDefaults() is no longer an accurate name AFAICT.

true, with the AssetMetadataFactory being used instead of directly cloning asset metadata objects, that method name should probably change.

  • It's not clear to me why getMetadataDefaults() is public.

a convenience thing, more than anything, i think - if you want to create your own asset directly and pass in metadata, having that be public means the collector can still help.

  • Its locking functionality is not used at all AFAICT.

not at present, because right now it's acting only as a simple factory. locking would be used for injecting the collector into blocks - the interface addition there has been included in this patch for months. that's one of those things, though, that i expect only princess to use. it doesn't hurt us to include it though, if there's a clear plan for its use (albeit not by core). right?

  • clearLastCss() feels like a leaky abstraction? Shouldn't it be the responsibility of the caller of AssetCollector::create() to pass the previous asset, if any?

hmmm...yeah, i think you might be right. that could be a better way to handle it. it gets rid of one bit of icky statefulness inside there.

12. In AssetCollectionBasicInterface:
* @param AsseticAssetInterface $asset
* The asset to add. Note that, despite the type requirements, it must
* conform to Drupal's AssetInterface.

Then why not simply use Drupal's AssetInterface, rather than Assetic's?

because then AssetAggregate couldn't implement the interface, as it would conflict with Assetic's AssetCollectionInterface, which AssetAggregate does implement.

on further reflection, though, that's one bit of interface reuse that we really may not benefit from very much at all. and it'd let us get rid of the ugly doubled-up replaceLeaf() vs. replace()and removeLeaf() vs. remove() methods on the aggregate. hmm....

ok, chunking again at this point; more later.

13. The whole collection (heh…) of "collection" interfaces & classes is… incredibly complex. [...] Why can't a simple AssetCollection fulfill the purpose of AssetAggregate? AFAICT that's because to use Assetic's filters, it must implement Assetic's AssetCollectionInterface. But couldn't that be solved much more simply, by letting Drupal's AssetCollectionInterface extend Assetic's identically named interface? Or does that bring downsides that I'm missing?

i've more or less given this response elsewhere, so i'll just reiterate - i think the essential model i've designed here is as simple as it can be while still meeting all requirements and describing the domain correctly. it ends up looking complex because there are a lot of discrete interfaces, and because we have to use inheritance instead of traits in certain places to reduce code duplication. but it gets the essentials right.

your description of AssetAggregate is absolutely correct - it is both a single logical asset, and a container for other assets. that's what an aggregate *is*, and that is different from a collection: it can only contain a single type of asset, it can't carry direct unresolved libraries, and it can't be sorted (because the order of its contents has meaning beyond convenience, as is the case in other collections).

so, could we get rid of aggregates and just use collections? sure. that's basically what Assetic does. but then,
collections would need to be able to store other collections directly, rather than just merging them, otherwise it would be impossible to encapsulate the work of creating an aggregate for later systems to use. alternatively, we'd have to make the CssGraphSorter::getGroupingKey() method into some sort of global service that everything is expected to use, repeatedly, in order to create the correct groups.

collections are a convenient way of passing around sets of asset data. aggregates are a way of encapsulating a specific subset of assets for grouped output. some shared behaviors, sure, but clearly separate responsibility.

14. There should be a AssetLibraryRepositoryInterface?

yep, just haven't extracted it yet. an AssetLibraryInterface as well.

15. In AssetLibraryRepository::resolveDependencies():

        // Only bother attaching if operating on an asset.
        if ($attach && $asset instanceof RelativePositionInterface) {
          foreach ($library as $libasset) {
            if ($asset instanceof AssetInterface &&
                $asset->getAssetType() !== $libasset->getAssetType()) {
              continue;
            }
            $asset->after($libasset);
          }
        }

bullet by bullet, now:

  • The docs say "if operating on an asset", then why should we check "if instanceof RelativePositionInterface"?

yeah, i recently changed that code path and forgot to update the doc there; it used to check AssetInterface, but i changed it to RelativePositionInterface.

  • The docs probably should probably say "if operating on an asset of the same type".

yeah, i'll add that.

  • It's calling ->after(), which is about order/relative position, not dependencies, so shouldn't that be a call to addDependency() instead?

no, that would be circular. this method is about resolving dependencies - dependencies inherently being the string id of a library, not a single asset. this is essentially a dereferencing operation: for the return value, we turn library string ids into an array of library object. but in this part of the loop, we're being helpful by dereferencing a step further by pulling the contained assets out of the library and setting up the after() relationship between the respective assets. and at the end of the day, it's that positioning relationship that is used to do the work, NOT the dependency relationship.

while having a declaratory layer where dependencies exists is great, when it comes to actually doing the work of sorting, grouping, and otherwise building a list of assets for output, the only thing i want to deal with is the assets themselves. libraries, or any other meta-structure that is not an individual asset, just make that process more complicated. that's why, in the current patch, we do this preparatory work out in drupal_pre_render_styles(), then pass the fully resolved collection into the services, where all they have to care about is the individual assets. it is not their job to do any ensuring that the collections have been fully resolved; they just take a collection of assets and do their thing.

we could maybe stand to have some additional methods on DependencyInterface that report whether or not an asset's deps have been resolved into positioning data, but it hasn't seemed necessary to me thus far.

16. CssGraphSorter::groupAndSort() receives "the full asset collection", rather than just the CSS assets, it retrieves just the CSS files itself by calling ->getCss() on the received collection.

yeah, i've been annoyed by this one a lot. i do agree that it should be shifted to the outside, and that doing so would improve code reusability with js. clearly. i've just been kinda spinning on whether those methods should just return an iterator (in which case they'd have to stay where they are now), or continue returning the whole collection (in which case they need to become more complicated - e.g., do they pass along unresolved libraries? what about js settings - do those only get passed on the getJs() variant?).

far from unanswerable questions, it's just that it's more small potatoes, so i've left it for a later cleanup pass.

17. CssGraphSorter::getGroupingKey() should check the asset's isPreprocessable() return value, and if it's FALSE, exclude it. This would help you to not have to special case ExternalAsset there.
Furthermore, I think we should strive to not have any instanceof SomeTypeOfAsset logic in there; we should be able to get metadata that we want to use for the grouping key, and if some metadata is missing, we just don't use that. That would simplify this code a lot, and it opens the door for potentially adding new asset types later on… (though I can't think of a use case for that).
This would also allow you to move groupAndSort() to abstract AssetGraphSorter and reuse it for the JsGraphSorter, further reducing code duplication.

i agree that i dislike the switch-ish checking there is awkward, and we should ideally just check metadata regardless of the asset type. that'd make the system more extensible in the event that people make their own asset classes (depend on abstractions, not concretions...).

i'm not sure i agree that it would actually make it reusable for the JsGraphSorter, though - metadata like async are present for js, but not css. then again, if a prop is always absent for one or the other, it wouldn't make a difference if it's used in the group key or not...

18. Can you rephrase this? It's hard to understand, at least to a non-native speaker:
* The impact of a dependency can be calculated myopically (without knowledge of
* the full set), as a dependency inherently guarantees the presence of the
* other vertex in the set.

yep, i've rewritten that whole phpdoc.

19. Shouldn't AssetGraph::transpose() also remove the original directed edges? Right now, AFAICT, it is effectively turning a directed graph into an undirected one?

nope, there's a new self() in there. it's operating on a fresh, empty graph object.

20. Inside CssGraphSorter::groupAndSort(), the docs imply that the grouping key serves to achieve optimal groups. This is true to some extent, but e.g. in the case of the "browsers" metadata that also ends up in the grouping key, it is a matter of correctness, not optimality.

i don't think i agree. optimality, to my mind, is about achieving a minimum given the correctness constraints.

21. AssetGraph adds directed edges in the "depends on" direction, i.e. if asset B depends on asset A, then there will be a directed edge from B to A. There's no problem with that. But if you'd invert that, couldn't you then avoid the transponse() operation in CssGraphSorter::groupAndSort() altogether?
AFAICT that's the only place where we'll use AssetGraph, so I think it's fine to invert the direction and make it "is a dependency of".

good call. we still need to leave the custom transpose() method in so that the overall behavior remains correct, but that's fine.

22. I think in places such as CssGraphSorter::groupAndSort(), we could all benefit from some ASCII art :)

for sure, me too. expressing a directed graph + optimality groups in (compact) ASCII art seems kinda tricky, but i'm sure we can work out some cases that are at least suggestive.

But when I log in, I get this:

Fatal error: Maximum function nesting level of '100' reached, aborting!

yep, that's a basic issue with this system: a depth-first traversal makes for a deep stack, and our custom algorithm that explores optimality groups at the same time as it explores normal edges makes that even deeper. gliph costs 2 func calls per normal edge descent and 3 per optimality group descent (ugh php 5.3 - in the php5.5 version, it's 1 and 2 calls, respectively). our custom algo means that we end up exploring most of the graph from a single start point, so with 35 css assets and a starting stack depth of ~22 (from what i've seen), we can blow that limit.

fortunately, that limit is not PHP's, it's xdebug's. set this in your php.ini:

xdebug.max_nesting_level=200

this value can be set at runtime, so we could just do that in the graph sorting class. we don't blow past it by much right now - apparently we reach a stack depth of 108 with the current patch.

#154:

"assets that render to a single HTML element"
You made a nice list there of all the classes to which the above statement applies: (Base|File|External|String)Asset … and AssetAggregate. Note how the "modifier word" is before "Asset" in all cases but one. Which makes me wonder: maybe we should rename AssetAggregate to AggregateAsset? Then, when you talk about it, it's just another type of asset… which is actually true. So I think this might be a small thing that helps in simplifying/conveying the different concepts.
"plus, all of the after()/before() stuff is translated onto libraries' contained assets, anyway. keeping it on the library is redundant."
From a technical POV it may be redundant, but from an API user's POV, it makes sense: "I want this library to run before this other library". The fact that this results in graph-based calculations to determine the final asset order does not change that. It's a lot easier conceptually to just say "library X must run before library Y" than "out of assets A, B, C, D, E and F of library X, only asset D must run before library Y".
Overall, my main gripe is that conceptually it makes little sense to say "hey this individual asset must run before that library". That's almost like saying "this car seat can enter the parking lot before that other car".
I agree that before/after relationships on individual assets is how it should work internally. But users of the API should only have to care about 1) library dependencies (which get resolved into an "after" relationship from each asset of the dependent library to the dependee library) and 2) relative positions of libraries (again resolved to the individual assets of the dependent to the dependee library). You still end up with the same desirable internal structure, but the user-facing system is easier to reason about.
"in the old system, sorting was automatic and implicit. grouping and creating the aggregate representations were a conjoined operation."
Sorting was indeed implicit. But grouping and creating the aggregate were definitely separate, at least since #352951: Make JS & CSS Preprocessing Pluggable: https://drupal.org/node/2034675. So maybe you mean "pre-#352951" in that statement?
"[in the new system,] sorting and grouping are actually the same operation. a"
Either I misunderstand that statement, or that's very bad: sorting is needed even when aggregation is disabled. Grouping is only necessary when aggregation is enabled.
ajaxPageState
We should definitely look into that, yes. I think something like https://docs.google.com/a/acquia.com/presentation/d/1Q44eWLI2qvZnmCF5oD2... is a natural fit.
"the real argument to keep with Assetic, i think, is that i'm hoping a lot of what i've done here can serve as a basis for Assetic 2.0 - Kris indicated his interest in making assets inert and using more services to do the work. and we have *definitely* moved in that direction."
But does that mean Assetic provides a discernible benefit? (If so: in Drupal 8 or later?)
"mostly, i think what this needs is a good diagram, and good docs."
+1 :)

#157/#158:
2. "getType()"
Good point about that potential source of confusion. However, that sounds to me like we should avoid "type" altogether. What about "source" for file/string/external and "kind" for CSS/JS?
3 + 4
We should discuss these later in more detail, it seems.
6: AssetInterface
D'oh, right, AssetAggregate is the perfect answer. Thanks for sharing your thoughts around a potential JsSettingsAsset — that sounds very sensible :)
7: "css/js" being stored twice
LOL, I'm stupid. Sorry. It indeed isn't.
9: external assets never being preprocessable
Interesting reply :) If you hardcode ExternalAsset::isPreprocessable() to FALSE, then shouldn't it be guaranteed that load() is never called? I guess that should only be guaranteed if isPreprocessable() is an instruction and not a hint. You're essentially suggesting to make it a hint.
I think that makes a lot of sense: one would then only have to override one of the asset handling classes ( CssCollectionGrouper) to make external assets be aggregated as well. Good call :)
10: about getLastModified()
I see your point wrt ExternalAsset. But you agree that it's fundamentally incompatible with StringAsset. That's why I think it's a fundamentally broken concept. The getHash() alternative I proposed does not suffer from this problem.
So what about this: keep getLastModified() for compatibility with Assetic. But also add getHash() (or… getETag(), if you want to stick to HTTP headers — interesting, no?), so that we can use getLastModified() as additional yet optional metadata within Drupal's asset handling system, and use get(Hash|Etag)() as the one true way to detect changes in assets.
15: "instanceof RelativePositionInterface"
My point wasn't that the docs are outdated, my point is that it's unclear why it's checking if it's an instance of RelativePositionInterface rather than AssetInterface. But I understand it now: it must be RelativePositionInterface because you're calling after().
17: move groupAndSort() to abstract AssetGraphSorter and reuse it for the JsGraphSorter not being possible due to JS-specific metadata like the "async" attribute
I think you misunderstood me: I don't want to move the getGroupingKey() method to the abstract class. That's the method that determines which pieces of metadata make up the group key, and they should indeed be implemented separately for each type of asset for the reasons you cite. But the groupAndSort() method OTOH, does not contain anything CSS- or JS-specific, so it should be safe to move to the abstract class.
19
Oh, I missed that part. That seems an elegant solution :)
20: "optimality, to my mind, is about achieving a minimum given the correctness constraints"
I disagree. The way it is implemented, it achieves a local optimum: the optimum for the current page, for the current collection of assets. Achieving optimality in the context of grouping assets for web pages is not about achieving a minimum on a single page, it's about achieving a minimum across pages. It is impossible to achieve an optimum without A) page visit frequencies, B) asset reference frequencies, C) typical user navigation paths across pages.

I think you're planning another reroll? How can we efficiently move this patch forward?


Finally, do you have anything to add to #152's "Reasons for this patch"? :)

weeeee!

I think you're planning another reroll? How can we efficiently move this patch forward?

yup, more coming presently. including more prolonged discussion.

the most substantive thing is that, based on recent discussion in #2114165: Add mrclay/minify vendor library to core, i'm gonna unblock myself on needing Minify and just port our current logic into FilterInterface, which will mean i can get the whole entire css stack working.

Status:Needs work» Needs review
Related issues:+#2028749: Explore addressable block rendering
StatusFileSize
new245.77 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new10.42 KB

sorry for the protracted delay. i'll try to get back into my once-a-day patch groove here again, since this is technically unblocked.

mostly minor updates in this patch, but a few things are fixed:

  • xdebug max nesting level is now set in AssetGraphSorter::__construct(), per my note above.
  • StringAsset now requires that non-empty string content be provided to it, making its state handling and id generation more predictable.
  • on AssetCollection::mergeCollection(), we now merge in the unresolved libraries, as well. this was an oversight and a bug before.
  • other assorted minor fixups, docs, and todos.

now, moving on to the more substantive work of refactoring our css minifier to follow assetic's interface.

The last submitted patch, 161: assets-1762204-161.patch, failed testing.

meanwhile, some responses to #159 -

Note how the "modifier word" is before "Asset" in all cases but one. Which makes me wonder: maybe we should rename AssetAggregate to AggregateAsset?

yeah, i'd be open to that. its primary identity is really the single asset, so its naming should reflect that. done it, committed & pushed to github, will be in the next patch.

Sorting was indeed implicit. But grouping and creating the aggregate were definitely separate, at least since #352951: Make JS & CSS Preprocessing Pluggable. So maybe you mean "pre-#352951" in that statement? Either I misunderstand that statement, or that's very bad: sorting is needed even when aggregation is disabled. Grouping is only necessary when aggregation is enabled.

yes, they're separate steps from the perspective that the thing that actually *creates* the aggregates is a separate class, and we can (and do) bypass it in the non-optimized case. the point i'm trying to make is that the *work* of creating a sort is fundamentally coupled to the governing logic for how to define groups. THAT is distinct from aggregating (which can be defined as acting on that linear list to create a bunch of AggregateAssets). but one cannot sort without a notion of the groups to be created; that is inherent and unavoidable.

But does that mean Assetic provides a discernible benefit? (If so: in Drupal 8 or later?)

not a lot. we could explore extending their "formula" system, possibly as an alternative to our libraries, but it's true that i've basically rerolled everything of substance. the benefit we get is a better-defined upgrade path to a possible future version of assetic, guaranteed by careful decisions we make today about compliance with its interfaces.

2. Good point about that potential source of confusion. However, that sounds to me like we should avoid "type" altogether. What about "source" for file/string/external and "kind" for CSS/JS?

i think better qualifying the method names is a good idea, but "kind" doesn't seem like quite the right word.

17. I think you misunderstood me: I don't want to move the getGroupingKey() method to the abstract class. That's the method that determines which pieces of metadata make up the group key, and they should indeed be implemented separately for each type of asset for the reasons you cite. But the groupAndSort() method OTOH, does not contain anything CSS- or JS-specific, so it should be safe to move to the abstract class.

ahh, i see. that makes more sense. and it might work. however...

one of the things we're gonna have to deal with in JS that we can blissfully ignore in CSS is splitting the graph into two parts - footer and header. i don't have a clear plan for that right now, as it involves juggling some state around these stateless services, and until it becomes clearer exactly how we replace the drupal_get_js() call from template_preprocess_html(), the requirements for that state-juggling are murky. i do suspect, however, that it may cause some divergence in that method for the CSS vs. JS case.

I disagree. The way it is implemented, it achieves a local optimum: the optimum for the current page, for the current collection of assets. Achieving optimality in the context of grouping assets for web pages is not about achieving a minimum on a single page, it's about achieving a minimum across pages. It is impossible to achieve an optimum without A) page visit frequencies, B) asset reference frequencies, C) typical user navigation paths across pages.

ah ok, totally fair. and you know i'm chasing that particular holy grail, too :)

however, i don't think that actually makes the naming of them as "optimality groups" incorrect. at the level of the graph algo, it does not know or care the basis on which the optimality groups were defined - it simply knows how to process them into the linear list that allows us to create aggregates. whether that is a local optimum or global optimum is outside of the graph algo's scope.

now, this is a new realization for me, so i'm not saying that all the docs are right. in particular, the ones on getGroupingKey() should probably be more careful about specifying that they are finding a local optimum. but the others are good...i think?

but one cannot sort without a notion of the groups to be created; that is inherent and unavoidable.

Thanks for that explanation. That makes sense :)

one of the things we're gonna have to deal with in JS that we can blissfully ignore in CSS is splitting the graph into two parts - footer and header. […] i do suspect, however, that it may cause some divergence in that method for the CSS vs. JS case.

Hah :) Good point.
I don't remember why, but IIRC it felt to me like the footer and header JS cases were going to result in two separate calls to JSGraphSorter. In any case, l'd say we don't need to discuss this further now: my suggestion would only be a tiny win and we're getting ahead of ourselves by talking about this already.

however, i don't think that actually makes the naming of them as "optimality groups" incorrect

It doesn't make sense to bikeshed this, so if you feel this is the way it should be, then that's fine, but in that case you should document your definition of "optimality groups".
I personally think something like "logical groups" makes more sense.

quick notes on the upshot of Wim's and my skype conversation just now:

  • allowing assets to declare dependencies directly is actually a feature addition. it could be handy-dandy, but for right now it's best to reduce scope and punt it to a follow-up.
  • for now, we'll move AssetInterface::getLastModified() into the adapter asset - meaning, we always throw an exception if it's called. we may not even need to add the getHash() methods Wim proposed earlier - we'll cross that bridge a little later.

scope reduction++

Component:base system» asset library system
Issue tags:+beta target

It would be fantastic if we could get some of this in for beta 1, but hesitant to consider it a blocker.

161: assets-1762204-161.patch queued for re-testing.

The last submitted patch, 161: assets-1762204-161.patch, failed testing.

Since there's minimal if any public API changes here (lots of additions but not really changes), assuming it's an improvement or neutral for both back end and front end performance I'd not have a problem getting it in after beta. However yes it's a sufficiently large change that it'd be good to get it in before if we can.

Issue summary:View changes
StatusFileSize
new243.11 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Component/ObjectState/FreezableTrait.php.
[ View ]

eeeeeeeeeeeeasing back in. just two things here:

  • per the discussions with Wim, shifts getLastModified() out to the adapter - meaning, it always throws an exception in all our systems, because we should never use it.
  • converts a whole crap ton of stuff to traits. it's only about 200 fewer LoC total, but i think it actually might make it *much* easier for folks to understand. there is now an AsseticAdapterTrait, BasicCollectionTrait, DependencyTrait, RelativePositionTrait, FreezableTrait. i'm especially happy with how i could use method aliasing and visibility shifting to make BasicCollectionTrait's internal _bcinit() method into the constructor for AssetCollection. woot woot.

no interdiff b/c it would be totally unhelpful after catching back up.

Status:Needs review» Needs work

The last submitted patch, 171: assets-1762204-171.patch, failed testing.

The last submitted patch, 171: assets-1762204-171.patch, failed testing.

still no php5.4 on testbots, eh? lol.

Not yet, their trying to get FPM working here #2135199: Provide php 5.4 testing on testbots for D8 code without breaking everything else. If anybody lurking this issue can help test testbot that would help traits and PHP 5.4 get in:)

StatusFileSize
new68.24 KB
new290.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch assets-1762204-176.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

another quick patch - this gets rid of FreezableTrait, because i moved it into a new composer lib, frozone. it also guts the locking out of AssetCollector, as frozone provides that pattern as well. doing this eliminates ~150 LoC out of our codebase, and a handful of unit tests, as they are all covered by frozone.

Status:Needs work» Needs review

Changing to Needs Review so #176 will get tested.

The last submitted patch, 152: assets-1762204-173.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 176: assets-1762204-176.patch, failed testing.

mm, i didn't do a catchup merge. probably collided on something in composer, i'll deal with it in the next patch.

All of the drupal_add_* functions are now privatized and removed from general use in core.

#2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses