| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
| Issue tags: | API change, API clean-up, JavaScript, JavaScript clean-up |
Issue Summary
Problem/Motivation
(Work on this is being coordinated in the 1762204-assets branch in the SCOTCH sandbox)
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 supplant the drupal_add_css/js() system.
This system should aim to bolt on top of the custom PartialResponse system being worked on in #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox].
This is also a major hurdle on the path towards the goals of SCOTCH.
This change is necessary because the current system is reliant on globals (drupal_add_css/js()), and as such, breaks the encapsulation introduced by Symfony, and the centralized approach to page output SCOTCH is aiming for.
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.
Tackling this issue properly entails a small kaleidoscope of considerations. I've found it easiest to break this down by thinking about what kind of code needs to interact with this system, and when.
Producers
- First and foremost, blocks need to be able to declare the assets they require via a clear part of their public interface. We don't really have to care about the non-blocks case, since we're shooting for having them be the only way we do
text/html; however, it would be nice to make sure we don't render othertext/htmlapproaches infeasible. - The base system needs to be able to declare its global(ish) assets (e.g., system.*.css)
- Themes need to be able to declare their global(ish) assets.
- Modules need to be able to declare global(ish) assets. This one is a slippery slope.
Since we're killing our global drupal_add_{css,js}(), we no longer have a single global dumping ground for them, which makes marshalling them all together more complicated.
Consumers
- The system responsible for weaving together all the assets reported by the above producers into a sane, ordered listing. (This is kinda sorta two systems - one for block-level assets, then another at the page level).
- Any intermediary/external caching systems that need to be cognizant of the "multipart" response from blocks - HTML (fragments) + assets.
- The system responsible for doing bundling and aggregation of these reported assets.
- The system responsible for deciding how to finally represent the bundled (or not) assets in HTML output.
Proposed resolution
We need a whole new approach to the declaration, ordering, aggregating, and final placement of assets into templates. Out of the box, Assetic helps significantly with the aggregating step, but is only somewhat helpful with the declaration step, and really no help with the sorting step. So, we must construct our own family of Assetic-related classes in order to manage sorting and declaration.
To assist with declaration, this patch introduces our own family of classes for representing the individual assets themselves:
Drupal\Core\Asset\AssetInterface- extends Assetic'sAssetInterfacewith some additional methods for handling asset metadata that 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\BaseAssetand copies logic from Assetic'sFileAssetto 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\BaseAssetand copies logic from Assetic'sHttpAssetto serve as the abstract base class for all externally-hosted assets.D\C\A\BaseStringAsset- extendsD\C\A\BaseAssetand copies logic from Assetic'sStringAssetto 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.
Also supporting the declaration process is a set of three classes: D\C\A\AssetLibrary, alongside D\C\A\AssetLibraryReference and D\C\A\AssetLibraryManager. These are, collectively, the replacement for our current "libraries" system. They are used to represent and access a versioned group of assets which other assets can declare as dependencies. D\C\A\AssetLibrary is the class that contains the individual assets in the library itself (which, of course, are all instances of things in our asset family tree, described above); D\C\A\AssetLibraryManager is a DIC service that keeps track of everything (essentially, the new repository for hook_library_info()), and D\C\A\AssetLibraryReference is the class that you create and attach to an asset you're declaring when you want that asset to depend on a particular library.
To assist with sorting (and declaration, at least for now), there is also the container for these assets: D\C\A\AssetBag, which implements D\C\A\AssetBagInterface. This is really the big differentiation point between Assetic and what we need, as none of their asset-group-managing mechanisms (Collections, Managers, Factories) are really designed to build up a fragmented, pseudo-organized list of assets, then massage them into correct order. That's what AssetBags are all about - though they also serve as a convenient single, stackable container for holding lots of declared assets.
Remaining tasks
Of the four stages (declaration, ordering, aggregation, and template injection), only the first two really should be dealt with in this patch. Template injection should be dealt with in a follow-up because it depends on #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox], and aggregation is a swappable step that can be seen as a performance optimization. Declaration has, I think, a complete skeleton in place. For ordering, libraries are still only marginally planned out, and we should also probably create an iterable container for AssetBagInterface::getJs() and AssetBagInterface::getCss() to return. Those iterables will be the primary handoff to the bundling/aggregation, and possibly also template injection steps.
Drupal\Core\AssetInterface::sort()needs to be implemented. this should iterate through all contained assets, grab their dependencies, then utilize theDrupal\Component\Graphsystem to generate a DAG that gives us proper ordering. I was initially approaching this by trying to make a new implementation ofDrupal\Component\Graph\Graphthat usesSplObjectStorageto do its thing with objects as keys, but that has (at least) two things that make it annoying enough as to probably not be worth it.- All of the
AssetInterface::*Dependency()methods need to be implemented onAssetBase. Same goes for after(), hasSequencing(), and getPreceders(). (ick on the latter two names, I know). - Many of the
Assetic\Asset\AssetInterfacemethods simply have copy/pasted code in them for doing things like file lookups. These need to be made more Drupalish, ideally by someone who understands our file handling well. If that necessitates altering the asset interfaces a bit, so be it. AssetLibrarydoes not follow the namespace + name pattern that our existing libraries do (because they currently just extendAssetCollections). I honestly don't see the use case that makes this necessary (multiple versions of the same library do not, IMO, cut it). In any case, this needs dealing with.- Assetic's
AssetReferenceacts like a passthrough class to the asset it references. The way it's currently written, ourAssetLibraryReferenceis really expected to be dereferenced into the actualAssetLibraryobject instance when it needs to be used. hook_library_info(), its invocation and implementations, need to be converted over to use theAssetLibraryManager, which should be populated during container compilation.- We should investigate the creation of our own
AssetFactory(one or more) to ease asset creation, but especially for contextually propagating defaults onto new assets. - We need test parity with existing tests, namely
CascadingStylesheetsTest,CascadingStylesheetsUnitTest, andJavascriptTest.
Realistically, we could probably get much of this system functioning in parallel with the current behaviors, commit this, then make followups for removing the deprecated global mechanisms.
User interface changes
None to speak of.
API changes
Pretty massive. Even if we don't do it in this patch, the goal is to remove drupal_add_css/js(). All declarations will need to go through the more targeted approaches.
Original report by nod_
Current API for adding JS or CSS files is over-complicated.
This issue is the middle step between #1737148: Explicitly declare all JS dependencies, don't use drupal_add_js and #1751602: Use Assetic to package JS and CSS files. All that in the context of fixing this issue: #352951: Make JS & CSS Preprocessing Pluggable.
The goal here is to come up with a very simple API to deal with files and removing all the aggregation steps and complicated setup currently in core as this will be tackled in the next Assetic issue. In the scope is to deceide what should happen to drupal.settings and how we can make that better than it is now (something that would very much simplify this is #1760548: Remove dependency on jQuery and drupal.js for JS settings).
Comments
#1
tag
#2
#3
Sorry I don't think this is an option. It's a few months since I reviewed Assetic, and it looks like it has a nice API for preprocessing files and allows them to be stitched together - which would be better than the crappy regexp for CSS, or the simple concat we currently have for JavaScript (and would allow things to be pluggable without having to re-implement tonnes of logic).
However Assetic, unless I completely missed something, doesn't help at all for determining which files to bundle together, which is where the vast majority of our complex logic is around. That logic is there for a reason to allow files to be aggregated at all, while also preventing the Drupal 6 situation of a different huge aggregate being served on almost any page. Making both bundling and preprocessing independently pluggable would be great, but we can't just rip out bundling and then 'replace' it with Assetic which doesn't actually tackle that problem at all.
#4
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.
#5
+1 on moving the bundling logic into a swappable/extendable function/class/assetic plugin. That should mean both bundling and preprocessing becomes pluggable, right?
#6
Having everything being registered will help us to make a per site basis library usage and open the way of static/global aggregation of almost site wide used bundles, reducing drastically number of parallel http requests and leveraging to the maximum the client cache. +1 on making this pluggable, because what I told just before is something I want to explore, but that a lot of people don't.
#7
Bumping since it's that important.
#8
I fail to see how this is a blocker, can you expand the issue summary?
What about adding settings via JS?
#9
#10
ok, per #1871596-22: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox], i'm hijacking this issue to focus specifically on the assets architecture required to kill drupal_add_css/js().
up front note - this patch is maybe 50% of what it needs to eventually be, but i think it's figured out the hard parts of how to structure this whole thing so that everything can work out well.
note also that i've got a branch for this in the SCOTCH repo, called
1762204-assets.this patch introduces a new Assetic-derived set of classes to encompass all of our different types of assets, as well as some containers for managing them. the basic reason for taking this approach is that Assetic is not designed to handle the case where assets are added NOT necessarily in the order in which they should be placed onto the page (nor is it particularly graceful about handling a mix of CSS and JS assets). however, the data we need to collect about assets is complex enough that we might as well just represent them with classes. and by representing them with classes that reuse Assetic's interfaces, we get to harvest the data we need but be immediately compatible with just about everything Assetic does, too.
the asset class family is:
Drupal\Core\Asset\AssetInterface- extends Assetic'sAssetInterfacewith 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\BaseAssetand copies logic from Assetic'sFileAssetto 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\BaseAssetand copies logic from Assetic'sHttpAssetto serve as the abstract base class for all externally-hosted assets.D\C\A\BaseStringAsset- extendsD\C\A\BaseAssetand copies logic from Assetic'sStringAssetto 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 whatAssetBags are about.finally, there's the least-worked-on part of this -
D\C\A\AssetLibrary, alongsideD\C\A\AssetLibraryReferenceandD\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\AssetLibraryis 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\AssetLibraryManageris a DIC service that keeps track of everything (essentially, the new repository forhook_library_info()), andD\C\A\AssetLibraryReferenceis the class that you create and attach to an asset you're declaring when you want that asset to depend on a particular library.there's one test in there -
AssetAssemblyTest- that should give a basic sense of how this is all supposed to be used. there's still QUITE a lot more to be added there, but i feel like it's enough that someone else can look at the test and begin to see where i'm going with it.to get a sense of how this all fits together, read the issue summary (the bits on assets) of #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox]. i'll be moving the relevant bits of it over here presently (along with itemized TODOs - there are definitely still lots). the basic idea, though, is that we create
AssetBags at various points in the request process, pass them back up the stack, and then add several compilation/aggregation stages as part of the main kernel request/response process.#11
better title, since i'm hijacking.
#12
#13
Upon applying locally, I noticed that it doesn't apply anymore. A small bit has changed in
CoreBundle.php. Otherwise identical.#14
I like what I read in #10, but without it actually working, it's of course just a nice story :)
It was interesting to see that you've changed the default scope of JS files from header to footer. I applaud that, but it may require changes to some core JS as well (since some may rely on header being the default).
I was mostly wondering how this could possibly work:
+ public function testSortingAndDependencyResolution() {+ $bag = new AssetBag();
+
+ $alm = new AssetLibraryManager();
+ $alm->set('jquery', $this->createJQueryAssetLibrary());
+ $dep = new AssetLibraryReference('jquery', $alm);
+
+ $css1 = new StylesheetFileAsset(DRUPAL_ROOT . '/core/misc/vertical-tabs.css');
+ $js1 = new JavascriptFileAsset(DRUPAL_ROOT . '/core/misc/ajax.js');
+ // $js1->addDependency($dep);
+
+ $bag->add($css1);
+ $bag->add($js1);
+
+ $this->assertEqual(array(new JavascriptFileAsset('core/misc/jquery.js'), $js1), $bag->getJs());
+ }
So I set out to get the tests to run. But it would fatal:
Fatal error: Interface 'Assetic\Asset\AssetInterface' not found in … AssetInterface.php. While trying to get that to work, I failed miserably — class loading has been all-automagically for me so far and I have no knowledge about that part of Drupal 8. I got similar failures when I tried to use that interface from a .module file that I knew was being executed.Hence, I'm also unable to do any meaningful further development, because I cannot actually run any of the new code.
For the same reason, the two above patches should fail — vendor class loading in D8 is currently broken, it seems (except for Symfony stuff)? I couldn't find a relevant issue. Somebody else hopefully knows?
In the mean time, here's a reroll with nitpicks fixed that will surely arise.
#15
ah, you beat me to being able to explain the issue with the classloading - very frustrating, that error. it's because our classloader decides it's a PEAR-style library rather than being properly namespaced & PSR0. woohoo. it would probably be fixed by #1658720: Use ClassLoader instead of UniversalClassLoader (thanks for the tip, @dawehner), but in the meantime, i've rolled an updated patch including the two-character fix that makes it work on my local. *facepalm*
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.
#16
The last submitted patch, assets-1762204-15.patch, failed testing.
#17
Rerolled #14 with changes from #15
#18
The last submitted patch, core-assets-1762204-17.patch, failed testing.
#19
Failing for the right reasons at least.
#20
OK, 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
AssetLibraryManagerand 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. theAssetLibraryManagerapproach 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.
#21
this will fail (intentionally), but let's change status so other people can see where those failures are :)
#22
The last submitted patch, core_assets-1762204-20.patch, failed testing.
#23
+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.
#24
ok, issue summary is updated. i need to let this sit for a little while, as i have to shift my focus over to the Response stuff, as well as filing an initial issue dealing with master displays. i'm hoping somebody else will be interested in picking up at least some of what's here...i'm more than happy to answer any questions as needed :)
#25
@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?
#26
@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.#27
@sdboyer I'm working on #1344648: Reincarnate graph class? first to make Unittest for it. Next I'll try this issue. Then Graph.inc
#28
fantastic. ping me when you have something that i can see about dropping into the sort() method. in the meantime, i've made a DX request in that other issue.
#29
@sdboyer I did not succeed in moving fast enough :(
Here is my small effort :(
<?php
protected function sort() {
if (!$this->needsSorting) {
return;
}
$this->sorted['js'] = $this->sorted['css'] = array();
$this->sorted['js'] = $this->_sort($this->getUnsortedJs());
$this->sorted['css'] = $this->_sort($this->getUnsortedCss());
$all = array();
$this->needsSorting = FALSE;
}
protected function _sort($items) {
$g = new DirectedAcyclicGraph();
foreach ($items as $item) {
$from_id = spl_object_hash($item);
$g->addNode($from_id, $item);
$deps = $js->getDependencies();
foreach ($deps as $dep) {
$to_id = spl_object_hash($dep);
$g->addNode($to_id, $dep);
$g->addLink($from_id, $to_id);
}
}
$tsl = $g->getTSL();
$result = arrau();
foreach($tsl as $id) {
$result[] = $g->getNodeData($id);
}
return $result;
}
?>
The patch adds a new vendor https://github.com/clemens-tolboom/graph as I think we really need real graph util for Drupal and doing it through GraphAPI contrib module takes way too much time I can handle.
An alternative
function _sort() {}would be allowing for cycles thus use a DirectedGraph then convert it to a DirectedAcyclicGraph allowing for errors like<?phpprotected function _sort($items) {
$g = new DirectedGraph();
foreach ($items as $item) {
$from_id = spl_object_hash($item);
$g->addNode($from_id, $item);
$deps = $js->getDependencies();
foreach ($deps as $dep) {
$to_id = spl_object_hash($dep);
$g->addNode($to_id, $dep);
$g->addLink($from_id, $to_id);
}
}
// We need to ignore cycles?
$dag = DirectedAcyclicGraph::fromDirectedGraph($g, TRUE);
$tsl = $dag->getTSL();
$result = arrau();
foreach($tsl as $id) {
$result[] = $g->getNodeData($id);
}
return $result;
}
?>
Feel free to copy the vendor/graph into Drupal namespace but I'll focus on the github lib + Graph API.
#30
With patch :p
#31
The last submitted patch, core_assets-1762204-29.patch, failed testing.
#32
Hmmm ... test will fail as the classes are not yet registered by the autoloader. That lead to https://packagist.org/packages/submit to submit https://github.com/clemens-tolboom/graph (which I hesitate to do) to discover https://github.com/clue/graph has way more code let alone tests. It lacks the ability to attach data.
@sdboyer: sorry. I for me am out of ideas :(
[edit: fixed wrong link to https://packagist.org/packages/zetacomponents/graph /edit]
#33
From the log
Fatal error: Class 'Graph\Component\Graph\DirectedAcyclicGraph' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Asset/AssetBag.php on line 183FATAL Drupal\system\Tests\Asset\AssetAssemblyTest: test runner returned a non-zero error code (255).
#34
@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
<?phpprotected function _sort($items) {
$core_id = '__CORE__';
$g = array();
foreach ($items as $item) {
$from_id = spl_object_hash($item);
$g[$core_id]['edges'][$from_id] = 1;
$deps = $js->getDependencies();
foreach ($deps as $dep) {
$to_id = spl_object_hash($dep);
$g[$from_id]['edges'][$to_id];
if (!is_array($to_id)) {
$g[$to_id]['edges'] = array();
}
}
}
$graph = new Graph($g);
$g = $graph->searchAndSort();
return $g[$core_id]['paths'];
}
?>
but that will fail due to the HEAD to TAIL bug mentioned but not fixed in #1915308: Make the graph component input an edge list and add topological sort.
#35
Ok; 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 anAssetBagInterfaceor similar (as originally proposed by sdboyer) but this is very much a work in progress.Drupal\Component\Graph\Graphnot satisfying our needs, I've knocked up a specialisedAssetGraphclass 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)).AssetBagis 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 anAssetBagproperty, 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
#36
thanks for the review, sorry for my delayed response. and that i'm further delaying a real response until later this week :(
in the meantime, though, any chance you could put up an interdiff vs. #20? i can keep the sandbox updated that way, at least.
#37
Does this help?
#38
About the DAG, we might get away without. In any case everything works when all weight keys are removed from scripts #1945262: hook_library_info() diet.
#39
per the discussion in #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox], marking this postponed - we're working on it in the SCOTCH sandbox.
@nod_ - it's possible, but i still think it's optimal. i'm aiming to get back to this this week (in the sandbox), at which point i'll revisit the DAG as well. honestly, i don't think it'll be that bad - the DAG, while somewhat complex to understand, is not terribly complex to implement and is a very well-defined problem space. and it definitely gives us a robust system to fall back on that, i think, folks will end up finding very intuitive.
#40
Removing the issue summary initiative tag, since sdboyer updated the summary several times since the tag was added.
I haven't read the patches yet, and probably won't until the sandbox is further along, but the benefits listed in the issue summary are cool.
The one thing I question though is:
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.
#41
#42
Per #40, removing the "D8 cacheability" tag. I'd like that tag to be focused on improving D8's caching via what's already available within existing core architecture (e.g., #attached and Drupal render cache). This issue remains a cool idea though on its own merits.
#43
In short, I'm worried about the state of this. Using Assetic is what will bring us better CSS minification, proper JS minification, other optimizations, and possibly most importantly: the ability to swap any Assetic filter (minifiers, mostly) for another one, in contrib.
No commits have been made to the
1762204-assetsbranch 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#attachedand move that over to thePartialResponsestuff that's being worked on over at #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox].Thoughts?
#44
all new commits are in the princess branch, i merged the
1762204-assetsbranch 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.
#45
#35: assets-1762204-201302270802.patch queued for re-testing.
#46
The last submitted patch, assets-1762204-201302270802.patch, failed testing.
#47
...as per #39.
Unless of course you intent to work on this Robin. (?)
#48
@klonos I'm currently trying some things in D8 with Assetic, can this issue stays open?
#49
...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.