[BUGFIX] Do not escape characters that do not need escaping in CSS string #1444

8ctopus · 2025-12-25T10:25:55Z

Add draft to attempt to resolve #1443

There are no specific tests yet, and there are improvements that can be made, I just want to get your approval before I polish things up.

coveralls · 2025-12-25T10:27:36Z

coverage: 70.775% (+0.3%) from 70.514%
when pulling 90d1602 on 8ctopus:css-string-escaping
into 2b61cd5 on MyIntervals:main.

JakeQZ

I think it could be significantly slower looping through the string rather than using the built-in function (particularly as some strings may be base64-encoded images). Also this is reimplementing some of what addslashes() does.

Wouldn't it be simpler (and more efficient) to use addslashes() as before, then do a str_replace() to replace \' with ' or \" with " depending on the string quoting type?

JakeQZ · 2025-12-30T01:04:52Z

Wouldn't it be simpler (and more efficient) to use addslashes() as before, then do a str_replace() to replace \' with ' or \" with " depending on the string quoting type?

PS. I agree with adding a separate method to do this.

8ctopus · 2026-01-07T06:35:02Z

Updated according to your feedback.

oliverklee · 2026-01-07T08:41:47Z

@8ctopus Thanks! Could you please rebase?

8ctopus · 2026-01-07T08:53:19Z

done

oliverklee · 2026-01-07T09:11:29Z

Thanks! GitHub is still showing merge conflicts. Could you please have a look?

…tring

8ctopus · 2026-01-07T10:28:26Z

now should be good.

oliverklee · 2026-01-07T11:09:01Z

Thanks! Now GitHub was able to run the CI jobs.

The PHP-CS-Fixer job suggests some improvements. Could you please apply those to get the build green? Thanks!

8ctopus · 2026-01-08T08:04:11Z

This time all tests are green, I also added a test to check that we do not escape when not necessary.

oliverklee · 2026-01-08T09:29:25Z

Thanks!

JakeQZ

This looks generally good now.

However, looking at the tests, there don't seem to be any covering that double-quotes within a single-quoted string are no longer escaped, e.g. if string quoting type is ', the quotes in "Hello World" don't need escaping and the result should be '"Hello World"'. Could you add a test for that?

I've suggested a couple of improvements, and a further correction to a comment that was previously wrong.

I'm not sure whether we have a project preference for string concatenation vs interpolation, so am asking @oliverklee for comment...

tests/fixtures/unicode.css

JakeQZ · 2026-01-08T23:40:44Z

tests/Unit/Value/CSSStringTest.php

+        $input = "data:image/svg+xml,%3Csvg width='24' height='24' viewBox='0 0 24 24' fill='none'" .
+        " xmlns='http://www.w3.org/2000/svg'%3E%3Cpath fill-rule='evenodd' clip-rule='evenodd'" .
+        " d='M14.3145 2 3V11.9987H17.5687L18 8.20761H14.3145L14.32 6.31012C14.32 5.32134 14.4207" .
+        ' 4.79153 15.9426 4.79153H17.977V1H14.7223C10.8129 1 9.43687 2.83909 9.43687 ' .
+        " 5.93187V8.20804H7V11.9991H9.43687V23H14.3145Z' fill='black'/%3E%3C/svg%3E%0A";


Autoformatting may apply some indentation, but I suggest leaving it as is for now. @oliverklee can run the autoformatter he uses as a post-PR (I think it's part of PHPStorm, which I don't have, and even if you have it, you may not have the same configuration set-up).

JakeQZ · 2026-01-09T00:02:47Z

tests/Unit/Value/CSSStringTest.php

+
+        $outputFormat = OutputFormat::createPretty();
+
+        self::assertSame("\"{$input}\"", (new CSSString($input))->render($outputFormat));


I think we are generally favouring using string contactenation rather than string interpolation, but I'm not sure of the rationale. All I can gather from some threads on StackOverflow is that it is a matter of personal preference. @oliverklee, are you able to expand on this?

If you regex the project with /\{\$.*\}/ you will see that interpolation is used 19 times, 4 times being my newly added code.

Yes, for new code, please use string concatenation instead of interpolation.

Yes, for new code, please use string concatenation instead of interpolation.

I've opened #1464, because I'm not sure that's best.

Apologies for the conflicting reviewer requests.

