Skip to content
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

allow empty strings and numbers as metadata type parameters #1019

Merged
merged 2 commits into from
Dec 12, 2018
Merged

allow empty strings and numbers as metadata type parameters #1019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93eecd5
allow empty strings and numbers as parameters
goetas Dec 9, 2018
319d3b2
allow also null as parameters
goetas Dec 9, 2018
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
47 changes: 28 additions & 19 deletions src/Type/InnerParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,47 +25,56 @@ public function __construct()
'skip' => '\s+',
'parenthesis_' => '<',
'_parenthesis' => '>',
'empty_string' => '""|\'\'',
'number' => '(\+|\-)?(0|[1-9]\d*)(\.\d+)?',
'null' => 'null',
'comma' => ',',
'name' => '(?:[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*\\\)*[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*',
'quote_:quoted_string' => '"',
'apostrophe_:apostrophed_string' => '\'',
],
'quoted_string' => [
'quoted_string' => '(?:[^"]|"")+',
'quoted_string' => '[^"]+',
'_quote:default' => '"',
],
'apostrophed_string' => [
'apostrophed_string' => '(?:[^\']|\'\')+',
'apostrophed_string' => '[^\']+',
'_apostrophe:default' => '\'',
],
],
[
'type' => new Choice('type', ['simple_type', 'compound_type'], null),
1 => new Token(1, 'name', null, -1, true),
2 => new Concatenation(2, [1], '#simple_type'),
3 => new Token(3, 'quote_', null, -1, false),
4 => new Token(4, 'quoted_string', null, -1, true),
5 => new Token(5, '_quote', null, -1, false),
6 => new Concatenation(6, [3, 4, 5], '#simple_type'),
7 => new Token(7, 'apostrophe_', null, -1, false),
8 => new Token(8, 'apostrophed_string', null, -1, true),
9 => new Token(9, '_apostrophe', null, -1, false),
10 => new Concatenation(10, [7, 8, 9], '#simple_type'),
'simple_type' => new Choice('simple_type', [2, 6, 10], null),
12 => new Token(12, 'name', null, -1, true),
13 => new Token(13, 'parenthesis_', null, -1, false),
14 => new Token(14, 'comma', null, -1, false),
15 => new Concatenation(15, [14, 'type'], '#compound_type'),
16 => new Repetition(16, 0, -1, 15, null),
17 => new Token(17, '_parenthesis', null, -1, false),
'compound_type' => new Concatenation('compound_type', [12, 13, 'type', 16, 17], null),
3 => new Token(3, 'number', null, -1, true),
4 => new Concatenation(4, [3], '#simple_type'),
5 => new Token(5, 'null', null, -1, true),
6 => new Concatenation(6, [5], '#simple_type'),
7 => new Token(7, 'empty_string', null, -1, true),
8 => new Concatenation(8, [7], '#simple_type'),
9 => new Token(9, 'quote_', null, -1, false),
10 => new Token(10, 'quoted_string', null, -1, true),
11 => new Token(11, '_quote', null, -1, false),
12 => new Concatenation(12, [9, 10, 11], '#simple_type'),
13 => new Token(13, 'apostrophe_', null, -1, false),
14 => new Token(14, 'apostrophed_string', null, -1, true),
15 => new Token(15, '_apostrophe', null, -1, false),
16 => new Concatenation(16, [13, 14, 15], '#simple_type'),
'simple_type' => new Choice('simple_type', [2, 4, 6, 8, 12, 16], null),
18 => new Token(18, 'name', null, -1, true),
19 => new Token(19, 'parenthesis_', null, -1, false),
20 => new Token(20, 'comma', null, -1, false),
21 => new Concatenation(21, [20, 'type'], '#compound_type'),
22 => new Repetition(22, 0, -1, 21, null),
23 => new Token(23, '_parenthesis', null, -1, false),
'compound_type' => new Concatenation('compound_type', [18, 19, 'type', 22, 23], null),
],
[]
);

$this->getRule('type')->setPPRepresentation(' simple_type() | compound_type()');
$this->getRule('simple_type')->setDefaultId('#simple_type');
$this->getRule('simple_type')->setPPRepresentation(' <name> | ::quote_:: <quoted_string> ::_quote:: | ::apostrophe_:: <apostrophed_string> ::_apostrophe::');
$this->getRule('simple_type')->setPPRepresentation(' <name> | <number> | <null> | <empty_string> | ::quote_:: <quoted_string> ::_quote:: | ::apostrophe_:: <apostrophed_string> ::_apostrophe::');
$this->getRule('compound_type')->setDefaultId('#compound_type');
$this->getRule('compound_type')->setPPRepresentation(' <name> ::parenthesis_:: type() ( ::comma:: type() )* ::_parenthesis::');
}
Expand Down
12 changes: 12 additions & 0 deletions src/Type/TypeVisitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,18 @@ private function visitSimpleType(TreeNode $element)
return ['name' => $value, 'params' => []];
}

