Add the ability to attach asset libraries directly from a template file

Added \Drupal::service('template.attachment.manager') with getAttachments and addAttachments.

• Renamed "AttachmentManager", not "TemplateAttachmentManager" because it is in the Template namespace already.
• Added a unit test for AttachmentManager.
• Cleaned up the attachments array in AttachmentManager.

It feels a bit odd

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -76,6 +77,7 @@ public function getFunctions() {
+      new \Twig_SimpleFunction('addLib', array($this, 'addLibraries')),

I kinda hate requiring a shortname, ... note: do we really use camel casing for those methods?

This is the result of a conversation with @dawehner on IRC but it doesn't work yet.

Note that to use this form, there are no parameters to the twig function:

{%
set libraries = [
  'some-library',
]
%}
{{ add_libraries() }}

That seems rather unusual, or in other words it feels magical. Why not pass in the libraries array? Or what is the reasoning for special casing it like this?

@Cottser any sufficiently advanced technology will seem like magic. Or, I'm groping in the dark. It's one or the other.

This is #12 but changing the command name to 'add_libraries' which matches the pattern of the other extensions.

I feel this needs a unit test of the Twig extension itself in addition to the unit tests on AttachmentManager.

It may be the other extensions don't have tests. For example, I grepped the codebase for 'url_from_path' and found nothing besides its definition in TwigExtension.

A WebTest for this would go in Drupal\system\Tests\Theme.

IS update, match function name to latest patch.

@cilefen regarding 'url_from_path', it's a throw back for the people who aren't using routes but are used to D7 paths. It may not survive as it seems we are aiming for routes only... but I don't think that is possible in reality so we'll see what happens there.

If I remember right, it was to placate my resistance to change:P

Supporting this. Twig can do it, people will expect something like this to work in Drupal templates.

I'm not up to date on twig so do we need to have a set and add_libraries() call? can't we have both at the same time, like:

{{ add_libraries([
    'special-css-library-1',
    'special-js-library-1',
    'some-library-we-know-is-defined-in-themename.libraries.yml',
  ]) }}

Just asking, the proposed solution in IS looks good enough to me.

@nod_ yes, you can have it like that. Personally I prefer it that way. Though a few think it's easier to read with the set, so we've done it that way fairly consistently that way with adding css classes on attributes for example.

There have been changes to how assets are managed recently, so this patch is just the twig function but it doesn't do anything.

Related: #2451411-9: Add libraries-override to themes' *.info.yml.

I went with

{{ attach('extension/library_name') }}

instead. This feels more natural. One library per call. More than one call (and thus more than one asset library) per template signals poor library structure. If multiple libraries need to be loaded, then it's 99.9% certain you should be using library dependencies!

I converted Bartik's messages template to attach the bartik/messages library (which I also split off from bartik/global-styling — meaning that that CSS is now only loaded when necessary).

Does this mean we now want to remove the ability to set $variables['#attached'] in preprocess functions? I.e. remove the bottom if-test from this bit of code in ThemeManager:

    if (isset($info['preprocess functions'])) {
      foreach ($info['preprocess functions'] as $preprocessor_function) {
        if (function_exists($preprocessor_function)) {
          $preprocessor_function($variables, $hook, $info);
        }
      }
      // Allow theme preprocess functions to set $variables['#attached'] and use
      // it like the #attached property on render arrays. In Drupal 8, this is
      // the (only) officially supported method of attaching assets from
      // preprocess functions. Assets attached here should be associated with
      // the template that we're preprocessing variables for.
      if (isset($variables['#attached'])) {
        $preprocess_attached = ['#attached' => $variables['#attached']];
        drupal_render($preprocess_attached);
      }
    }

Or is it worth having both supported?

@davidhernandez I think 'attach' is the word to use because it matches the terminology elsewhere.

I forgot to update the test coverage. Easy fixes.

I think 'attach' is the word to use because it matches the terminology elsewhere.

My reasoning exactly.

This is naming, but I think attach_library() is better, that also matches the internal name better.

The rest looks good to me :).

attach_library() suggests other things may be attached. I don't think we want templates attaching feeds, headers or <head> elements. If we agree on that, then the conclusion is that we only want one thing to be attachable by templates: libraries. Then IMO attach() is better than attach_library().

