Comments

tstoeckler’s picture

Responding to @Wim Leers' comment in #2350877-43: Deprecate/rename drupal_add_feed(), drupal_add_html_head(), drupal_add_html_head_link(), drupal_add_http_header(), and allow to be set declaratively in #attached because I didn't make it over there before that was marked fixed.

First of all, AFAIK Libraries API is the only example of a module needing to have things #attached. Correct me if I'm wrong.

In general the use-case of having more "domain-specific" declarations of assets than the "pure" JS and CSS is valid. I could imagine a "Skinr"-like module for D8 wanting to support things like $build['#attached']['skin'][] = 'foo' or similar, which then gets "expanded" into the real JS and CSS at some point.
In D7 we actually had that use-case in core with $build['#attached']['drupal_add_tabledrag']; again this is nothing than an encapsulation of a bunch of JS and CSS into a "domain-specific" notation. It just happens to be that we circumvented this particular use-case with the introduction of '#type' => 'table' which does that internally.
So I do think having this possibility is valid and not at all far-fetched.

But in Drupal 7, Libraries API is utterly broken in several ways:

I will gladly criticize Libraries API's design - and even rant about it at times - but the next section is entirely off-topic so I won't respond to that. Except for...

Reliance on statics, a la drupal_add_css(), even though #attached was indeed already a soft requirement in D7, to ensure all necessary assets are loaded in case of cache hits: libraries_load($name);

This is slightly off-topic but wanted to set this straight regardless: Please remember that D7 now is not D7 over 4 years ago. drupal_add_css() and drupal_add_js() were anything but deprecated at the time the code was written. You have all the right to complain that it hasn't changed since then, but that's a different story.

Therefore, I think the answer to all of this can be quite simple: Libraries API is in the business of not providing a libraries.info.yml file, since it doesn't come with a static list of available libraries.

I think you mean *.libraries.yml, not libraries.info.yml, right?

Instead, it is in the business of implementing hook_library_info_alter() and populating that with the libraries it provides. So that e.g. mymodule/some-library can declare a dependency on libraries/foo or libraries/react or libraries/angular or …

That is not correct. Information should never be added in *_alter() hooks. That just means we need a hook_libraries_build() for dynamically added library definitions or some such. Thanks for bringing that up, will open an issue.

Then there is no need for $element['#attached']['libraries_load'] = … anymore, because the libraries Libraries API makes available will just be attachable like this: $element['#attached']['library'][] = 'libraries/foo';.

So one thing you're missing here is the variant support. It should be possible to specify the variant of the library to attach. Although I certainly agree that Libraries API could be more helpful here in terms allowing to configure the default based on environment etc. I do think if you explicitly want to load the source version of a library you should be able to. But that could perhaps still be achieved by introducing some specific syntax of the form $element['#attached']['library'][] = 'libraries/foo/minified' or $element['#attached']['library'][] = 'libraries/foo.minified' or something like that.

I'd have to sleep on that proposal but at first sight it looks fairly awesome :-) That way Libraries API wouldn't have a separate API but would allow just using the core API which would probably resolve a lot of confusion of people.
There are a couple of things we would have to sort, but I think it could be doable. One thing that comes to mind, is the dependency system. Libaries API's dependency system a) supports version constraints just like we have it for module dependencies and b) it supports having a dependency which is missing, in which case the loading is aborted. But again, I think those are things we will be able to sort out in the one or the other way.

That could also work in the same way for other modules, i.e. the above use case could be $build['#attached']['libraries'][] = 'skin/foo';.

Thanks for bringing that up!

catch’s picture

Could different variants of the same library just be exposed as different libraries? So you'd have $library, $library_minified, $library_unminified and then $library can change around depending on development mode etc. but the other two are fixed?

hook_libraries_build()

That sounds good and if it blocks an 8.x stable of libraries module should be critical.

tstoeckler’s picture

tstoeckler’s picture

Re #2: Yes, that was exactly what I was trying to hint at with the 'libraries/foo/minified' stuff, but forgot to say explicitly (sorry).

OK, so will mark that one critical then. I agree.

rjacobs’s picture

I just wanted to note that there is an existing Libraires API issue open now that speaks to this exact topic (e.g. have Libraries API create core library entries on behalf of modules that implement hook_libraries): #1386368: Register Libraries in hook_library()

I've made some comments over there in recent days in reaction to the topic that ultimately spun-off this one. What I've stated over there sounds like it's in-line with what's been stated above... though I'm certainly not totally in-tune with all the moving parts (I'm just interested and trying to follow along). Should that existing Libraries API issue be moved against D8 (it started as a D7 issue) to serve as a place for the considerations that are specific to Libraries API?

Could different variants of the same library just be exposed as different libraries? So you'd have $library, $library_minified, $library_unminified and then $library can change around depending on development mode etc. but the other two are fixed?

