-
Notifications
You must be signed in to change notification settings - Fork 65
Unescape constant strings #143
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
Conversation
I would expect double quoted string to support the same escaping sequences as PHP.
See https://github.com/nikic/PHP-Parser/blob/master/lib/PhpParser/Node/Scalar/String_.php#L71-L130
I initially was going to do so, but didn't for parity with Psalm. I'd be more than happy to mirror PHP's functionality though, if @ondrejmirtes also agrees :)
Yes, parity with PHP would be great, we're in a very edge case territory anyway. And inform the Psalm maintainers afterwards 😊 BTW is this a BC break? Do we need a feature flag for the new behaviour?
src/Parser/ConstExprParser.php
Outdated
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.
decode or parse string would be better name
5a3df11 to
9e0ab4e
Compare
I've basically copied most of the implementation from PHP-Parser, but that probably needs to be mentioned somewhere. Any suggestions?
A github.com permalink in a code comment would be fine I think.
This is gonna need some modification and a bleedingEdge toggle in phpstan-src too, right?
It should only require a feature toggle :)
What about the bool $trimStrings = false? parameter? Shouldn't we eventually get rid of it? In PHPStan it's probably called with false...
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.
Would it be possible to use mb_chr() or IntlChar::chr() instead?
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.
Neither mbstring nor intl are declared as a dependency, so that would need to be conditional. That would only make the code more complicated for little gain.
What about the
bool $trimStrings = false? parameter? Shouldn't we eventually get rid of it? In PHPStan it's probably called with false...
I would definitely like to get rid of it, especially because it is used with both true/false internally. @method parameter default values are parsed without unescaping, which eventually leads to unescaped values in PHPStan as well. Those never really propagate anywhere though...
There isn't a pure BC-friendly approach to removing it I'm afraid, but I really don't think it should be used at all so maybe it's not that bad?
9e0ab4e to
720fc79
Compare
Thank you!
Fix #142