Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Backport of D8 issue #936316: CSS aggregation strips some essential whitespace within strings
Comment | File | Size | Author |
---|---|---|---|
#31 | drupal-css-aggregation-strips-whitespace-2877131-31.patch | 126.67 KB | collinhaines |
Issue fork drupal-2877131
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:
- 2877131-d7-css-aggregation changes, plain diff MR !565
Comments
Comment #2
hass CreditAttribution: hass commentedComment #3
hass CreditAttribution: hass commentedFixed two tabs.
Comment #4
mikeytown2 CreditAttribution: mikeytown2 commentedAdded this test to AdvAgg and it passes.
+1 RTBC from me. AdvAgg has had the new regex in it for quite some time now.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis 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:
There should be spaces before and after the string-concatenation operators.
Comment #6
oadaeh CreditAttribution: oadaeh at Hook 42 commentedAttached 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).
Comment #8
sjerdoThe 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:
Comment #9
oadaeh CreditAttribution: oadaeh at Hook 42 commented@sjerdo thanks for the clean up.
Comment #10
anrikun CreditAttribution: anrikun commentedPatch at #8 works for me, thanks!
Comment #11
joseph.olstadBumping to 7.61. This didn't make it into 7.60
Comment #12
joseph.olstadComment #13
gbirch CreditAttribution: gbirch as a volunteer and at Tech-Tamer, LLC commentedFor 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".
Comment #14
joseph.olstadComment #15
Liam MorlandReroll.
Comment #16
joseph.olstadComment #17
joseph.olstadComment #18
joseph.olstadRTBC is good
1) Has test coverage (check)
2) Fix and test is already in D8 (check)
3) Passes new tests (check)
Comment #19
joseph.olstad#3047844: [Regression] Tests fail on PHP 5.3
Comment #20
MustangGB CreditAttribution: MustangGB commentedComment #21
MustangGB CreditAttribution: MustangGB commentedComment #22
collinhaines CreditAttribution: collinhaines commentedRe-rolling for latest line number changes in
includes/common.inc
with Drupal 7.71.Comment #24
collinhaines CreditAttribution: collinhaines commentedRe-rolling for latest line number changes in
includes/common.inc
with Drupal 7.79.Comment #25
izmeez CreditAttribution: izmeez commentedJust 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?
Comment #27
izmeez CreditAttribution: izmeez commentedThis 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.
Comment #28
Liam MorlandI 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.
Comment #29
izmeez CreditAttribution: izmeez commentedAdded 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.
Comment #30
collinhaines CreditAttribution: collinhaines commentedHow embarrassing. Newly created files back from #15 are included in attached patch.
Comment #31
collinhaines CreditAttribution: collinhaines commentedRe-rolling for latest line number changes in
includes/common.inc
with Drupal 7.80.Comment #32
TR CreditAttribution: TR commentedThis 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!
Comment #33
izmeez CreditAttribution: izmeez commentedThis 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.
Comment #34
TR CreditAttribution: TR commentedHi 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?
Comment #35
izmeez CreditAttribution: izmeez commentedThe meta issue is the third rendition after the releases in December and was it April?
The issue is there, along with many others,
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.
Comment #36
TR CreditAttribution: TR commentedMost 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 agree! I just wish it didn't take 4 years or more to get simple fixes into core!
Comment #37
mcdruidI 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.
Comment #38
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC + 1
Comment #45
mcdruidComment #47
mcdruidThank you everybody!