I'd be happy to merge with string interpolation for now. We can revisit this later pending the outcome of that discussion.

@oliverklee, would you be happy to merge this with the strings as are, pending further revisitation?

Yes, I'd be fine to keep the string interpolation when we merge this PR. I'd still like to get rid of the addslash-first-and-then-partially-undo part, though.

JakeQZ · 2026-01-09T00:03:11Z

tests/Unit/Value/CSSStringTest.php

+
+        $expected = str_replace("'", "\\'", $input);
+
+        self::assertSame("'{$expected}'", (new CSSString($input))->render($outputFormat));


src/Value/CSSString.php

tests/Unit/Value/CSSStringTest.php

8ctopus · 2026-01-09T05:31:01Z

However, looking at the tests, there don't seem to be any covering that double-quotes within a single-quoted string are no longer escaped, e.g. if string quoting type is ', the quotes in "Hello World" don't need escaping and the result should be '"Hello World"'. Could you add a test for that?

done

oliverklee · 2026-01-09T09:09:15Z

I'm not sure whether we have a project preference for string concatenation vs interpolation,

For new/updated code, we prefer string concatenation over interpolation (without having a hard-and-fast rule, though).

oliverklee · 2026-01-09T09:20:57Z

src/Value/CSSString.php

+
+    private function escape(string $string, string $quote): string
+    {
+        $string = \addslashes($string);


Instead of first escaping unnecessarily and then undoing part of the work, I'd prefer to only escape what we need to escape in the first place, i.e., directly use str_replace and avoid addslashes.

I suggested the current approach in my original review - see #1444 (review)

Hm … couldn't we use just the str_replace and skip the addslashes (without using a loop)?

… or use addcslashes and provide the characters to escape?

We could probably use addcslashes to escape the backslash and quote.

The only other character that addslashes escapes is the null character (U+0000), which I don't think we need to worry about (there are no tests for this, it is highly unlikely to be used in the real world, and it was probably just an inconsequential side-effect of addslashes that that would be the behaviour - other control characters aren't handled, apart from newline which is currently str_replaced separately by the calling method).

Perhaps we could move the str_replace for newline to the new method as part of this change.

oliverklee · 2026-01-09T09:21:25Z

tests/Unit/Value/CSSStringTest.php

+
+        $outputFormat = OutputFormat::createPretty();
+
+        self::assertSame("\"{$input}\"", (new CSSString($input))->render($outputFormat));


Yes, for new code, please use string concatenation instead of interpolation.

JakeQZ

looking at the tests, there don't seem to be any covering that double-quotes within a single-quoted string are no longer escaped

done

Thanks for your perseverence <3

I've made a suggestion for what the additional test should cover (to be consistent with the first test for single quotes), and for the test method naming. (The comments appear in order of writing in the Conversation tab, and should be read in that order, which is not in the order of the code. Hope that makes sense and they make sense.)

The code change suggested includes avoiding string interpolation in the one instance covered, though that would still need to resolved in the other three instances as per @oliverklee's comments.

JakeQZ · 2026-01-09T22:31:47Z

tests/Unit/Value/CSSStringTest.php

+        $outputFormat = OutputFormat::createPretty();
+
+        $input = "'Hello World'";
+
+        self::assertSame("\"{$input}\"", (new CSSString($input))->render($outputFormat));
+
+        $input = '"Hello World"';


