Problem/Motivation

Stealing this idea from dawehner!

We are now using libraries for all CSS and JS. To add any of these libraries in a theme, they either have to be added site-wide or a theme needs to use a preprocess function to specify when they get added. It would be great if a themer had the ability to add a library directly from a template, to avoid creating preprocess functions (and using php.)

Proposed resolution

Add a Twig simple function.

{# In a Twig template we attach a library of our choosing. #}
{{ attach_library('mytheme/library1') }}

Remaining tasks

Discuss.
Agree.
Do.
Post change record.
Rejoice.

API changes

Adding a function would be an API addition.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category This is a feature because it is an api addition.
Issue priority Normal because it is nice to have, but isn't preventing anything form working.
Unfrozen changes Unfrozen because it is an addition that should really only affect templates, which are not frozen.
Prioritized changes The main goal of this issue is to improve the themer experience.
Disruption This should not be very disruptive because it is an addition, so there are no backwards compatibility concerns. Besides the addition the only noticed change might be in core template files.
Files: 
CommentFileSizeAuthor
#85 interdiff-59to85.txt420 bytesdavidhernandez
#85 add_the_ability_to-2398331-85.patch8.77 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,483 pass(es). View
#79 interdiff.txt420 bytesnod_
#79 core-twig-attach_library-2398331-79.patch8.34 KBnod_
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,116 pass(es), 1 fail(s), and 0 exception(s). View

Comments

davidhernandez’s picture

FileSize
991 bytes

Just getting started making a twig simple function.

cilefen’s picture

Status: Active » Needs review
FileSize
2.66 KB
2.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,772 pass(es). View

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

davidhernandez’s picture

Issue tags: +Needs tests

I'm going to do some more tweaking but this actually functions using addLib in a template. For example, {{ addLib('bartik/maintenance_page) }}

davidhernandez’s picture

FileSize
3.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,843 pass(es). View

Not sure why it didn't add my file.

davidhernandez’s picture

FileSize
3.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch add_the_ability_to-2398331-5.patch. Unable to apply patch. See the log in the details link for more information. View

Changing that from a service to a static class.

Status: Needs review » Needs work

The last submitted patch, 5: add_the_ability_to-2398331-5.patch, failed testing.

davidhernandez’s picture

FileSize
3.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,286 pass(es). View

That was dumb. I diffed my previous commit instead of versus head.

davidhernandez’s picture

Status: Needs work » Needs review
cilefen’s picture

cilefen’s picture

FileSize
5.64 KB
5.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/includes/common.inc. View
  • 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.

Status: Needs review » Needs work

The last submitted patch, 10: add_the_ability_to-2398331-10.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
5.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,307 pass(es). View
dawehner’s picture

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?

cilefen’s picture

FileSize
2.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

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() }}

Status: Needs review » Needs work

The last submitted patch, 14: add_the_ability_to-2398331-14.patch, failed testing.

Cottser’s picture

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?

cilefen’s picture

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

davidhernandez’s picture

Question about the function name. Is there a standard convention we follow? Is camel casing only used for class methods?

cilefen’s picture

Status: Needs work » Needs review
FileSize
748 bytes
5.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,558 pass(es). View

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.

cilefen’s picture

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.

cilefen’s picture

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

joelpittet’s picture

Issue summary: View changes

IS update, match function name to latest patch.

joelpittet’s picture

@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

nod_’s picture

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.

davidhernandez’s picture

@nod_, no, the set should optional. In the example, it is just a cleaner (easier to read) way to send the array.

joelpittet’s picture

@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.

cilefen’s picture

FileSize
997 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,153 pass(es). View

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

Wim Leers’s picture

Issue tags: +TX (Themer Experience)
Wim Leers’s picture

Title: Add the ability to include a library directly from a template file » Add the ability to attach asset libraries directly from a template file
Related issues: +#2451411: Add libraries-override to themes' *.info.yml
Wim Leers’s picture

FileSize
4.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,708 pass(es), 18 fail(s), and 10 exception(s). View
4.52 KB

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?

Status: Needs review » Needs work

The last submitted patch, 30: twig_attach_asset_library-2398331-30.patch, failed testing.

davidhernandez’s picture

Thanks for working on this! :D

I'm not sure 'attached' would feel more natural for themers, but I leave that up for debate. I'm not going to bikeshed on the name.

Does this mean we now want to remove the ability to set $variables['#attached'] in preprocess functions?

I don't think so. I'd want to have both options available, because you don't want to have to have a template just to attach the library. I don't think attaching from the template is the "better" way to do it, just that we want to give themers the ability so they don't need to know the PHP way to do it.