if ('empty_string' === $token) {
return '';
}

if ('null' === $token) {
return null;
}

if ('number' === $token) {
return false === strpos($value, '.') ? intval($value) : floatval($value);
}

$escapeChar = 'quoted_string' === $token ? '"' : "'";

if (false === strpos($value, $escapeChar)) {
Expand Down
10 changes: 8 additions & 2 deletions src/Type/grammar.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,28 @@

%token parenthesis_ <
%token _parenthesis >
%token empty_string ""|''
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks weird, I think this should be handled as part of strings themselves, meaning that they will no longer be a single token but a node instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was the only way I found to make it work with empty strings.
Better solutions are welcome

Copy link
Contributor

Choose a reason for hiding this comment

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

If you make the inner token optional in simple_type (<quoted_string>? and <apostrophed_string>?), it will work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tried right now,

1) JMS\Serializer\Tests\Serializer\Type\ParserTest::testParse with data set #7 ('Foo<'a',''>', array('Foo', array('a', '')))
Error: Call to a member function getValueToken() on null

/home/goetas/projects/serializer/src/Type/TypeVisitor.php:36
/home/goetas/projects/serializer/src/Type/TypeVisitor.php:22
/home/goetas/projects/serializer/vendor/hoa/compiler/Llk/TreeNode.php:333
/home/goetas/projects/serializer/src/Type/TypeVisitor.php:69
/home/goetas/projects/serializer/src/Type/TypeVisitor.php:71
/home/goetas/projects/serializer/src/Type/TypeVisitor.php:24
/home/goetas/projects/serializer/src/Type/Parser.php:30
/home/goetas/projects/serializer/tests/Serializer/Type/ParserTest.php:29

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, you made the token optional (nullable), so the visitor needs to be updated accordingly. :)

%token number (\+|\-)?(0|[1-9]\d*)(\.\d+)?
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't allow floats in the following form: .123
Should be written as something similar:

[+-]?(?:(?:0|[1-9]\d*(?:\.0*[1-9]\d*)?)|\.0*\d*[1-9])

https://regex101.com/r/Nadose/1

Also this doesn't handle exponential floats.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's true. My goal was to solve the reported bug. A more complete implementation is welcome

%token null null
%token comma ,
%token name (?:[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*\\)*[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*

%token quote_ " -> quoted_string
%token quoted_string:quoted_string (?:[^"]|"")+
%token quoted_string:quoted_string [^"]+
%token quoted_string:_quote " -> default

%token apostrophe_ ' -> apostrophed_string
%token apostrophed_string:apostrophed_string (?:[^']|'')+
%token apostrophed_string:apostrophed_string [^']+
%token apostrophed_string:_apostrophe ' -> default

type:
simple_type() | compound_type()

#simple_type:
<name>
| <number>
| <null>
| <empty_string>
| ::quote_:: <quoted_string> ::_quote::
| ::apostrophe_:: <apostrophed_string> ::_apostrophe::

Expand Down
32 changes: 24 additions & 8 deletions tests/Serializer/Type/ParserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,30 @@ public function validTypesProvider(): iterable
'array<Foo>',
$type('array', [['name' => 'Foo', 'params' => []]]),
];
yield [
'Foo<\'a\'>',
$type('Foo', ['a']),
];
yield [
'Foo<5>',
$type('Foo', [5]),
];
yield [
'Foo<5.5>',
$type('Foo', [5.5]),
];
yield [
'Foo<null>',
$type('Foo', [null]),
];
yield [
'Foo<\'a\',\'b\',\'c\'>',
$type('Foo', ['a', 'b', 'c']),
];
yield [
'Foo<\'a\',\'\'>',
$type('Foo', ['a', '']),
];
yield [
'array<Foo,Bar>',
$type('array', [['name' => 'Foo', 'params' => []], ['name' => 'Bar', 'params' => []]]),
Expand All @@ -72,14 +96,6 @@ public function validTypesProvider(): iterable
'Foo<"asdf asdf">',
$type('Foo', ['asdf asdf']),
];
yield [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed this as it will not allow '' (empty strings syntax) and was not available in the 1.x parser

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From what I can guess, what you were trying to do here was handled by YAML

'Foo<"""bar""">',
$type('Foo', ['"bar"']),
];
yield [
"Foo<'a''b'>",
$type('Foo', ["a'b"]),
];
}

public function testEmptyString(): void
Expand Down