(That was my reasoning. Should've posted that right away.)

The function having only one parameter, I agree. Dependencies and all that, if more are needed several calls to the function is not the end of the world.

We have to keep #attached in preprocess, module can/will use it and they shouldn't need to have users adding stuff to their templates to do so.

I agree with Fabianx about the naming though. It's arbitrary to say "attach" means "attach libraries only" in the context of twig, it is also counter-intuitive compared to the rest of the API. To me it's the same discussion we had optimized/minified in library declaration: attach_library is more explicit and can't be misunderstood without a lot of bad faith, very easy to think attach could means something else. As you said, we want to make sure people understand only libraries can be attached in templates. On top of that, the PHP method is called attachLibrary, let's keep things consistent.

Solid reasoning — thanks! Rerolled accordingly.

This is not a feature request, but a task.

This would be RTBC, but still needs tests.

And its nice to get the messages into its own library as a good example - but need bartik approval for that by @emmamaria. Having @Cottser and/or @joelpittet look over this would be good, too.

This needs a beta evaluation and possibly a change record or change record update.

will do tests for this

Been playing around with this, but I couldn't get it to work. It's probably just too late. I'll give it another go tomorrow. As mentioned (on Twitter), I like this idea!

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -257,4 +278,24 @@ public function isUrlGenerationSafe(\Twig_Node $args_node) {
+   * This makes for a much better TX.

Small comment: I fear people won't know what TX means looking at this.

Not putting needs work because of that, just because of the tests :)

Edit: it definitely works for me, @sqndr make sure you clear caches after applying the patch?

I'm a fan! RTBC++ after the doc cleanup.

Here's a change record it could use a few eyes and touchups if you have them.

https://www.drupal.org/node/2456753

I fear people won't know what TX means looking at this.

Agreed. Also got it working ;) Again: I like this!

What I tried yesterday was:
- Copy file from html.html.twig from core/system/templates into own theme
- Add {{ attach_library('panda/kitten') }} (same line as screencast above)

Result:
- No kitten.css is added to the page

Remarks:
- As mentioned, cleared the cache. Works for page.twig.html.

Something I did wrong?

One thing I wonder is if the library added this way is available in preprocess function?

How would we go about getting rid of a library added from a template, is it something that should even be possible?

I updated the change record.

@sqndr Please stop embedding multi-megabyte GIFs. They make loading d.o issues terribly, terribly slow. It's fine to attach them, just don't embed them. Or, alternatively, just attach screencasts. They'll be smaller too!

Addressing remaining feedback.