I'm fine with one library per call.

The last submitted patch, 30: twig_attach_asset_library-2398331-30.patch, failed testing.

cilefen’s picture

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,737 pass(es). View
1.6 KB

I forgot to update the test coverage. Easy fixes.

Wim Leers’s picture

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

My reasoning exactly.

Fabianx’s picture

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

The rest looks good to me :).

Wim Leers’s picture

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.)

nod_’s picture

Status: Needs review » Needs work

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.

Wim Leers’s picture

Issue tags: +Needs issue summary update
FileSize
6.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,733 pass(es). View
1.73 KB

Solid reasoning — thanks! Rerolled accordingly.

davidhernandez’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

I agree with nod_. "attach()" sounds more generic.

davidhernandez’s picture

Status: Needs work » Needs review
Fabianx’s picture

Category: Feature request » Task
Status: Needs review » Reviewed & tested by the community

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.

Fabianx’s picture

Status: Reviewed & tested by the community » Needs review
davidhernandez’s picture

Issue tags: +Needs change record

Definitely would need a change record.

lauriii’s picture

Assigned: Unassigned » lauriii

will do tests for this

sqndr’s picture

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!

Cottser’s picture

Status: Needs review » Needs work
+++ 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?

joelpittet’s picture

Issue tags: +Novice

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

joelpittet’s picture

Issue tags: -Needs change record

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

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

sqndr’s picture

Issue summary: View changes
FileSize
2.9 MB

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

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

sqndr’s picture

Issue summary: View changes
FileSize
2.17 MB

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?

nod_’s picture

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?

cilefen’s picture

I updated the change record.

davidhernandez’s picture

From Wim via Twitter, "html template is special: it must render the assets, so it can't add more."

@nid_, I don't know if manipulating it from preprocess is possible, but it doesn't seem like we need it to be possible. The template is at the highest level (or is it lowest?) so adding something there should be considered the end of the line. If someone needs more complex logic, the library probably shouldn't be added at the template level.

Also, agreeing about the "TX" line. The whole line is probably superfluous, but we can at least spell it out if it is kept, or make it more detailed.

Change record looks good.

Wim Leers’s picture

@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!

Wim Leers’s picture

Assigned: lauriii » Wim Leers

Addressing remaining feedback.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests, -Novice
FileSize
9.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,973 pass(es). View
4.19 KB
  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.

nod_’s picture

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

sqndr’s picture

Issue summary: View changes
sqndr’s picture

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

sqndr’s picture

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, looks great!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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?

cilefen’s picture

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

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

#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.

Wim Leers’s picture

#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.)

alexpott’s picture

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?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

Wim Leers’s picture

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.

lauriii’s picture

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.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

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.

alexpott’s picture

@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?

catch’s picture

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.

catch’s picture

Status: Reviewed & tested by the community » Needs review

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.

Fabianx’s picture

+1 to setting preprocess: false here.

catch’s picture

Status: Needs review » Needs work
nod_’s picture

Status: Needs work » Needs review
FileSize
8.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,116 pass(es), 1 fail(s), and 0 exception(s). View
420 bytes

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.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, very nice and concerns addressed!

Wim Leers’s picture

+1 — makes sense :)

catch’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 79: core-twig-attach_library-2398331-79.patch, failed testing.

davidhernandez’s picture

#79 is missing the template file used for the test. twig_theme_test.attach_library.html.twig

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
8.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,483 pass(es). View
420 bytes
nod_’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed a2efc8a on 8.0.x
    Issue #2398331 by cilefen, davidhernandez, Wim Leers, nod_: Add the...
davidhernandez’s picture

I published the change record. Rejoice!

nod_’s picture

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.

davidhernandez’s picture

It might be an even better thing for core to do because if people copy the template they can just remove the line if they don't want the library. No need to worry about -remove -override stuff. But it's still case by case. Bartik and Seven certainly have a few places where this can be changed. Maybe we'll start there and not make a project of it all.

lauriii’s picture

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..

Cottser’s picture

@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.

Status: Fixed » Closed (fixed)

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

calbasi’s picture

Version: 8.0.x-dev » 8.2.8
Category: Task » Bug report

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.

calbasi’s picture

Version: 8.2.8 » 8.0.x-dev
Category: Bug report » Task

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

calbasi’s picture

Version: 8.0.x-dev » 8.2.8
Category: Task » Bug report

Just resetting issue status, and related stuff

davidhernandez’s picture

Version: 8.2.8 » 8.0.x-dev
Category: Bug report » Task

This is a closed issue. If you think you've found a bug, please open a new issue.