You would probably need a good way to ensure that only one variant could be loaded at time, correct? You would not want one module loading (via #attached) $library_minified and another loading $library_unminified as then you would be bypassing features Libraries API currently provides to avoid conflicts. Anyway, the concepts around how derivative library entries could be handled (per-module and per-variant) was something that got me thinking (and posting) over in that other issue.

Wim Leers’s picture

#1:

Domain-specific attachments
could imagine a "Skinr"-like module for D8 wanting to support things like $build['#attached']['skin'][] = 'foo' or similar, which then gets "expanded" into the real JS and CSS at some point.

It's true that this could be useful. But I think we long, long ago set out on a course where this was not intended to be supported. Plus, why can't you use asset libraries for that instead?

RE: Libraries API was built in the D6/D7 era
Absolutely! And that's also why I completely understand why Libaries API in D7 works the way it does. But that doesn't mean it shouldn't move on towards D7/D8-era code.
RE: filename
Yes, of course, sorry!
RE: _alter() hooks should not add stuff
I think that's theoretically correct, but at the same time, I don't see the harm in adding libraries? If we really didn't want you to add stuff, the hooks would be implemented in such a way that you can't do that. You're right that this at the very least should be clarified though.
RE: variants
Indeed, configurable which variant should be used site-wide (minified or not), which would probably clear all caches when that setting changes. Indeed, also, that a specific syntax could be introduced to tie to a specific variant.
RE: proposal looking fairly awesome
I'm so glad that you say that, and that I see a smiley in there :) I was afraid I might've hurt your feelings after all, which definitely was not my intention.
I know you've just taken over Libraries API, and it's clear that it's been ported from an earlier time. Changing Libraries API in the middle of the D7 cycle to be "better" would probably caused a lot of frustration also (just like the current implementation causes a different set of frustrations).
In any case, I'm relieved :)
And yes, just using the core API would indeed simplify things a lot, and bring "dependency management integrated with 'core asset libraries'" for free.
That could also work in the same way for other modules, i.e. the above use case could be $build['#attached']['libraries'][] = 'skin/foo';.

Exactly :)


#5:

You would probably need a good way to ensure that only one variant could be loaded at time, correct? You would not want one module loading (via #attached) $library_minified and another loading $library_unminified as then you would be bypassing features Libraries API currently provides to avoid conflicts.

Exactly! This is another reason why I think that a site-wide configuration that "updates" all library definitions to match the setting is preferable.

Fabianx’s picture

So while this is not directly libraries API I have a contrib use case for putting data in #attached - I currently use it for cache-revalidation (which in D7) is the only way to do that.

I assume I will no longer need that, but just pointing out that while the callback part can be replaced with #post_render_cache, there might still be the need to store 'cache meta data'. And nothing else is assets now.

e.g. Fonts - not loaded via CSS/JS?

And probably some other more 'modern' assets / meta data like things.

I think for libraries just improving the core libraries mechanism should be fine, however.

rjacobs’s picture

So here's my attempt at a summary so far:

  1. Most people seem to agree that Libraries API (8.x) should declare core library definitions on behalf of the modules that use its API (when assets are involved). This would allow standard Drupal best practices to be leveraged at load/include time. The pending challenge here is dealing with libraries that are declared to Libraries API multiple times with different variants/files. Constructing a canonical core library definition for such cases, or allowing different variations to be included while still avoiding load conflicts, would require well-defined rules. This could possible be explored in an existing Libraries API issue like #1386368: Register Libraries in hook_library().
  2. Assuming #1, Libraries API needs a way to declare core libraries dynamically. While hook_library_info_alter() already offers a possible way to do this, building new definitions of anything inside an alter hook introduces consistency and integrity problems. A proper solution for this could be finalized in #2358981: Provide a mechanism for dynamic library declarations.
  3. There are probably other cases where the ability to use a generic callback in #attached will be missed, but other existing cases don't seem prolific and have alternatives that don't seem to crippled DX (e.g., #pre_render, additional core library definitions, etc.). Also support for special targeted needs could still be added directly to drupal_process_attached() if warranted in the future.

If that's not totally off-base maybe this specific issue is fairly resolved? I suppose that #1 may not be as cleanly separable from core as I assume though.

tstoeckler’s picture

Status: Active » Fixed

Yes, let's close this one. #2358981: Provide a mechanism for dynamic library declarations is already marked critical so that's fine.

You would probably need a good way to ensure that only one variant could be loaded at time, correct?

You have hit the nail on the head here @rjacobs. I am still not 100% sure that tying this to the core library system will not bite us in the tail due to problems such as this and also it's going to be a lot of work for Libraries API to be remotely usable in D8 (at least as soon as the legacy _drupal_add_js()/css() have been ripped out), but I really don't have any hard objections here, so I'm willing to embark on this adventure :-)

Wim Leers’s picture

And I am willing to help with those interesting challenges, if you want and need the help :)

Status: Fixed » Closed (fixed)

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