Allow attributes passed with CSS in libraries (SRI)

Adding relationship to another issue that is the same

Setting back to CNW for tests.

This related issue is the 7.x equivalent.

I've been working on #2851748: Secure sites by adding attributes "integrity" and "crossorigin" to CDN files and can confirm that attributes passed to JS already works:

However, CSS does not:

I'm confirming that the issue still exists in HEAD. I haven't actually tried or really reviewed the patch, yet.

I'm just adding this to the scope of issues that really needs to be resolved.

I'm half tempted to mark this as a task or even as a bug since this directly affects the ability (natively) to provide proper SRI values for external resources. Technically speaking though, this adds new [desired] functionality that previously wasn't possible... so I suppose I'll leave it as a feature request; for now. If someone else feels this needs recategorization, please mention it here so we can see how we want to move forward.

The official documentation says that this should be possible. I'd aggree to consider this as a bug :)

My 2 cts.

Tagging for Seattle sprints

@LittleCoding, sorry, this wasn't on my radar while I was in Seattle.

That being said, seeing how nothing happen in Seattle with this issue, removing the tag.

Changing to a bug and bumping priority since existing documentation conflicts with actual behavior (per #28).

Setting back to CNW since there are no tests.

New patch for drupal 8.7

patch that failed on #36 is rerolled and corrected on 8.8.x

Thanks!

This still needs tests, so I'm going to switch it back to Needs Work I'm going to try to work on the tests this week.

Ok, new patch, the only change that I did to #37 (besides to add the test) was that I changed the array_replace parameters order, it was moving the rel, media and href to after the custom attributes making the tag look different than the others link tags.

So with the previous version, it was printing the tag like this:

<link integrity="sha384-oS3vJWv+0UjzBfQzYXAZQ7Ay" crossorigin="anonymous" rel="stylesheet" media="all" href="//use.something.css" />

And with my change it is printing it like this:

<link rel="stylesheet" media="all" href="//use.something.css" integrity="sha384-oS3vJWv+0UjzBfQzYXAZQ7Ay" crossorigin="anonymous" />

And that's it.

Thanks for all the help on this one.

David.

(why was added the "api change" tag )

HEAD is always fixed first. Whether or not the fix is backported to previous versions is irrelevant.

Reviewed it and I have a questions, and a bug.
Tests look good to me.

      // Attributes may only be set if this stylesheet is output independently
      // as a LINK element.
      if (!empty($css_asset['attributes'])) {
        $element['#attributes'] = array_replace($element['#attributes'], $css_asset['attributes']);
      }

Bug: This doesn't check if the css is a group (ie preprocessed) - adjusting the if statement to if (!$css_asset['preprocess'] && !empty($css_asset['attributes'])) { should resolve this.

Question: Why use array_replace instead of +=? += is used in the equivalent functionality for JS, and is a tiny bit better performance (given caching that's not really a major concern and it is a minor performance difference I believe)

Question: Why use array_replace instead of +=? += is used in the equivalent functionality for JS, and is a tiny bit better performance (given caching that's not really a major concern and it is a minor performance difference I believe)

No idea, I'm going to change it in my next patch.

I added the changed that you suggested and it make the test to fail. I will try to dig a bit more on this later this week.

Thanks for your review!

Testing Drupal\Tests\Core\Asset\CssCollectionRendererUnitTest
..F...                                                              6 / 6 (100%)

Time: 5.26 seconds, Memory: 4.00MB

There was 1 failure:

1) Drupal\Tests\Core\Asset\CssCollectionRendererUnitTest::testRender with data set #2 (array(array(0, 'file', 'all', true, 'public://css/file-all', array(), array('sha384-psK1OYPAYjYUhtDYW+Pj2yc', 'anonymous', 'test'))), array(array('html_tag', 'link', array('stylesheet', 'all', 'file_url_transform_relative:f...-all?0', 'sha384-psK1OYPAYjYUhtDYW+Pj2yc', 'anonymous', 'test'), array())))
Failed asserting that Array &0 (
    0 => Array &1 (
        '#type' => 'html_tag'
        '#tag' => 'link'
        '#attributes' => Array &2 (
            'rel' => 'stylesheet'
            'media' => 'all'
            'href' => 'file_url_transform_relative:file_create_url:public://css/file-all?0'
        )
        '#browsers' => Array &3 ()
    )
) is identical to Array &0 (
    0 => Array &1 (
        '#type' => 'html_tag'
        '#tag' => 'link'
        '#attributes' => Array &2 (
            'rel' => 'stylesheet'
            'media' => 'all'
            'href' => 'file_url_transform_relative:file_create_url:public://css/file-all?0'
            'integrity' => 'sha384-psK1OYPAYjYUhtDYW+Pj2yc'
            'crossorigin' => 'anonymous'
            'random-attribute' => 'test'
        )
        '#browsers' => Array &3 ()
    )
).

/var/www/html/web/core/tests/Drupal/Tests/Core/Asset/CssCollectionRendererUnitTest.php:283

ah yes, of course... sorry, actually do:

if (empty($css_asset['preprocessed']) && !empty($css_asset['attributes'])) {

using the other form wouldn't work for all cases.

Hi LittleCoding

Thanks for your help but your patch does not contain the test added on #39 so even if it pass it will need work.

Also you should add interdiffs so we can know what was changed.

Also, I'm curious why did you change the approach taken since #37? it seems that it was almost ready.

@gnuget The change in coding is to match the method used in the JsCollectionRenderer class, we should not have each doing their own thing.

Makes sense, thanks.

Great point about keeping the testing. would you like to redo the patch with that included?

I can do it later today/this week, feel free to do it if you have time before than me. :-)

+          } else {
+            // Prevent attributes from being added to an aggregate file.
+            $css_asset['attributes'] = [];
           }

In which case this else is going to be executed? it is possible to add custom attributes to the aggregated links generated by drupal?

Also, this else is not present in the JsCollectionRenderer either, so not so sure if I'm missing something here.

Ok, new patch using the code of LittleCoding (using the same approach as JsCollectionRenderer) and re-adding the tests that I added in #39.

Also, @LittleCoding, I removed your else because I didn't found a way to add custom attributes to the aggregated files, if there is a way just let me know and I'm going to be happy to restore that change (and add code to test this part of the code).

Question: Why use array_replace instead of +=? += is used in the equivalent functionality for JS, and is a tiny bit better performance (given caching that's not really a major concern and it is a minor performance difference I believe)

Well, according to #2623960: Improve the merge() method on Attribute objects, this should really be using array_merge_recursive(). As far as "why", it's so that it can override any already set attributes that core may have made on the assumption of the file in question; more control.

Using += actually means they can't be overridden as it will simply ignore any keys that already exist in the array.

+++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.php
@@ -61,6 +61,9 @@ public function render(array $css_assets) {
+          } else {
+            // Prevent attributes from being added to an aggregate file.
+            $css_asset['attributes'] = [];
           }

I don't feel like this is the best approach.

An asset could be external and not preprocessed, but still need attributes.

I don't feel like this is the best approach.

An asset could be external and not preprocessed, but still need attributes.

Actually if you see my patch, I didn't add that else :-p

Well, according to #2623960: Add a merge() method on Attribute objects, this should really be using array_merge_recursive(). As far as "why", it's so that it can override any already set attributes that core may have made on the assumption of the file in question; more control.

This might be worth it to create a follow-up to change JsCollectionRenderer as well?

I noted that you changed again the issue to needs work.

What needs to be addressed to changed it to needs review again?

The patch #48 working great. I just only removed the else part. Now i am adding updated patch here.

+++ b/core/lib/Drupal/Core/Asset/CssCollectionRenderer.php
@@ -72,6 +75,11 @@ public function render(array $css_assets) {
 
+      // Attributes may only be set if the stylesheet is outputed independently.
+      if (!empty($element['#attributes']['href']) && !empty($css_asset['attributes'])) {

Should be 'output' here.

I think we need test coverage for this.

Does this needs a change record even if in the official documentation says that this should already work?

I think that this is why this is marked as a bug and not as a feature request.

https://www.drupal.org/docs/8/theming/adding-stylesheets-css-and-javascr...

This looks pretty good, thanks LittleCoding.

I'm going to remove a few tags:

• API change: Given that the documentation explicitly says that this should already work on this way there is not an API change so no need to have this tag.
• Needs tests: Since #39 the patch has a test that covers this.
• Needs change record: Given that the documentation explicitly says that this should already work on this way no
  change record needed..
• Needs issue summary update: This was updated at #68.

This seems ready to me. Thanks!

Fixing test failure in #69. Adjusted the indexes in the data provider as index 1 is missing.

Refactored tests by using Prophecy.

 /**
   * {@inheritdoc}
   */
  protected function setUp(): void {

Added missing comment and added return type hinting which is mandatory in Drupal 9 now.

Rerolled for 9.1.

Also, since #2897408: Remove IE9 support from CssCollectionRenderer and provide it in contrib instead the renderer only outputs link tags, so the conditional can be simplified.

Crediting people from the duplicate issue.

Committed and pushed 758adf0e97 to 9.1.x and ea20f12a26 to 9.0.x. Thanks!

We need a backport patch. I think that'd be #74 without the protected function setUp(): void {

Updated patch #74, without the protected function setUp(): void { as mentioned in #83, please review.

Hi @mrinalini9

Thanks for porting the patch!

Whenever a new patch is provided it is recommended to provide an interdiff so we can see what changed between patches.

You can read here how make interdiffs.

For this time I made it for you, It is attached.

And looks great!

Thanks again.

Discussed with @lauriii we decided to hold off on committing this to 8.9.x until after 8.9.0 because of the possibility of existing sites breaking because of incorrect integrity hashes in css component definitions. The plan is to commit this early in 8.9.1 dev cycle to give sites that have incorrect libraries the best chance of discovering this before release. Note that Drupal 9.0 will support this so if you 100% needs this to work you can apply the 8.x patch or move to Drupal 9 :)

To this end I've created the change record https://www.drupal.org/node/3144708

This is definitely a security improvment.

As in #78, the condition can be simplified since the element is always a link element with an href attribute since 8.7

The same version as in comment #91
But with replacing
From

$element['#attributes'] += $css_asset['attributes'];

To

$element['#attributes'] = NestedArray::mergeDeep($element['#attributes'], $css_asset['attributes']);

What allows to override type and other default attributes.

What's the reason for wanting to be able to change those default attributes? Is there not currently a way to change those attributes when necessary?

Since the current patch is backporting changes from D9 to D8.9, this change should probably be a separate issue so that it's committed to the latest release first. Since this same behaviour is done separately for JS and CSS, a patch should address both renderr classes for consistency.

There seems to be some conjecture between #91 and #95 here - can we make sure the latest patch is the one under consideration? Thanks in advance

Hey,
The attribute now works and i'm able to add a title for instance.
changing default rel="stylesheet" to rel="alternate stylesheet" doesn't seem to work"

"libraries.yml":

css:
    theme:
      style/style.css: {}
      style/print.css: { media: print }
      style/styleguide.css: { type: "css", attributes: { rel: "stylesheet alternate ", title: "alternate-css" } }

Any idea why ?

@LittleCoding Thanks for pointing that up, went past that one :)
I do think it's relevant to allow that out of the box.

Re-upping #91 since that is the patch that should be considered for backport. #95 offers a different feature set and would need to be changed against 9.2.x first.

Discussed with @catch and we agreed to backport this to 8.9.x

Committed e880e48 and pushed to 8.9.x. Thanks!

Hello everyone, any chance we could see a compatibility of this on Drupal 9.3.x ?

@KarimBou The change to allow additional attributes according to the original scope of this issue was committed to 8.9+, and 9.1+

If you're interested in overriding default attributes as in #94, you should open a separate issue as previously mentioned.