Skip to content

bug: Allow string quote to be escaped within phpdoc constant#6798

Merged
kubawerlos merged 1 commit intoPHP-CS-Fixer:masterfrom
mvorisek:improve_phpdoc_parsing
Apr 27, 2023
Merged

bug: Allow string quote to be escaped within phpdoc constant#6798
kubawerlos merged 1 commit intoPHP-CS-Fixer:masterfrom
mvorisek:improve_phpdoc_parsing

Conversation

@mvorisek
Copy link
Copy Markdown
Contributor

@mvorisek mvorisek commented Feb 25, 2023

Reference: phpstan/phpdoc-parser#142 (1, 2).

@mvorisek mvorisek changed the title bug: fix edge phpdoc parsing cases bug: Allow string quote to be escaped within phpdoc constant Feb 25, 2023
@mvorisek mvorisek marked this pull request as ready for review February 25, 2023 11:08
@mvorisek mvorisek force-pushed the improve_phpdoc_parsing branch 3 times, most recently from 26172bf to 8c2fdd3 Compare February 25, 2023 16:52
@@ -88,7 +88,7 @@ final class TypeExpression
(?i)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would love to hear more about the purpose of this PR.
looking only at \\\'"\'"'"|"|"'"\'\\"""""\"" is not helping ;)

Copy link
Copy Markdown
Contributor Author

@mvorisek mvorisek Feb 26, 2023

Choose a reason for hiding this comment

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

this PR allows type like 'a\'b' to be accepted (quote char to be escaped/present)

  1. escaping the quote character was previously not possible
  2. \n could be possibly reprinted wrongly, thus tested ;-)
  3. the test has 2 types, if parsed, they are parsed as two types - this is what the test does

@keradus
Copy link
Copy Markdown
Member

keradus commented Mar 12, 2023

@Wirone
, feeling up for a challenge to review this beauty?


yield ['array < int , callable ( string ) : bool >', ['array < int , callable ( string ) : bool >']];

yield ['\'a\\\'s"\\\\\n\r\t\'|"b\\"s\'\\\\\n\r\t"', ['\'a\\\'s"\\\\\n\r\t\'', '"b\\"s\'\\\\\n\r\t"']];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we have a few more? What is the smallest accepted type? '\''?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added smallest type - '' empty string and a few other simple strin before the complex test

@kubawerlos
Copy link
Copy Markdown
Member

Why do we need it? If I saw ' in type in code review I would comment "hey, the type here is broken". Can you add some real-life type test case?

@Wirone
Copy link
Copy Markdown
Member

Wirone commented Mar 21, 2023

@Wirone, feeling up for a challenge to review this beauty?

@keradus I agree with @kubawerlos - I don't see the need for this change. Lack of PR description and more examples does not help 😉

@mvorisek
Copy link
Copy Markdown
Contributor Author

@Wirone please see phpstan/phpdoc-parser#143, escaped phpdoc string is valid and must be supported, it is obvious the original \'[^\']+?\' regex cannot handle escaped quotes in quotes.

@Wirone
Copy link
Copy Markdown
Member

Wirone commented Mar 22, 2023

Looking at examples provided phpstan/phpdoc-parser#142 (1, 2) gave me a context needed to understand why it's required, thanks @mvorisek 🙂. It would be easier for reviewers if it was provided in the description, though 😅.

@mvorisek mvorisek requested a review from kubawerlos April 9, 2023 09:01
@mvorisek mvorisek force-pushed the improve_phpdoc_parsing branch 2 times, most recently from 3702e6b to 6ef3e5f Compare April 9, 2023 09:27
@Wirone
Copy link
Copy Markdown
Member

Wirone commented Apr 20, 2023

@mvorisek some PRs were merged, could you please resolve the conflicts and update the branch for this PR?

@mvorisek mvorisek force-pushed the improve_phpdoc_parsing branch from 6ef3e5f to add37a1 Compare April 21, 2023 06:31
@mvorisek
Copy link
Copy Markdown
Contributor Author

@Wirone conflict resolved

@mvorisek
Copy link
Copy Markdown
Contributor Author

@Wirone can this PR be merged?

@kubawerlos kubawerlos merged commit efa1c44 into PHP-CS-Fixer:master Apr 27, 2023
@Wirone
Copy link
Copy Markdown
Member

Wirone commented Apr 27, 2023

Thank you @mvorisek 🍻

@mvorisek mvorisek deleted the improve_phpdoc_parsing branch April 27, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants