Problem/Motivation

The change in Add support for minified external CSS libraries has made it so that CSS assets marked as minified no longer get URLs contained in them rewritten upon aggregation.

Steps to reproduce

  1. Define a CSS asset with minified: true which has a relative path to a font.
  2. Enable aggregation. Observe that the font does not load.
  3. Disable aggregation. Observe that the font does load.

Proposed resolution

I think the check added here should be on $css_asset['type'] rather than $css_asset['minified'].

Remaining tasks

Write a fix and update tests.

Issue fork drupal-3535330

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

benabaird created an issue. See original summary.

benabaird changed the visibility of the branch 11.x to hidden.

benabaird changed the visibility of the branch 3535330-assets-paths-in to hidden.

benabaird’s picture

Status: Active » Needs review

First time doing this, I guess I opened a request somehow on 11.x. Sorry for the noise.

I've changed the check to look at the asset type rather than whether it's minified. https://git.drupalcode.org/project/drupal/-/merge_requests/12687 has green tests, so marking this as needs review.

benabaird’s picture

Status: Needs review » Needs work

Ah just ran into an issue on this, will need to check the solution more.

False alarm, the issue was caused by me testing this incorrectly, not the fix.

benabaird’s picture

Status: Needs work » Needs review
quietone’s picture

Version: 11.2.x-dev » 11.x-dev

In Drupal core changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies. Thanks

nicxvan’s picture

Yep changes should be on 11.x and backported.

dalin’s picture

Issue tags: +Needs tests

Just ran into this when all my custom fonts disappeared.

I think this MR also needs a _new_ test to confirm that relative paths are being replaced as expected.

herved’s picture

Hello, thanks for creating this issue.
I did hit this as explained in #3414173-37: Add support for minified external CSS libraries (comments 37-39).
But I also hit another issue which relates to #3416508: Skip generating aggregates that won't be used for stale asset requests
Both are closely related so I opened #3536795: "Only file JavaScript/CSS assets can be optimized" errors in logs and proposed a slightly different approach.
Any feedback is welcome :) Thanks

joelpittet made their first commit to this issue’s fork.

joelpittet changed the visibility of the branch 11.x to active.

joelpittet changed the visibility of the branch 11.x to hidden.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new2.52 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

@dalin I tried my hand to add some more tests for a few of these cases but I didn't want to step on @benabaird's toes so I put it on a new branch MR 12687 with slightly different take on how to solve this and some more comments.

@benabaird I hope this works for you?

anybody’s picture

Confirming this major issue if aggregation is enabled.

joelpittet’s picture

@anybody if by chance you can review and test the solution fixes the problem that would help move it along. It's got tests to go with the direction I am hoping to take this.

anybody’s picture

Thanks, I'll ask @thomas.frobieter to give it a try in the affected project!

thomas.frobieter’s picture

Status: Needs review » Reviewed & tested by the community

I can no longer see any problems, it works perfectly again!

  • catch committed 2f94a274 on 11.2.x
    Issue #3535330 by joelpittet, benabaird: Assets paths in CSS no longer...

  • catch committed ca25750b on 11.x
    Issue #3535330 by joelpittet, benabaird: Assets paths in CSS no longer...
catch’s picture

Version: 11.x-dev » 11.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Status: Fixed » Closed (fixed)

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