Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
8ctopus wants to merge 7 commits into
base: main
Choose a base branch
from
+60 −4
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
676dd8c
[FEATURE] Do not escape characters that do not need escaping in CSS s…
8ctopus Dec 25, 2025
07504ba
[TASK] Cleanup code formatting
8ctopus Jan 8, 2026
84e938d
[TASK] Add do not escape characters that do not need escaping test
8ctopus Jan 8, 2026
2c201fc
[TASK] Cleanup code formatting
8ctopus Jan 8, 2026
234f5da
[TASK] Cleanup code formatting
8ctopus Jan 8, 2026
3855f91
[TASK] Clean up code
8ctopus Jan 9, 2026
90d1602
[TASK] Add one more test to test css string escaping
8ctopus Jan 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ Please also have a look at our

### Documentation

## 9.2.0:

### Fixed

- Do not escape characters that do not need escaping in CSS string (#1444)

## 9.1.0: Add support for PHP 8.5

### Added
Expand Down
11 changes: 10 additions & 1 deletion src/Value/CSSString.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public function getString(): string
*/
public function render(OutputFormat $outputFormat): string
{
$string = \addslashes($this->string);
$string = $this->escape($this->string, $outputFormat->getStringQuotingType());
$string = \str_replace("\n", '\\A', $string);
return $outputFormat->getStringQuotingType() . $string . $outputFormat->getStringQuotingType();
}
Expand All @@ -111,4 +111,13 @@ public function getArrayRepresentation(): array
'contents' => $this->string,
];
}

private function escape(string $string, string $quote): string
{
$string = \addslashes($string);
Copy link
Collaborator

@oliverklee oliverklee Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@JakeQZ JakeQZ Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@oliverklee oliverklee Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@oliverklee oliverklee Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… or use addcslashes and provide the characters to escape?

Copy link
Collaborator

@JakeQZ JakeQZ Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


$replace = $quote === '"' ? ["\\'" => "'"] : ['\\"' => '"'];

return \str_replace(array_keys($replace), array_values($replace), $string);
}
}
4 changes: 2 additions & 2 deletions tests/ParserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,10 @@ public function unicodeParsing(): void
self::assertSame('"\\"\\""', $firstContentRuleAsString);
}
if ($selector === '.test-9') {
self::assertSame('"\\"\\\'"', $firstContentRuleAsString);
self::assertSame('"\\"\'"', $firstContentRuleAsString);
}
if ($selector === '.test-10') {
self::assertSame('"\\\'\\\\"', $firstContentRuleAsString);
self::assertSame('"\'\\\\"', $firstContentRuleAsString);
}
if ($selector === '.test-11') {
self::assertSame('"test"', $firstContentRuleAsString);
Expand Down
41 changes: 41 additions & 0 deletions tests/Unit/Value/CSSStringTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
namespace Sabberworm\CSS\Tests\Unit\Value;

use PHPUnit\Framework\TestCase;
use Sabberworm\CSS\OutputFormat;
use Sabberworm\CSS\Value\CSSString;
use Sabberworm\CSS\Value\PrimitiveValue;
use Sabberworm\CSS\Value\Value;
Expand Down Expand Up @@ -107,4 +108,44 @@ public function getArrayRepresentationIncludesContents(): void
self::assertArrayHasKey('contents', $result);
self::assertSame($contents, $result['contents']);
}

/**
* @test
*/
public function doesNotEscapeCharactersThatDoNotNeedToBeEscaped(): void
Copy link
Collaborator

@JakeQZ JakeQZ Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and

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

{
$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";
Comment on lines +117 to +121
Copy link
Collaborator

@JakeQZ JakeQZ Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


$outputFormat = OutputFormat::createPretty();

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

@JakeQZ JakeQZ Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@8ctopus 8ctopus Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@oliverklee oliverklee Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@JakeQZ JakeQZ Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@oliverklee oliverklee Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


$outputFormat->setStringQuotingType("'");

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

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

@JakeQZ JakeQZ Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

}

/**
* @test
*/
public function doesNotEscapeCharactersThatDoNotNeedToBeEscaped2(): void
Copy link
Collaborator

@JakeQZ JakeQZ Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
$outputFormat = OutputFormat::createPretty();

$input = "'Hello World'";

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

$input = '"Hello World"';
Comment on lines +139 to +145
Copy link
Collaborator

@JakeQZ JakeQZ Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));


$outputFormat->setStringQuotingType("'");

self::assertSame("'{$input}'", (new CSSString($input))->render($outputFormat));
}
}
2 changes: 1 addition & 1 deletion tests/fixtures/unicode.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
.test-6 { content: "\00A5" } /* Same as "¥" */
.test-7 { content: '\a' } /* Same as "\A" (Newline) */
.test-8 { content: "\"\22" } /* Same as "\"\"" */
.test-9 { content: "\"\27" } /* Same as ""\"\'"" */
.test-9 { content: "\"\27" } /* Same as "\"'" */
.test-10 { content: "\'\\" } /* Same as "'\" */
.test-11 { content: "\test" } /* Same as "test" */

Expand Down