1. Removed the "TX" line.
2. Slightly improved the CR.
3. Added tests. (Inspired by those added in #2308187: Provide a twig extension for file_create_url.)

One thing I wonder is if the library added this way is available in preprocess function?

How would we go about getting rid of a library added from a template, is it something that should even be possible?

The same problem exists for preprocess functions. The only way to remove this is to write a subtheme and override the parent theme's preprocess function.

Similarly for templates: you'd have to override the template in a subtheme. Or, if the attach_library() is in a Twig Block, then you can choose to extend the base theme's template and only override that block.

The problem described in #53 and my Twitter response cited in #56 need some more explaining probably.

While rendering a page, we're rendering many render arrays, theme functions and templates. The attached libraries bubble up as expected. But then we reach a point where we've rendered everything in the page except the <html> itself: we've rendered page.html.twig (which represents the contents of the <body>) and everything it contains. However, at some point, we need to render the <html> tag, which contains <body> (and thus the rendered page.html.twig template). But it also contains the <head>, which must list all assets to be loaded.

And that is where things get tricky. The html.html.twig template's preprocess function (template_preprocess_html()) generates the styles, scripts and scripts_bottom variables, which the html.html.twig template prints.

The template_preprocess_html() preprocess function preprocesses variables for the template, and then the template is handled. So any asset_library() call in the html.html.twig template simply is too late.

Therefore THEME_preprocess_html() and attach_library() in html.html.twig are impossible to support.

That sounds reasonable to me, no issue left for me, Thanks!

Sorry 'bout the gifs. Still looks good to me.

RTBC, looks great!

I wonder how this

+++ b/core/themes/bartik/bartik.libraries.yml
@@ -22,7 +22,6 @@ global-styling:
-      css/components/messages.css: {}

+++ b/core/themes/bartik/templates/status-messages.html.twig
@@ -24,6 +24,7 @@
+    {{ attach_library('bartik/messages') }}

I wonder how this plays out with css aggregation?

Somewhat related - I am merging those dependency setter methods. #2416831: Add an active_theme twig function

#65: The same as if added in a theme preprocess function via #attached, which we support.

It only does not work, the same as for preprocess_html() for html.html.twig, but that is as CSS is already aggregated at that point.

#65: like #67 says: it's exactly the same as HEAD.

(In Drupal 8.1.0, we should leverage the asset libraries dependency tree that we now finally have in D8 to group assets in a smarter way. But that's completely independent and out of scope for this issue.)

Yes we can cause extra aggregations by using #attached in HEAD - but this patch changes things in bartik to cause extra aggregations on every page that has a message. Given that often when adding a message people are doing a value that adds value to your site does this make sense?

Let's just revert the changes to bartik and then I think we're fine to proceed here. The beta evaluation needs an update because it calls this a feature yet the issue is a task.

I think he change to Bartik is a net improvement: messages are shown *rarely*, and almost only after a POST, so it's logical for messages' CSS to be excluded from the global styling (which is loaded for every page) of a theme.

I don't think there is a real reason to exclude messages.css by default because the improvement is so small. I think its more about what kind of example we want to give in the core theme for this kind of situation. What I think is that messages-block is one of the best examples to demonstrate how people should use libraries (only load attachments when they are necessary) so probably it would probably make sense to include the attachment in the template even though in this example the improvement made to rest of the pages is minimal.

Yes, the net win is minimal, but the principle is right: only load assets when necessary, and definitely those that are needed only a small fraction of the time.

So, yes, it's more about the example than the performance gain. (That's also why there's no perf-related tag on this issue.)

Tentatively re-RTBC'ing.

@Wim Leers but is there not a tension here? Downloading fresh aggregated css assets because they include different libraries has a significant cost to it - right?

This got discussed a lot on #769226: Optimize JS/CSS aggregation for front-end performance and DX.

In this case messages.css would be a case for a file that never gets aggregated. So you get the extra HTTP request/cache miss when messages are shown, but not the cost of generating and downloading the new aggregate.

Back to CNR - I think we should consider setting preprocess: false on the messages.css file, that'd only be bad for a site that is showing a permanent drupal_set_message() for some reason (which I have seen, but is unusual). Otherwise it gets rid of the main trade-off here between showing it everywhere or downloading duplicate files.

+1 to setting preprocess: false here.

As is, when CSS aggregation is on, there is a new aggregate file containing only message.css, it's not invalidating existing aggregates, just add one more file to download.

How stuff is aggregated could use an improvement but anyway, preprocess if off for messages.css.

Back to RTBC, very nice and concerns addressed!

+1 — makes sense :)

Wow #1924522: Remove separate CSS_AGGREGATE_THEME aggregate file never actually made it in, @nod_ does that explain #79 or is something else happening?

Thanks, looks like I'm a bit rusty :)

Okay my concerns have been addressed - thanks everyone! Committed a2efc8a and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Almost feel like we should fill follow-ups to move a bunch of stuff in templates now that we can. Like vertical tabs, progress bar, dropbutton, details polyfill, etc.

I think this issue broke some peoples Twig Extensions because now you are forced to add arguments: ['@renderer'] in services.yml. Btw noticed that exceptions doesn't work very well on situations like this..

@lauriii Those people should be extending the vendor \Twig_Extension, not core's. See also #2408013-88: Adding Assertions to Drupal - Test Tools. and the Twig extension part of https://www.drupal.org/node/1831138.

Hi,

If I'm not wrong, I should be able to load a library from any twig template of my theme. But I can not do it from a template like page--imprescindible--%.html.twig, that in the other hand is loading well its html markup.

When I try to load the same library from html.html.twig its OK. I use:

{{ attach_library('basic/imprescindibles_section') }}

Well, is it working as desired? Reading here and there it seems I should be able to load a library from any theme twig template, but drupal documentation suggest other way to add a library for certain pages:

https://www.drupal.org/docs/8/theming-drupal-8/adding-stylesheets-css-an... (look at "Attaching a library to a subset of pages" section)

The situation is the opposite you can read here: https://sqndr.github.io/d8-theming-guide/libraries/libraries-and-twig.html and not the described here: https://medium.com/@ToddZebert/loading-and-using-javascript-in-drupal-8-...

Then, I wonder if I have found a bug? or I can only use attach_library from html.html.twig? If the second is true, then I'd edit drupal.org documentation trying to help with all this information mess.

In fact, at drupal documentation you can read:

"Attaching a library via a Twig template
You can attach an asset library to a Twig template using the attach_library() function in any *.html.twig"

If that's true, I should be able to load my library from page--imprescindible--%.html.twig

Just resetting issue status, and related stuff

#3050386: Allow loading CSS and JavaScript directly from templates is proposing to take this even further.