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.

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

PhpdocToParamTypeFixer - fix for breaking PHP syntax for type having reserved name #5006

Merged
merged 1 commit into from Jul 10, 2020
Merged

PhpdocToParamTypeFixer - fix for breaking PHP syntax for type having reserved name #5006

merged 1 commit into from Jul 10, 2020

Conversation

kubawerlos
Copy link
Contributor

Twin PR to #4963

I wonder if it would be useful to have some class e.g. SyntaxChecker with (static?) method isValid, which will handle cache internally and all needed will be to call it like this:

if (!SyntaxChecker::isValid(sprintf('<?php function f(%s $x) {}', $returnType))) {
    continue;
}

Currently, this will have 2 uses (here and PhpdocToReturnTypeFixer, maybe also AbstractPsrAutoloadingFixer, not mandatory), but I imagine quite soon we will have PhpdocToPropertyTypeFixer.

@julienfalque
Copy link
Member

[...] but I imagine quite soon we will have PhpdocToPropertyTypeFixer.

Indeed, #4592 introduces a parent abstract class where we could move this logic but a dedicated class sounds better :)

@kubawerlos
Copy link
Contributor Author

@julienfalque the idea with SyntaxValidator failed as to validate the code in PHP 5.6 Tokens class fails and only working solution, which is using eval is... you know evil ;)

Let's try with placing the check in AbstractPhpdocToTypeDeclarationFixer.

@SpacePossum
Copy link
Contributor

I liked the validator more (composition over inheritance), can you explain the issue on 5.6 a bit more?

@kubawerlos
Copy link
Contributor Author

Tokens::fromCode runs token_get_all under the hood, with TOKEN_PARSE flag if it exists, but it does not in PHP 5.6. Without the flag everything is valid -> https://3v4l.org/CfW8M

And that call was actually the validation done in SyntaxValidator class.

Also as composition over inheritance is usually what I want to do in this case I'm not so sure - adding class SyntaxValidator would be like stating: "Look, here is the class to use in fixers" and I don't want to do that - I cannot see any other case when we would like to do this.

@SpacePossum SpacePossum added the RTM Ready To Merge label Jul 6, 2020
@SpacePossum SpacePossum added this to the 2.16.5 milestone Jul 10, 2020
@SpacePossum SpacePossum removed the RTM Ready To Merge label Jul 10, 2020
@SpacePossum
Copy link
Contributor

Thank you @kubawerlos.

@SpacePossum SpacePossum merged commit 7405b4d into PHP-CS-Fixer:2.16 Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants