[D7] CSS aggregation strips some essential whitespace within strings

Added this test to AdvAgg and it passes.

/* A pseudo selector with essential whitespace wrapped in quotes. */
img[style*="float: right"] {
  padding-left:5px;
}

+1 RTBC from me. AdvAgg has had the new regex in it for quite some time now.

    // Remove certain whitespace.
    // There are different conditions for removing leading and trailing
    // whitespace.
    // @see http://php.net/manual/regexp.reference.subpatterns.php
    $contents = preg_replace('<
      # Do not strip any space from within single or double quotes
      (' . $double_quot . '|' . $single_quot . ')
      # Strip leading and trailing whitespace.
      | \s*([@{};,])\s*
      # Strip only leading whitespace from:
      # - Closing parenthesis: Retain "@media (bar) and foo".
      | \s+([\)])
      # Strip only trailing whitespace from:
      # - Opening parenthesis: Retain "@media (bar) and foo".
      # - Colon: Retain :pseudo-selectors.
      | ([\(:])\s+
    >xS',
      // Only one of the three capturing groups will match, so its reference
      // will contain the wanted value and the references for the
      // two non-matching groups will be replaced with empty strings.
      '$1$2$3$4',
      $contents
    );

This does not look like a direct backport of the Drupal 8 patch - the automated tests are different (and appear to be testing fewer cases), there is one code change missing (the replacement of >xS with >xSs), the required code comment update (from "three capturing groups" to "four capturing groups") is missing, etc. Don't we need all those changes for Drupal 7 too?

Also, minor:

+      # Do not strip any space from within single or double quotes
+        ('.$double_quot.'|'.$single_quot.')

There should be spaces before and after the string-concatenation operators.

Attached is a patch that adds the missing code & comment change and attempts to add the missing tests (I'm not sure I got that part right).

The file names were incorrect. Renamed files quotes.optimized.css to quotes.css.optimized.css and quotes.unoptimized.css to quotes.css.unoptimized.css

Also added a comma at the end of the array:

+++ b/modules/simpletest/tests/common.test
@@ -1037,7 +1037,8 @@ class CascadingStylesheetsUnitTest extends DrupalUnitTestCase {
       'css_input_with_import.css',
       'css_subfolder/css_input_with_import.css',
-      'comment_hacks.css'
+      'comment_hacks.css',
+      'quotes.css'
     );

@sjerdo thanks for the clean up.

Patch at #8 works for me, thanks!

Bumping to 7.61. This didn't make it into 7.60

For anyone who comes upon this thread, has this problem with a CSS content string, and does not want to patch core, there is a workaround: replace any space that is being erroneously stripped with "\000020".

Reroll.

RTBC is good

1) Has test coverage (check)
2) Fix and test is already in D8 (check)
3) Passes new tests (check)

#3047844: [Regression] Tests fail on PHP 5.3

Re-rolling for latest line number changes in includes/common.inc with Drupal 7.71.

Re-rolling for latest line number changes in includes/common.inc with Drupal 7.79.

Just ran some retests. The patch in comment #15 still passes all tests but the re-rolls in #22 and #24 don't. Maybe, this means there's something wrong with the re-rolls?

This should be RTBC and has lost that status because of the newer re-rolls failing. The patch in #15 is a re-roll of the patch in #8 which was RTBC. Will try to reset the status to RTBC.

I just created an issue fork and merge request with the patch in #15. The re-roll patches don't include the three new files. That may be why they are failing.

Added this issue to #3207851: [meta] Priorities for 2021-06-02 release of Drupal 7.

I'm tempted to hide the recent re-rolls but will leave them for now so that the author of them can see and deal with it. Thanks.

How embarrassing. Newly created files back from #15 are included in attached patch.

Re-rolling for latest line number changes in includes/common.inc with Drupal 7.80.

This is the same modification that was committed to Drupal 8 and Drupal 9 more than four years ago - it is clearly working without problems in those newer versions of core, and should be committed to Drupal 7 as well. It's just wasting a lot of developer time when we encounter the problem and figure out the solution, only to find out that this issue has been know for more than 10 years #936316: CSS aggregation strips some essential whitespace within strings and that it's been fixed for more than 4 years in that same issue.

The backport to D7 is just a re-roll of the patch committed to D8 and D9. It has tests, and the tests pass. This patch has been RTBC here for more than three years (#10). Please commit it to D7!

This has been added to #3207851: [meta] Priorities for 2021-06-02 release of Drupal 7 for more visibility. @mcdruid is working on things. You can always weigh in there if you want.

Hi izmeez, thank you for adding this. I see you did that more than a month ago, but there is no indication here that this is part of the meta issue. Shouldn't the meta issue have this issue and all the other issues linked as related issues?

The meta issue is the third rendition after the releases in December and was it April?

The issue is there, along with many others,

#2877131: [D7] CSS aggregation strips some essential whitespace within strings Patch in comment #15 still applies, re-roll of #8 which was RTBC, has test coverage, passes tests, fixed in D8.

The key seems to be to keep each line short, but if you have a few pertinent words to add go ahead and edit the meta issue summary.

The maintainers have said they were finding other methods, linking issues, tagging them as "Target for release xyz" etc. was not working for them and wanted to try having a meta issue where the issues are actually listed and yes each of them is a link to the issue.

Granted there are a lot and there has been some amazing efforts being made by individuals to work on them. Like for instance the compatibility with php 8 in the last maintenance release along with other fixes. Also, it provides a list for anyone else who wants to create their own make file or composer file to patch core while they wait.

Sorry, I don't have a better answer. But, after my first little while of learning drupal and reading you should never patch core or use dev modules, i learned it doesn't work that way. I can't imagine surviving with drupal and not patching core and other modules or using dev modules. There just isn't time to wait for things to grind through. And fortunately, there is a vast resource of community efforts and fixes one can draw on like the patch in this thread.

Also, @liam-morland has created a merge request.

And thanks for adding your concerns and keeping interest alive in moving the issue forward.

Most of the meta issues I have worked on add each individual sub-issue as a "Related issue", in addition to listing each issue in the issue summary. This is what I always do in the projects that I maintain. If that is done in the meta issue, then this issue would show a block in the right sidebar titled "Referenced by" with a link to the meta issue. - that way anyone visiting this issue would see that it is part of the meta issue. Without that "Referenced by" block, we have no way of knowing that this issue is part of the meta issue.

I know that's not something that was created by you. I was just explaining why I posted here rather than in the meta issue - I didn't know about that meta issue and I didn't realize you had already added this to the meta issue!

I can't imagine surviving with drupal and not patching core and other modules or using dev modules.

I agree! I just wish it didn't take 4 years or more to get simple fixes into core!

I appreciate everyone's efforts to cajole this along.

At the moment I'm not finding MRs particularly helpful on old issues with lots of patches as it's not clear (to me at least) if there's anything new in the MR, and we can't use MRs to actually merge changes into core (see #3177220: Remove commit author radios in the "Credit & committing" section. / slack link).

The patch from the MR is currently the same as #31.

https://www.drupal.org/commitlog/commit/2/dc583b732afe4a4f5de2e18b7d4eba... is the D8/9 commit for comparison.

Hiding old patches and running all the tests on #31.

We'll try to get this one in for the release next week.

RTBC + 1

Thank you everybody!