Currently the aggregator fails when it encounters a CSS like

background: url(/path/*file*)

because /* is treated as the start of CSS comment.

A proposed fix will be attached in short time. It basically keeps unquoted url() intact.

This was discovered along with a discussion about the possibility of including //-style syntax on www-style, the mailing list for CSS spec development. If //-style comment are introduced in the future, we need this patch to deal with the very general

@import url( http://dont.com/even/look.css )

too, before //-style comments are removed.

CommentFileSizeAuthor
#51 1761498-nr-bot.txt925 bytesneeds-review-queue-bot
#44 1761498-nr-bot.txt1.28 KBneeds-review-queue-bot
#37 css_aggregation_for_comments_in_unquoted_url-1761498-37.patch1003 bytesbandanasharma
#29 css_aggregation_for_comments_in_unquoted_url-1761498-3--29.patch1003 bytesandypost
#25 1761498-25.patch281.9 KBchiranjeeb2410
#21 1761498-21.patch281.9 KBchiranjeeb2410
#17 1761498-17.patch281.9 KBchiranjeeb2410
#15 1761498-15.patch208.26 KBchiranjeeb2410
#10 css_aggregation_for_comments_in_unquoted_url-1761498-3.patch183.31 KBneerajpandey
#3 css_aggregation_for_comments_in_unquoted_url-1761498-3.patch905 byteskennyluck
#1 css_aggregation_for_comments_in_unquoted_url-1761498-1.patch801 byteskennyluck

Issue fork drupal-1761498

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:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kennyluck’s picture

kennyluck CreditAttribution: kennyluck commented
FileSize
css_aggregation_for_comments_in_unquoted_url-1761498-1.patch801 bytes

This patch doesn't handle cases like

u\rl(/path/*file*)

but I don't think we should bother.

kennyluck’s picture

kennyluck CreditAttribution: kennyluck commented
Status: Active » Needs review
kennyluck’s picture

kennyluck CreditAttribution: kennyluck commented
FileSize
css_aggregation_for_comments_in_unquoted_url-1761498-3.patch905 bytes

tweaks

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Needs reroll now that #352951: Make JS & CSS Preprocessing Pluggable has landed. Also needs test coverage. Please take a look at the patch at #2014851-5: Drupal CSS preprocessing breaks protocol-relative paths to get an idea of where to look and how to do it.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Component: base system » CSS
Issue tags: -CSS aggregation

.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +CSS aggregation

d.o fail.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Assigned: kennyluck » Unassigned
Issue summary: View changes

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

neerajpandey’s picture

neerajpandey CreditAttribution: neerajpandey at Google Code-In commented
Status: Needs work » Needs review
FileSize
css_aggregation_for_comments_in_unquoted_url-1761498-3.patch183.31 KB

Reroll patch done.

Status: Needs review » Needs work

The last submitted patch, 10: css_aggregation_for_comments_in_unquoted_url-1761498-3.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
1761498-15.patch208.26 KB

@Wim Leers,

Patch rerolled for #3. Expect a green flag!

Status: Needs review » Needs work

The last submitted patch, 15: 1761498-15.patch, failed testing. View results

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
1761498-17.patch281.9 KB

Status: Needs review » Needs work

The last submitted patch, 17: 1761498-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented

@chiranjeeb2410 looks like you have the D7 branch or something in your patch, want to try again with 8.5.x branch?

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@joelpittet, will do it right away

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
1761498-21.patch281.9 KB

@joelpittet,

Patch rerolled against 8.5.x. Hope it shows green!
Please review.

Status: Needs review » Needs work

The last submitted patch, 21: 1761498-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented

@chiranjeeb2410 see the size of the patch is 281KB. That's a bit big to review. It should be less than 1K like #3 I now realize it's #10 that was where things went askew.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@joelpittet,

It's the reroll of the patch in comment #3, that's needed right, cause the one in #10 failed automated testing.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
1761498-25.patch281.9 KB

Rerolled against latest core. Please review.

Status: Needs review » Needs work

The last submitted patch, 25: 1761498-25.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.5.x-dev » 8.6.x-dev

Please use patch from comment #3 & it still needs tests

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@andypost, been doing that all this time. Dunno why automated tests aren't complying.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
FileSize
css_aggregation_for_comments_in_unquoted_url-1761498-3--29.patch1003 bytes

Here's a straight reroll for d8

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

btw \Drupal\Tests\Core\Asset\CssOptimizerUnitTest does not test processFile(), loadFile()

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs review » Reviewed & tested by the community

@andypost,

Thanks for the patch.
Works as intended. Straight RTBC for me!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Priority: Minor » Normal
Status: Reviewed & tested by the community » Needs work

This reads like at least a normal bug to me. We need automated test coverage of this change. We can add the test coverage to \Drupal\Tests\Core\Asset\CssOptimizerUnitTest

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

bandanasharma’s picture

bandanasharma CreditAttribution: bandanasharma as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented
Status: Needs work » Needs review
FileSize
css_aggregation_for_comments_in_unquoted_url-1761498-37.patch1003 bytes

Re-roll the patch for d9.

tanubansal’s picture

tanubansal CreditAttribution: tanubansal at Salsa Digital commented

Tested #37 on 9.1
Working fine for me
RTBC +1

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
FileSize
1761498-nr-bot.txt1.28 KB

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

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

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

djsagar opened merge request !5592

djsagar’s picture

djsagar CreditAttribution: djsagar at OpenSense Labs commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

djsagar’s picture

djsagar CreditAttribution: djsagar at OpenSense Labs commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
FileSize
1761498-nr-bot.txt925 bytes

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

ameymudras’s picture

ameymudras CreditAttribution: ameymudras at Salsa Digital commented

I can confirm that the patch #37 still applies to 11.x and works as expected. Re rolling to a MR was not needed here. We might need a supporting test here.