phpstan / phpdoc-parser

Next-gen phpDoc parser with support for intersection types and generics
MIT License
1.32k stars 62 forks source link

Key should be quoted when it contains spaces in ArrayShapeItemNode #251

Closed ruudk closed 1 week ago

ruudk commented 1 week ago

I noticed something odd.

$input = "array{states: array{'Hello World': int}}";
$expected = "array{states: array{'Hello World': int}}";
$lexer = new Lexer();
$constExprParser = new ConstExprParser(true, true);
$typeParser = new TypeParser($constExprParser);
$phpDocParser = new PhpDocParser($typeParser, $constExprParser);

$tokens = new TokenIterator($lexer->tokenize($input));
$phpDocNode = $phpDocParser->parseTagValue($tokens, '@var');

$this->assertInstanceOf(VarTagValueNode::class, $phpDocNode);
$this->assertInstanceOf(ArrayShapeNode::class, $phpDocNode->type);

$printer = new Printer();
$this->assertSame($expected, $printer->print($phpDocNode));

Failed asserting that two strings are identical. Expected :'array{states: array{'Hello World': int}}' Actual :'array{states: array{Hello World: int}}'

As you can see, the key Hello World is not quoted, while it should.

When I try to feed the wrong PHPDoc into parseTagValue it returns InvalidTagValueNode.

Am I doing something wrong, or is this a bug that I can try to fix 😊 ?

ruudk commented 1 week ago

I think the same problem can happen for object key names. Those keys can also contain spaces.

Created a PR to fix it: https://github.com/phpstan/phpdoc-parser/pull/252

ruudk commented 1 week ago

I'm sorry for this, but the problem was that my TypeParser did not have $quoteAwareConstExprString enabled. While my ConstExprParser did. I didn't notice I had to configure it in 2 locations.

ondrejmirtes commented 1 week ago

Yeah, I'm aware of this potential issue and that's why for 2.0 I'm changing the constructor to accept a shared ParserConfig object: https://github.com/phpstan/phpdoc-parser/commit/9d57f3db5bba9b0d8d11726389eb8af3126780b4

I know that this "service locator" is an anti-pattern, but you gotta know the rules so that you can know when to break them 😂

Also: All of the current "options" in 1.x are there for the same reason bleeding edge exists: They represent new behaviour which will become the default (and only) behaviour in 2.0. That's why the 2.0 removes all of these options now.

I'm adding ParserConfig class where the new options during 2.x series development will be added and shared across services 👍