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

PhpdocToReturnTypeFixer - fix for breaking PHP syntax for type having reserved name #4963

Merged
merged 4 commits into from
Jun 14, 2020
Merged

PhpdocToReturnTypeFixer - fix for breaking PHP syntax for type having reserved name #4963

merged 4 commits into from
Jun 14, 2020

Conversation

kubawerlos
Copy link
Contributor

This test tries all tokens as return type which results with 73 failures.

Limiting test to PHP 7.4 is only because of laziness - finally, it should run for all PHP versions.

A question to @keradus, @SpacePossum and @julienfalque: do you think we

  • can have "internal" repository like php-cs-fixer/all-token to provide us with all tokens to test like this
  • should I make the currently used Packagist/release ready and we can use the stable version of it
  • should we put this somewhere in PHP CS Fixer repository?

@julienfalque
Copy link
Member

I would prefer not having to maintain that list over time. Maybe we could have a way to detect that automatically, e.g. https://3v4l.org/OCU5L:

function is_valid_type($type) {
    try {
        return true === eval("function ({$type} \$arg) {}; return true;");
    } catch (ParseError $exception) {
        return false;
    }
}

Hacky but way simpler.

@kubawerlos
Copy link
Contributor Author

Shouldn't be time-consuming to keep the list up to date - how often new tokens are added to PHP? Every new "major" release - and we would, of course, automate that.

Approach with eval (which is basically "change the code, but if it breaks the PHP syntax then don't") would be a precedence - we don't have a fixer yet to test the change by verifying syntax from string - maybe finally we go with it, but I'd like to try more cleaner approach - after all, it's open-source and we don't have to hurry.

P.S. pinging @Slamdunk for an opinion as the author of fixer.

@kubawerlos kubawerlos changed the title [RFC] PhpdocToReturnTypeFixer - breaking PHP syntax for type being reserved name PhpdocToReturnTypeFixer - breaking PHP syntax for type being reserved name May 23, 2020
@SpacePossum
Copy link
Contributor

I think the best place would be somewhere within the tests/ folder of the project for simplicity, however if you want to publish and maintain the package somewhere else than 👍

For maintaining the list; maybe we can have test that fails if you run the tests on a PHP major version higher than we know is complete for the list. Than on the failure we can update the list and update the test, something like that?

@Slamdunk
Copy link
Contributor

While I appreciate the clean idea of @kubawerlos , as I get older I have less and less spare time to give to FOSS projects, and I guess everyone will have the same issue in the long run. I'd like to invest my (and your) time in a activities with more values, like fixers for new syntax in the upcoming PHP versions.

So I'd go for @julienfalque solution: it seems to me easy to implement, future proof and with low performance impact since it can be easily cached if needed.

@kubawerlos kubawerlos marked this pull request as ready for review May 25, 2020 21:16
@SpacePossum
Copy link
Contributor

for me eval is a no-go in the code base, maybe we can generate the code and use the linter to verify the result?

@kubawerlos
Copy link
Contributor Author

Agree with evil eval.

I was trying to create a linter instance in the fixer, but it was a lot of copy-paste code as Linter needs executable in the constructor.

The current solution is already used and looks acceptable to me.

@kubawerlos kubawerlos changed the title PhpdocToReturnTypeFixer - breaking PHP syntax for type being reserved name PhpdocToReturnTypeFixer - fix for breaking PHP syntax for type having reserved name Jun 5, 2020
@SpacePossum SpacePossum added this to the 2.15.8 milestone Jun 8, 2020
@SpacePossum SpacePossum added the RTM Ready To Merge label Jun 10, 2020
@SpacePossum
Copy link
Contributor

Thank you @kubawerlos.

@SpacePossum SpacePossum merged commit 6ce78a0 into PHP-CS-Fixer:2.15 Jun 14, 2020
@SpacePossum SpacePossum removed the RTM Ready To Merge label Jun 14, 2020
@kubawerlos kubawerlos deleted the fix_PhpdocToReturnTypeFixer branch June 14, 2020 17:30
SpacePossum added a commit that referenced this pull request Jul 10, 2020
…pe having reserved name (kubawerlos)

This PR was squashed before being merged into the 2.16 branch (closes #5006).

Discussion
----------

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

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:
```php
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`.

Commits
-------

76dde6f PhpdocToParamTypeFixer - fix for breaking PHP syntax for type having reserved name
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

4 participants