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
- Define a CSS asset with
minified: truewhich has a relative path to a font. - Enable aggregation. Observe that the font does not load.
- 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.
| Comment | File | Size | Author |
|---|
Issue fork drupal-3535330
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
Comment #7
benabaird commentedFirst 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.
Comment #8
benabaird commentedAh 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.
Comment #9
benabaird commentedComment #10
quietone commentedIn 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
Comment #11
nicxvan commentedYep changes should be on 11.x and backported.
Comment #12
dalinJust 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.
Comment #13
herved commentedHello, 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
Comment #18
needs-review-queue-bot commentedThe 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.
Comment #19
joelpittet@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?
Comment #20
anybodyConfirming this major issue if aggregation is enabled.
Comment #21
joelpittet@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.
Comment #22
anybodyThanks, I'll ask @thomas.frobieter to give it a try in the affected project!
Comment #23
thomas.frobieterI can no longer see any problems, it works perfectly again!
Comment #26
catchCommitted/pushed to 11.x and cherry-picked to 11.2.x, thanks!