The first test checks that single quote (') is escaped when it needs to be but not when it doesn't.

I'd suggest the second test does likewise for double quote ("), i.e. focuses only on the double quote and includes confirmation that it is still escaped when it needs to be:

Suggested change

$outputFormat = OutputFormat::createPretty();

$input = "'Hello World'";

self::assertSame("\"{$input}\"", (new CSSString($input))->render($outputFormat));

$input = '"Hello World"';

$input = '"Hello World"';

$outputFormat = OutputFormat::createPretty();

self::assertSame('"\\"Hello World\\""', (new CSSString($input))->render($outputFormat));

JakeQZ · 2026-01-09T22:34:03Z

tests/Unit/Value/CSSStringTest.php

+    /**
+     * @test
+     */
+    public function doesNotEscapeCharactersThatDoNotNeedToBeEscaped2(): void


Now there are two test methods, rather than appending the number 2 to the name of the second, they could be renamed to indicate their character speciality:

Suggested change

public function doesNotEscapeCharactersThatDoNotNeedToBeEscaped2(): void

public function doesNotEscapeDoubleQuotesThatDoNotNeedToBeEscaped(): void

JakeQZ · 2026-01-09T22:34:21Z

tests/Unit/Value/CSSStringTest.php

+    /**
+     * @test
+     */
+    public function doesNotEscapeCharactersThatDoNotNeedToBeEscaped(): void


...and

Suggested change

public function doesNotEscapeCharactersThatDoNotNeedToBeEscaped(): void

public function doesNotEscapeSingleQuotesThatDoNotNeedToBeEscaped(): void

oliverklee added the bug label Dec 25, 2025

oliverklee assigned 8ctopus Dec 25, 2025

oliverklee added this to the 9.2.0: New features, bug fixes and deprecations milestone Dec 25, 2025

JakeQZ reviewed Dec 29, 2025

View reviewed changes

8ctopus force-pushed the css-string-escaping branch from 2a66812 to 47970ab Compare January 7, 2026 08:52

[FEATURE] Do not escape characters that do not need escaping in CSS s…

676dd8c

…tring

8ctopus force-pushed the css-string-escaping branch from 47970ab to 676dd8c Compare January 7, 2026 10:27

8ctopus added 4 commits January 8, 2026 11:43

[TASK] Cleanup code formatting

07504ba

[TASK] Add do not escape characters that do not need escaping test

84e938d

[TASK] Cleanup code formatting

2c201fc

[TASK] Cleanup code formatting

234f5da

JakeQZ requested changes Jan 9, 2026

View reviewed changes

JakeQZ requested a review from oliverklee January 9, 2026 00:36

JakeQZ reviewed Jan 9, 2026

View reviewed changes

tests/Unit/Value/CSSStringTest.php Outdated Show resolved Hide resolved

8ctopus added 2 commits January 9, 2026 09:25

[TASK] Clean up code

3855f91

[TASK] Add one more test to test css string escaping

90d1602

oliverklee changed the title ~~Do not escape characters that do not need escaping in CSS string~~ [BUGFIX] Do not escape characters that do not need escaping in CSS string Jan 9, 2026

oliverklee requested changes Jan 9, 2026

View reviewed changes

JakeQZ requested changes Jan 9, 2026

View reviewed changes


		$outputFormat = OutputFormat::createPretty();

		self::assertSame("\"{$input}\"", (new CSSString($input))->render($outputFormat));


		$expected = str_replace("'", "\\'", $input);

		self::assertSame("'{$expected}'", (new CSSString($input))->render($outputFormat));

	public function doesNotEscapeCharactersThatDoNotNeedToBeEscaped2(): void
	public function doesNotEscapeDoubleQuotesThatDoNotNeedToBeEscaped(): void

	public function doesNotEscapeCharactersThatDoNotNeedToBeEscaped(): void
	public function doesNotEscapeSingleQuotesThatDoNotNeedToBeEscaped(): void

[BUGFIX] Do not escape characters that do not need escaping in CSS string #1444

Are you sure you want to change the base?

[BUGFIX] Do not escape characters that do not need escaping in CSS string #1444

Uh oh!

Conversation

8ctopus commented Dec 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Dec 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JakeQZ left a comment

Choose a reason for hiding this comment

Uh oh!

JakeQZ commented Dec 30, 2025

Uh oh!

8ctopus commented Jan 7, 2026

Uh oh!

oliverklee commented Jan 7, 2026

Uh oh!

8ctopus commented Jan 7, 2026

Uh oh!

oliverklee commented Jan 7, 2026

Uh oh!

8ctopus commented Jan 7, 2026

Uh oh!

oliverklee commented Jan 7, 2026

Uh oh!

8ctopus commented Jan 8, 2026

Uh oh!

oliverklee commented Jan 8, 2026

Uh oh!

JakeQZ left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

8ctopus commented Jan 9, 2026

Uh oh!

oliverklee commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakeQZ left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

8ctopus commented Dec 25, 2025 •

edited

Loading

coveralls commented Dec 25, 2025 •

edited

Loading

JakeQZ left a comment •

edited

Loading

oliverklee commented Jan 9, 2026 •

edited

Loading

JakeQZ left a comment •

edited

Loading