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:
- #2073819: [META] Remove direct calls to drupal_add_css()
- #2073823: [META] Remove drupal_add_js() calls
- #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff
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 ourAssetInterface
, which is an extension of Assetic'sAssetInterface
. - 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 intoAssetInterface
objects, which need them to function, but they're kept in a separate class in order to keepAssetInterface
'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 ourAssetCollectionInterface
, 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 newAssetLibrary
class implementsAssetCollectionInterface
. - Asset Aggregates - these are also a container for
AssetInterface
objects, and have some collection-style methods, but unlike collections, they themselves implementAssetInterface
, 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 AssetAggregate
s. 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 plainException
s that are thrown should be converted to an appropriate more specific type. (some progress made)FileAsset
,StringAsset
andExternalAsset
need unit tests.ExternalAsset
needs love. Right now it just throws exceptions if itsload()
orgetLastModified()
methods are called, but this is wrong - it needs to work, even if core doesn't use it.Figure out a sane basis forStringAsset
to return a value fromgetLastModified()
.- Deal with uniqueness in collections. That entails:
Deciding on a standard form for ids (e.g., files - absolute path? relative? what if #1308152: Add stream wrappers to access extension files goes in?)- Figuring out if and when to allow changing ids, recalculating them, and when such state changes should be considered appropriate.
Make interfaces fluent, where possible and appropriate.Dependency resolution needs to be handled. This is big, and probably involves reviving theAssetLibraryRepository
.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()
anddump()
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 makingAssetMetadataBag
s, andAssetCollector
should probably use it instead of directly callingnew 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)
Refactordrupal_process_attached()
to utilize anAssetCollector
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 toAssetCollectionInterface
)- 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
intoJsCollectionAggregator
. And tests. - Write a
JsGraphSorter
. And tests.
- Rewrite
- 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.
RewriteAnd tests.CssCollectionOptimizer
.- Rewrite
CssOptimizer
. And tests. - Rewrite
CssDumper
. And tests. RefactorAnd tests.CssCollectionGrouper
intoCssCollectionAggregator
.Write aAnd tests.CssGraphSorter
.
- In doing the previous two items, we need to decide just how much we actually rely on Assetic's
load()
anddump()
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(still needs a little tweaking, though)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.Graph sorting needs functional tests. TheAssetGraph
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 throughdrupal_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.
Comment | File | Size | Author |
---|---|---|---|
#176 | assets-1762204-176.patch | 290.63 KB | sdboyer |
#176 | interdiff.txt | 68.24 KB | sdboyer |
#171 | assets-1762204-171.patch | 243.11 KB | sdboyer |
#161 | interdiff.txt | 10.42 KB | sdboyer |
#161 | assets-1762204-161.patch | 245.77 KB | sdboyer |
Comments
Comment #1
nod_tag
Comment #2
sunComment #3
catchSorry 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.
Comment #4
nod_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.
Comment #5
Kiphaas7 CreditAttribution: Kiphaas7 commented+1 on moving the bundling logic into a swappable/extendable function/class/assetic plugin. That should mean both bundling and preprocessing becomes pluggable, right?
Comment #6
pounardHaving 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.
Comment #7
nod_Bumping since it's that important.
Comment #8
tim.plunkettI fail to see how this is a blocker, can you expand the issue summary?
What about adding settings via JS?
Comment #9
catchComment #10
sdboyer CreditAttribution: sdboyer commentedok, 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'sAssetInterface
with some additional methods for handling the metadata we need.D\C\A\BaseAsset
- extends Assetic'sBaseAsset
, and is a handy abstract base for all our asset classes.D\C\A\BaseFileAsset
- extendsD\C\A\BaseAsset
and copies logic from Assetic'sFileAsset
to serve as the abstract base class for all assets that are accessible via the local filesystem/a stream wrapper.D\C\A\BaseExternalAsset
- extendsD\C\A\BaseAsset
and copies logic from Assetic'sHttpAsset
to serve as the abstract base class for all externally-hosted assets.D\C\A\BaseStringAsset
- extendsD\C\A\BaseAsset
and copies logic from Assetic'sStringAsset
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 implementsD\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 whatAssetBag
s are about.finally, there's the least-worked-on part of this -
D\C\A\AssetLibrary
, alongsideD\C\A\AssetLibraryReference
andD\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 forhook_library_info()
), andD\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
AssetBag
s 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.Comment #11
sdboyer CreditAttribution: sdboyer commentedbetter title, since i'm hijacking.
Comment #12
Wim LeersComment #13
Wim LeersUpon applying locally, I noticed that it doesn't apply anymore. A small bit has changed in
CoreBundle.php
. Otherwise identical.Comment #14
Wim LeersI 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:
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.
Comment #15
sdboyer CreditAttribution: sdboyer commentedah, 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*
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.
Comment #17
nod_Rerolled #14 with changes from #15
Comment #19
nod_Failing for the right reasons at least.
Comment #20
sdboyer CreditAttribution: sdboyer commentedOK, new patch with two additions, and one removal:
AssetBagInterface::sort()
, NOT when the assets themselves are added.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. theAssetLibraryManager
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.
Comment #21
sdboyer CreditAttribution: sdboyer commentedthis will fail (intentionally), but let's change status so other people can see where those failures are :)
Comment #23
Owen Barton CreditAttribution: Owen Barton commented+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.
Comment #24
sdboyer CreditAttribution: sdboyer commentedok, 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 :)
Comment #24.0
sdboyer CreditAttribution: sdboyer commentedRevamp after hijacking.
Comment #25
clemens.tolboom@sdboyer: Regarding the remaining task
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 useAcyclicDirectedGraph::getTSL()
(code).Should/Could we upgrade Graph.inc to mentioned classes as we have feature freeze?
Comment #26
sdboyer CreditAttribution: sdboyer commented@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.Comment #27
clemens.tolboom@sdboyer I'm working on #1344648: Reincarnate graph class? first to make Unittest for it. Next I'll try this issue. Then Graph.inc
Comment #28
sdboyer CreditAttribution: sdboyer commentedfantastic. 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.
Comment #29
clemens.tolboom@sdboyer I did not succeed in moving fast enough :(
Here is my small effort :(
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 likeFeel free to copy the vendor/graph into Drupal namespace but I'll focus on the github lib + Graph API.
Comment #30
clemens.tolboomWith patch :p
Comment #32
clemens.tolboomHmmm ... 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]
Comment #33
clemens.tolboomFrom the log
Comment #34
clemens.tolboom@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
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.
Comment #35
lunaris CreditAttribution: lunaris commentedOk; a little movement on this after lots of useful discussions with sdboyer in IRC.
AssetInterface
,BaseAsset
,FileAsset
,AssetGraph
(more in a second) andAssetBag
. It's likely that for the DIC we'll need anAssetBagInterface
or similar (as originally proposed by sdboyer) but this is very much a work in progress.Drupal\Component\Graph\Graph
not satisfying our needs, I've knocked up a specialisedAssetGraph
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 aroundAssetGraph
, 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 anAssetBag
property, as well as some dumb mutators/accessors (to be fleshed out/made more robust).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
Comment #36
sdboyer CreditAttribution: sdboyer commentedthanks 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.
Comment #37
Cameron Tod CreditAttribution: Cameron Tod commentedDoes this help?
Comment #38
nod_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; introduce "before" and "after" for conditional ordering.
Comment #39
sdboyer CreditAttribution: sdboyer commentedper 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.
Comment #40
effulgentsia CreditAttribution: effulgentsia commentedRemoving 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:
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.
Comment #41
xjmComment #42
effulgentsia CreditAttribution: effulgentsia commentedPer #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.
Comment #43
Wim LeersIn 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 thePartialResponse
stuff that's being worked on over at #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox].Thoughts?
Comment #44
sdboyer CreditAttribution: sdboyer commentedall 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.
Comment #45
robinvdvleuten CreditAttribution: robinvdvleuten commented#35: assets-1762204-201302270802.patch queued for re-testing.
Comment #47
klonos...as per #39.
Unless of course you intent to work on this Robin. (?)
Comment #48
robinvdvleuten CreditAttribution: robinvdvleuten commented@klonos I'm currently trying some things in D8 with Assetic, can this issue stays open?
Comment #49
klonos...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.
Comment #50
robinvdvleuten CreditAttribution: robinvdvleuten commentedFor 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.
Comment #51
Owen Barton CreditAttribution: Owen Barton commented@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.
Comment #52
robinvdvleuten CreditAttribution: robinvdvleuten commented@owen The princess branch seems to be broken. I get an error about a missing Timer class. Does anyone know how to fix this?
Comment #53
sdboyer CreditAttribution: sdboyer commentedyeah - 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.
Comment #54
sdboyer CreditAttribution: sdboyer commentedrebooting 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:
AssetBase
hierarchy. it's still pretty dirty, but it's much more functional. probably most important is that i've set upArrayAccess
-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.AssetLibraryCollector
, which is intended for use in a replacement tohook_library_info()
, andAssetCollector
, which is intended to be injected into a block'sdeclareAssets()
method (also added by this patch).AssetCollector
andAssetLibrary
in this iteration. these also inherently test certain segments ofAssetBase
.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:
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...hook_library_info()
.AssetLibraryManager
needs finalizing - a couple more methods. not big stuff.AssetBag
, parts of theAssetBase
hierarchy,AssetLibraryManager
,AssetLibraryCollector
.drupal_add_js()
anddrupal_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 fromgetCss()
andgetJs()
, rather than an array. this'll be the thing that a sorter can later work with.and things that are definitively followups:
AssetBase
hierarchy - but as long as it works for now and can live behind that interface, we can refactor it later.FilterManager
, but we'll want and need to do that and have it behave very similarly to how theAssetLibraryManager
does - a global/container-encapsulated listing of available filters.also, this is effectively blocked on #1308152: Add stream wrappers to access extension 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.
Comment #56
sdboyer CreditAttribution: sdboyer commentedoh, 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.
Comment #57
sdboyer CreditAttribution: sdboyer commentedoh 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
andAssetAssemblyTest
, 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.Comment #59
sdboyer CreditAttribution: sdboyer commentedoh yeah, we're not in sandbox anymore.
Comment #60
sdboyer CreditAttribution: sdboyer commented...well now, that was easier than i thought.
this interdiff changes
AssetLibraryManager
toAssetLibraryRepository
, 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 inhook_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!
Comment #61
sdboyer CreditAttribution: sdboyer commentedComment #63
Wim LeersFirst: 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
andcore/lib/Drupal/Core/Asset/AssetLibraryReference.php
.The whole point of
AssetCollector
is not very clear.I think it would feel more natural to have all these methods on
AssetBag
s instead. I suspect you made this split to keepAssetBag
s stupid?This method is only being used in
AssetLibraryRepository
and in tests. Doesn't that mean this method is not truly necessary? Along with all thecreate*Asset
methods?What would an "asset-like" object be?
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.This is only being used in
AssetLibraryRepository
AFAICT, which suggests to me that it might be better to have the logic to manipulateAssetBag
s in there, rather than have a separate class (AssetLibraryCollector
)?This is closely related to my remarks on
AssetCollector
.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.
Same.
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.
Comment #65
sdboyer CreditAttribution: sdboyer commentedremoving 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.
Comment #66
sdboyer CreditAttribution: sdboyer commentedresponding to #63:
yep. sorry if i gave a different impression. this is decidedly WIP.
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.
yes, i don't want
AssetBag
s 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:
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.
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.
an
AssetLibrary
. that's the other thing that implementsAssetDependencyInterface
.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.
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).
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.
Comment #67
sdboyer CreditAttribution: sdboyer commentedoh ffs, firefox
Comment #68
sdboyer CreditAttribution: sdboyer commentedRefactored
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.Comment #70
sdboyer CreditAttribution: sdboyer commentedupdating to reflect the fact that this patch isn't pushing for a change to existing public APIs.
Comment #71
sdboyer CreditAttribution: sdboyer commentedand 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:
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.CssCollectionGrouperNouveaux
(the goal being to swap that in forCssCollectionGrouper
). 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 - henceCssCollectionGrouperNouveaux::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.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 outgroup
. 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 killevery_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.Comment #73
sdboyer CreditAttribution: sdboyer commented#71: assets-1762204-71.patch queued for re-testing.
Comment #75
clemens.tolboomI'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 :-/
Comment #76
sdboyer CreditAttribution: sdboyer commented@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.
Comment #77
sdboyer CreditAttribution: sdboyer commentedupdated 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.
Comment #79
sdboyer CreditAttribution: sdboyer commentedHad to reroll after #2088651: Update symfony components to latest 2.3.* in order to re-reconcile gliph; no interdiff. this should apply and pass.
Comment #81
mparker17#79: assets-1762204-79.patch queued for re-testing.
Comment #83
sdboyer CreditAttribution: sdboyer commentedugh. lemme track that down...
Comment #84
sdboyer CreditAttribution: sdboyer commentedok, 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.Comment #86
sdboyer CreditAttribution: sdboyer commentedok, 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 theAssetGraph
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.Comment #87
sdboyer CreditAttribution: sdboyer commentedi'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 :)Comment #88
sdboyer CreditAttribution: sdboyer commentedwow, 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.
Comment #90
clemens.tolboomSome tiny remarks:
Is the issue summary up to date with the latest patches? Esp. regarding the Glyph library.
A lot files have No newline at end of file
Comment #91
sdboyer CreditAttribution: sdboyer commentedyeah, 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.
Comment #93
herom CreditAttribution: herom commented#91: assets-1762204-91.patch queued for re-testing.
Comment #94
RobLoachThis is exceptionally awesome.
Comment #95
sdboyer CreditAttribution: sdboyer commented@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:
BaseAsset
,AssetCollection
, andAssetCollector
.BaseAsset
now descends from Assetic'sBaseAsset
(again). This probably won't be an issue, but could be a little awkward because all of their base's properties areprivate
.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.AssetCollectionBasicInterface::reindex()
, so that was removed from everywhere.AssetLibraryRepository
andAssetLibraryCollector
. 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 anAssetCollection
- that would mean resolving and building the library as it's being worked through withindrupal_process_attached()
. This is probably not the end of the world, and it got rid of bad code, sooo...Comment #95.0
sdboyer CreditAttribution: sdboyer commentedadd repo info
Comment #95.1
sdboyer CreditAttribution: sdboyer commentedhome stretch! totally rewrite the summary.
Comment #95.2
sdboyer CreditAttribution: sdboyer commentedUpdated issue summary.
Comment #95.3
sdboyer CreditAttribution: sdboyer commentedAdded todo for drupal_process_attached()
Comment #96
sdboyer CreditAttribution: sdboyer commentedno takers, eh? :(
i'll return to this next week.
Comment #97
sdboyer CreditAttribution: sdboyer commentedOK, 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!
Comment #98
sdboyer CreditAttribution: sdboyer commentedok, knocked out the
AssetCollectorInterface
and some accompanying exception improvements. also put in the skeleton forBaseAssetAggregateTest
- that's gonna suck to write :/ but, that's on deck for later tonight.Comment #98.0
sdboyer CreditAttribution: sdboyer commentedUpdated issue summary.
Comment #98.1
sdboyer CreditAttribution: sdboyer commentedticked off AssetCollector interface, couple other minor points.
Comment #99
sdboyer CreditAttribution: sdboyer commentedok, 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.Comment #101
sdboyer CreditAttribution: sdboyer commented#99: assets-1762204-99.patch queued for re-testing.
Comment #103
sdboyer CreditAttribution: sdboyer commentedok, i don't know wtf is going on there, there are no syntax errors in that file on my local.
Comment #104
tim.plunkettif (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)) {
Comment #105
sdboyer CreditAttribution: sdboyer commentedyep, 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...
Comment #105.0
sdboyer CreditAttribution: sdboyer commentedUpdated issue summary.
Comment #106
sdboyer CreditAttribution: sdboyer commentedthis 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...
Comment #106.0
sdboyer CreditAttribution: sdboyer commentedMarked exception thing complete - it's good enough, and the remainder will be fixed by other refactors.
Comment #107
sdboyer CreditAttribution: sdboyer commentedok, 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.Comment #108.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #108.1
sdboyer CreditAttribution: sdboyer commentedstrikethrough on separated graph sorter
Comment #108.2
sdboyer CreditAttribution: sdboyer commentedinterface for AssetMetadataBag is done; add todo/notes about ids in collections.
Comment #109
sdboyer CreditAttribution: sdboyer commentednext pass. this does the following:
AssetOrderingInterface
into two interfaces - one for dependencies (DependencyInterface
), and one for pure positioning (RelativePositionInterface
).MetadataFactoryInterface
- a factory for asset metadata - and makes theAssetCollectorInterface
utilize it.should still have the same 6 fails.
Comment #111
sdboyer CreditAttribution: sdboyer commentedtwo changes on this pass:
ParameterBag
s.this will fail again because of composer conflicts. i'll reroll again for that later.
Comment #111.0
sdboyer CreditAttribution: sdboyer commentedchecked off creation of asset metadata factory.
Comment #112
jibranFor Testing.
Comment #113.0
(not verified) CreditAttribution: commentedmark finished the step to simplify AssetMetadataBag
Comment #113.1
sdboyer CreditAttribution: sdboyer commentedBuncha new todos
Comment #114
sdboyer CreditAttribution: sdboyer commented1.5 things in this pass:
FileAsset
,StringAsset
, andExternalAsset
. 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 theExternalAsset
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.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.
Comment #115
sdboyer CreditAttribution: sdboyer commentedComment #116.0
(not verified) CreditAttribution: commentedtouchup, plus one more item
Comment #116.1
sdboyer CreditAttribution: sdboyer commentedMark off unit tests for base asset types
Comment #117
sdboyer CreditAttribution: sdboyer commentedfair bit more code to be focused on relatively small niggling details, but, here's what we got.
BaseAggregateAsset
, excludingload()
anddump()
.BaseAggregateAssetTest
andAssetCollectionTest
.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.
Comment #119
sdboyer CreditAttribution: sdboyer commentedthis pass does a ton.
BaseAssetAggregateTest::isEmpty()
correctly, fixing the new failure from the last patch.removeLeaf()
,replaceLeaf()
methods to work correctly with grace mode (recursing calls should always operate with grace, otherwise exceptions are thrown prematurely).AssetAggregateInterface
-each()
andeachLeaf()
. 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 toAssetCollectionBasicInterface
.AssetCollectionBasicInterface
now extends\Countable
.BaseAggregateAsset
was extending off the leaf asset hierarchy, ultimately having Assetic'sBaseAsset
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 aBasicAssetCollection
that implementsAssetCollectionBasicInterface
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.BaseAggregateAsset
toAssetAggregate
, and got rid ofCssAggregate
andJsAggregate
. because...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.Comment #119.0
sdboyer CreditAttribution: sdboyer commentedmark "basic" unit tests for BaseAssetAggregate as complete.
Comment #121
sdboyer CreditAttribution: sdboyer commentedquick pass this time.
AssetLibraryRepository
andAssetLibraryCollector
, the latter of which was renamed toAssetLibraryFactory
. these two are massively cleaned up from their ugly state before; they're halfway decent now.system
,jquery
). it's still in two parts, it's just joined together around a colon.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.Comment #122
sdboyer CreditAttribution: sdboyer commentedComment #122.0
sdboyer CreditAttribution: sdboyer commentedmark of plan for colletion ids - that's now in hand, even if it means reindexing
Comment #124
sdboyer CreditAttribution: sdboyer commented#121: assets-1762204-121.patch queued for re-testing.
Comment #126
nod_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.
Comment #127
sdboyer CreditAttribution: sdboyer commentedahhhh, a comment other than me!! :)
yep, i see no reason that slash would be an issue. i'll change it in my next reroll.
Comment #128
cosmicdreams CreditAttribution: cosmicdreams commentedA 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.
Comment #129
sdboyer CreditAttribution: sdboyer commented@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:
AssetLibraryRepository
. this is only about 50% coverage though, so there's more to go.Comment #130
sdboyer CreditAttribution: sdboyer commentedComment #132
Kiphaas7 CreditAttribution: Kiphaas7 commentedCompletely 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 :).
Comment #133
sdboyer CreditAttribution: sdboyer commented#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 fromDependencyInterface
because the collection itself does not have dependencies, as collections themselves are not [logical] assets.core.services.yml
with a number of additional services.drupal_process_attached()
and the variousdrupal_add*()
functions that sends declared assets to a global collection/collector combo, so that we can use them in the new system.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.AssetCollectionInterface::reverse()
, which reverses the current order of assets in the collection.AssetGraph::transpose()
; it was traversing using the wrong graph method, causing it to incorrectly drop vertices with degree of 0.AssetCollector
was incorrectly including js assets in its auto-after()
. this is now fixed.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 toreverse()
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 ahook_library_info_alter()
call, indicating that i likely just forgot to invoke the alter back inAssetLibraryFactory
.once that declaration vector is captured correctly, we just need #2114165: Consider adopting 3rd party CSS minification library 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.
Comment #134.0
(not verified) CreditAttribution: commentedreintroduced AssetLibraryRepository
Comment #135
sdboyer CreditAttribution: sdboyer commentedgoddamn core.services.yml reconciliation. testbot also complained about common.inc, but i didn't see any issues with that. hopefully this will work.
Comment #137
sdboyer CreditAttribution: sdboyer commentedhmmm...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.
Comment #138
sdboyer CreditAttribution: sdboyer commentednext pass. four things done, here:
AssetLibraryFactory
, per the note at the end of #133.CssCollectionAggregator
up to date (it had an outdated class reference).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:
and this is the set produced by the new system. yes, we have full paths in the new system.
and, for the lookie-loo, here's a screenie of the homepage when rendered through this:
Comment #140
sdboyer CreditAttribution: sdboyer commentedturns 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...Comment #141
sdboyer CreditAttribution: sdboyer commentedfound 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?
Comment #142
sdboyer CreditAttribution: sdboyer commentedComment #144
sdboyer CreditAttribution: sdboyer commentedoooh, 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.htmlspecialchars()
inDbLogTest
.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 failingEditLoadingTest
, andViewAjaxTest
.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.
Comment #145
sdboyer CreditAttribution: sdboyer commentedand...bumping to critical, on the basis that we need to get this to the point where we can really make performance benefit comparisons.
Comment #145.0
sdboyer CreditAttribution: sdboyer commentedticked off several major todos. woot.
Comment #146
sdboyer CreditAttribution: sdboyer commentedComment #147
sdboyer CreditAttribution: sdboyer commentedComment #148
sdboyer CreditAttribution: sdboyer commentedComment #149
sdboyer CreditAttribution: sdboyer commentedComment #150
sdboyer CreditAttribution: sdboyer commentedoook, another pass. this was mostly internals, again.
void
is now returning$this
.AssetAggregateTest
andAssetCollectionTest
from the shared parentBasicAssetCollectionTest
. it was a lot of redundancy for very little value.DependencyInterface
andRelativePositionInterface
and their implementations (libraries and base assets); cleaned up a lot, and added a ton of proper tests.DependencyInterface
no longer inherit fromRelativePositionInterface
. interface segregation principle aside, the two really are distinct.AssetLibrary
up to >95%.Comment #152
Wim LeersWow. 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:
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):
before()/after()
for libraries? That can also be necessary, even though it'd usually only apply to its JS.before()/after()
syntax for#attached
?ajaxPageState
. So why this change (besides being more logical)? How do you plan to updateajaxPageState
?array('#markup' => '<p>Hi!</p>', '#attached' => array(…))
and runsdrupal_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:
Plus, it looks like e.g. Assetic's CSS minifier is barely worthwhile to use (#2114165-10: Consider adopting 3rd party CSS minification library).
Does that mean we should reconsider using Assetic at all?
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.
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.
AssetInterface::getAssetType()
overly verbose? Wouldn'tAssetInterface::getType()
suffice?BaseAsset
implementsDependencyInterface
, 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.
RelativePositionInterface
'sbefore()
andafter()
'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?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.AssetInterface
simply extend not onlyAsseticAssetInterface
, but alsoDependencyInterface
andRelativePositionInterface
?'js'
or'css
) stored both inAssetInterface
and theAssetMetadataInterface
that every object implementingAssetInterface
must hold?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 hardcodeisPreprocessable()
to returningFALSE
. Drupal will listen to that and not callload()
, 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.)getLastModified()
methods that are yet to be implemented forFileAsset
andStringAsset
… 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 (seeCacheBustingWorker::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.AssetCollector
:restoreDefaults()
is no longer an accurate name AFAICT.getMetadataDefaults()
is public.clearLastCss()
feels like a leaky abstraction? Shouldn't it be the responsibility of the caller ofAssetCollector::create()
to pass the previous asset, if any?AssetCollectionBasicInterface
:Then why not simply use Drupal's
AssetInterface
, rather than Assetic's?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'sAssetInterface
, Drupal'sAssetCollectionBasicInterface
(making it both an individual asset and an asset collection from a Drupal POV!) and Assetic'sAssetCollectionInterface
. Similar things then logically also apply toAssetAggregate
.Why can't a simple
AssetCollection
fulfill the purpose ofAssetAggregate
? AFAICT that's because to use Assetic's filters, it must implement Assetic'sAssetCollectionInterface
. But couldn't that be solved much more simply, by letting Drupal'sAssetCollectionInterface
extend Assetic's identically named interface? Or does that bring downsides that I'm missing?AssetLibraryRepositoryInterface
?AssetLibraryRepository::resolveDependencies()
:->after()
, which is about order/relative position, not dependencies, so shouldn't that be a call toaddDependency()
instead?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);
CssGraphSorter::getGroupingKey()
should check the asset'sisPreprocessable()
return value, and if it's FALSE, exclude it. This would help you to not have to special caseExternalAsset
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()
toabstract AssetGraphSorter
and reuse it for theJsGraphSorter
, further reducing code duplication.AssetGraph::transpose()
also remove the original directed edges? Right now, AFAICT, it is effectively turning a directed graph into an undirected one?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.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 thetransponse()
operation inCssGraphSorter::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".CssGraphSorter::groupAndSort()
, we could all benefit from some ASCII art :) :D I think things likewill 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).
Comment #153
Wim LeersROFL — 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
Comment #154
sdboyer CreditAttribution: sdboyer commentedWimLeers+++++++++++++++++++++++++++++++++++++++++
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.
we talked about this a bit in IRC, but until the patch i've posted just here, it WAS possible to do
before()
andafter()
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:
BaseAsset
FileAsset
ExternalAsset
StringAsset
AssetAggregate
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 definesafter()
andbefore()
, 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.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:
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.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.
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.
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.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.
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.
Comment #157
sdboyer CreditAttribution: sdboyer commentedoh, 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.
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.
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.
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...
yup, this highlights a recent refactoring; the logs tell me Oct. 15.
DependencyInterface
andRelativePositionInterface
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.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 aFileAsset
- 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 singleJsSettingsAsset
, adding values to it incrementally, and that along to other collections it's merged into. there is no reason thatJsSettingsAsset
should be able to declare positioning or dependencies; it inherently can't have dependencies, and positioning should be determined by the system.errr...it isn't? well, in the implementation,
AssetInterface
objects ask their metadata what their type is. returning toJsSettingsAsset
, 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 toAssetInterface::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 :)
good call. i've made the change locally, will push it up later.
hmm, so i'm halfway in agreement with you here.
isPreprocessable()
should be hardcoded to FALSE inExternalAsset
. 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 theisPreprocessable()
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.
i'm gonna assume you meant
ExternalAsset
, notFileAsset
. 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 theLast-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.
i'll take these one at a time...
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.
true, with the
AssetMetadataFactory
being used instead of directly cloning asset metadata objects, that method name should probably change.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.
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?
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.
because then
AssetAggregate
couldn't implement the interface, as it would conflict with Assetic'sAssetCollectionInterface
, whichAssetAggregate
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()
andremoveLeaf()
vs.remove()
methods on the aggregate. hmm....ok, chunking again at this point; more later.
Comment #158
sdboyer CreditAttribution: sdboyer commentedi'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.
yep, just haven't extracted it yet. an
AssetLibraryInterface
as well.bullet by bullet, now:
RelativePositionInterface
"?yeah, i recently changed that code path and forgot to update the doc there; it used to check
AssetInterface
, but i changed it toRelativePositionInterface
.yeah, i'll add that.
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.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.
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...yep, i've rewritten that whole phpdoc.
nope, there's a
new self()
in there. it's operating on a fresh, empty graph object.i don't think i agree. optimality, to my mind, is about achieving a minimum given the correctness constraints.
good call. we still need to leave the custom
transpose()
method in so that the overall behavior remains correct, but that's fine.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.
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.
Comment #159
Wim Leers#154:
(Base|File|External|String)Asset
… andAssetAggregate
. Note how the "modifier word" is before "Asset" in all cases but one. Which makes me wonder: maybe we should renameAssetAggregate
toAggregateAsset
? 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.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.
ajaxPageState
#157/#158:
AssetInterface
AssetAggregate
is the perfect answer. Thanks for sharing your thoughts around a potentialJsSettingsAsset
— that sounds very sensible :)ExternalAsset::isPreprocessable()
to FALSE, then shouldn't it be guaranteed thatload()
is never called? I guess that should only be guaranteed ifisPreprocessable()
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 :)getLastModified()
ExternalAsset
. But you agree that it's fundamentally incompatible withStringAsset
. That's why I think it's a fundamentally broken concept. ThegetHash()
alternative I proposed does not suffer from this problem.So what about this: keep
getLastModified()
for compatibility with Assetic. But also addgetHash()
(or…getETag()
, if you want to stick to HTTP headers — interesting, no?), so that we can usegetLastModified()
as additional yet optional metadata within Drupal's asset handling system, and useget(Hash|Etag)()
as the one true way to detect changes in assets.RelativePositionInterface
rather thanAssetInterface
. But I understand it now: it must beRelativePositionInterface
because you're callingafter()
.groupAndSort()
to abstractAssetGraphSorter
and reuse it for theJsGraphSorter
not being possible due to JS-specific metadata like the "async" attributegetGroupingKey()
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 thegroupAndSort()
method OTOH, does not contain anything CSS- or JS-specific, so it should be safe to move to the abstract class.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"? :)
Comment #160
sdboyer CreditAttribution: sdboyer commentedweeeee!
yup, more coming presently. including more prolonged discussion.
the most substantive thing is that, based on recent discussion in #2114165: Consider adopting 3rd party CSS minification library, 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.Comment #161
sdboyer CreditAttribution: sdboyer commentedsorry 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:
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.AssetCollection::mergeCollection()
, we now merge in the unresolved libraries, as well. this was an oversight and a bug before.now, moving on to the more substantive work of refactoring our css minifier to follow assetic's interface.
Comment #163
sdboyer CreditAttribution: sdboyer commentedmeanwhile, some responses to #159 -
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.
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
AggregateAsset
s). but one cannot sort without a notion of the groups to be created; that is inherent and unavoidable.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.
i think better qualifying the method names is a good idea, but "kind" doesn't seem like quite the right word.
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 fromtemplate_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.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?Comment #164
Wim LeersThanks for that explanation. That makes sense :)
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.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.
Comment #165
sdboyer CreditAttribution: sdboyer commentedquick notes on the upshot of Wim's and my skype conversation just now:
AssetInterface::getLastModified()
into the adapter asset - meaning, we always throw an exception if it's called. we may not even need to add thegetHash()
methods Wim proposed earlier - we'll cross that bridge a little later.scope reduction++
Comment #166
xjmIt would be fantastic if we could get some of this in for beta 1, but hesitant to consider it a blocker.
Comment #167
andyceo CreditAttribution: andyceo commented161: assets-1762204-161.patch queued for re-testing.
Comment #169
catchSince 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.
Comment #170
larowlanNote #2168111: Allow drupal_render() to pass up #attached to parents
Comment #171
sdboyer CreditAttribution: sdboyer commentedeeeeeeeeeeeeasing back in. just two things here:
getLastModified()
out to the adapter - meaning, it always throws an exception in all our systems, because we should never use it.AsseticAdapterTrait
,BasicCollectionTrait
,DependencyTrait
,RelativePositionTrait
,FreezableTrait
. i'm especially happy with how i could use method aliasing and visibility shifting to makeBasicCollectionTrait
's internal_bcinit()
method into the constructor forAssetCollection
. woot woot.no interdiff b/c it would be totally unhelpful after catching back up.
Comment #174
sdboyer CreditAttribution: sdboyer commentedstill no php5.4 on testbots, eh? lol.
Comment #175
joelpittetNot 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:)
Comment #176
sdboyer CreditAttribution: sdboyer commentedanother quick patch - this gets rid of
FreezableTrait
, because i moved it into a new composer lib, frozone. it also guts the locking out ofAssetCollector
, 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.Comment #177
mikeytown2 CreditAttribution: mikeytown2 commentedChanging to Needs Review so #176 will get tested.
Comment #180
sdboyer CreditAttribution: sdboyer commentedmm, i didn't do a catchup merge. probably collided on something in composer, i'll deal with it in the next patch.
Comment #181
jessebeach CreditAttribution: jessebeach commentedAll 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
Comment #182
cordoval CreditAttribution: cordoval commented@sdboyer I cleaned up a bit the PR and here is the diff https://github.com/cordoval/drupal-1/pull/1/files
it needs rebase off of the real 8.x master, but i am getting better understanding of what is being done here.
Comment #183
catchThe really release blocking part of this is #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render which I don't think the current patch here solves at all.
Everything else I'd be really happy to put into either 8.0.x, but also 8.X.x - it would mean an API break for javascript weights, but that's an easy/nice conversion for a small number of contrib/custom modules. I don't think there's anything else here that specifically ties this to the release, so downgrading to major.
Comment #184
Wim Leers@codoval: If you can continue working on this, I can help with reviews :)
Comment #185
webchickWhile not strictly related to this issue, everyone I know who helped with the other issues is following here, and since the idea was for this to make everything all better...
Here is some developer feedback from a themer actively trying to use this to add in an external JS library to a theme: #2298551: Documentation updates adding/adjusting inline/external JS in themes Would love suggestions there on how we could've made this not an hours-long task, but still retain the benefits of what these "make Drupal output render-cache-friendly" initiatives are trying to do.
Comment #186
RainbowArrayI'll see if I can take a look at this during TC Drupal.
Comment #187
mgiffordComment #188
Wim Leers#2356845: Remove the assetic library actually removed the Assetic library. Moving to 8.1.x.
Comment #189
Wim LeersApparently we're marking all 8.1.x issues postponed for now, so doing that here too.
Comment #190
Wim LeersDefinitely not a beta target anymore.
Comment #191
webchickAlso of interest: https://twitter.com/fabpot/status/648520881145253888
Sounds like Assetic is potentially being tanked in Symfony 3.0 in favour of bower/grunt/etc.
Won't fix?
Comment #192
Crell CreditAttribution: Crell as a volunteer commentedYeah, won't-fix. If we revisit using a 3rd party assest mangling library in 8.1 or later the landscape will have shifted enough that this thread is simply not relevant.
Comment #193
mikeytown2 CreditAttribution: mikeytown2 commentedFYI: #2588901: AdvAgg 8.x-2.x Port was created today