-
Notifications
You must be signed in to change notification settings - Fork 148
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.
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
[BUGFIX] Parse @font-face src property as comma-delimited list #790
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,13 +117,21 @@ public static function parse(ParserState $oParserState): Rule | |
* @param string $sRule | ||
* | ||
* @return array<int, string> | ||
* The first item is the innermost separator (or, put another way, the highest-precedence operator). | ||
* The sequence continues to the outermost separator (or lowest-precedence operator). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer this text to be part of the general description of this method instead of the |
||
*/ | ||
private static function listDelimiterForRule($sRule): array | ||
{ | ||
if (\preg_match('/^font($|-)/', $sRule)) { | ||
return [',', '/', ' ']; | ||
} | ||
return [',', ' ', '/']; | ||
|
||
switch ($sRule) { | ||
case 'src': | ||
return [' ', ',']; | ||
default: | ||
return [',', ' ', '/']; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can integrate the preg matching into the |
||
} | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,58 @@ | ||||||
<?php | ||||||
|
||||||
declare(strict_types=1); | ||||||
|
||||||
namespace Sabberworm\CSS\Tests\Value; | ||||||
|
||||||
use PHPUnit\Framework\TestCase; | ||||||
use Sabberworm\CSS\Parsing\ParserState; | ||||||
use Sabberworm\CSS\Settings; | ||||||
use Sabberworm\CSS\Rule\Rule; | ||||||
use Sabberworm\CSS\Value\RuleValueList; | ||||||
use Sabberworm\CSS\Value\ValueList; | ||||||
|
||||||
/** | ||||||
* @covers \Sabberworm\CSS\Rule\Rule | ||||||
*/ | ||||||
final class RuleTest extends TestCase | ||||||
{ | ||||||
/** | ||||||
* @return array<string, array{0: string, 1: array<int, string>}> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
See: |
||||||
*/ | ||||||
public static function provideRulesAndExpectedParsedValueListTypes(): array | ||||||
{ | ||||||
return [ | ||||||
'src (e.g. in @font-face)' => [ | ||||||
" | ||||||
src: url('../fonts/open-sans-italic-300.woff2') format('woff2'), | ||||||
url('../fonts/open-sans-italic-300.ttf') format('truetype'); | ||||||
", | ||||||
[RuleValueList::class, RuleValueList::class], | ||||||
], | ||||||
]; | ||||||
} | ||||||
|
||||||
/** | ||||||
* @test | ||||||
* | ||||||
* @param array<int, string> $expectedTypeClassnames | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
* | ||||||
* @dataProvider provideRulesAndExpectedParsedValueListTypes | ||||||
*/ | ||||||
public function parsesValuesIntoExpectedTypeList(string $rule, array $expectedTypeClassnames): void | ||||||
{ | ||||||
$subject = Rule::parse(new ParserState($rule, Settings::create())); | ||||||
|
||||||
$value = $subject->getValue(); | ||||||
self::assertInstanceOf(ValueList::class, $value); | ||||||
|
||||||
$actualClassnames = \array_map( | ||||||
function ($component): string { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can make the function static and add a type declaration for |
||||||
return \get_class($component); | ||||||
}, | ||||||
$value->getListComponents() | ||||||
); | ||||||
|
||||||
self::assertSame($expectedTypeClassnames, $actualClassnames); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we maybe use |
||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make this more specific as we're touching the return value anyway?