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 - Introduction #4056

Open
wants to merge 25 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@jg-development

jg-development commented Oct 20, 2018

like #4054 add PhpdocToParamTypeFixer
but from master to master

Finishes #3258

@jg-development

This comment has been minimized.

jg-development commented Oct 23, 2018

Are there any objections or can it be merged?

@ntzm

This comment has been minimized.

Contributor

ntzm commented Oct 23, 2018

It will be reviewed in time, it might take a while :) Just sit back and relax

@dmvdbrugge

This is my review so far, I can't guarantee this is all 😉

*/
public function getPriority()
{
// NoSuperfluousPhpdocTagsFixer.

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Oct 23, 2018

Contributor

Please comment not only what priority-relation it has, but also if it has to be run before or after said fixer.

This comment has been minimized.

@jg-development

jg-development Oct 23, 2018

ok I clearify this

public function getPriority()
{
// NoSuperfluousPhpdocTagsFixer.
return 8;

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Oct 23, 2018

Contributor

While you did add a test fixture, I miss the relation in

\PhpCsFixer\Tests\AutoReview\FixerFactoryTest::provideFixersPriorityCases()

This comment has been minimized.

@jg-development

jg-development Oct 23, 2018

I was/am not familiar with this class and the priorityCases ... I will implement it

class Foo
{
/** @param Bar */
function __construct() {}

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Oct 23, 2018

Contributor

Shouldn't you have the params, to be sure it's untouched? This case isn't doing anything anyway now because of doc/signature mismatch.

',
],
'skip resource special type' => [
'<?php /** @param resource */ function my_foo() {}',

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Oct 23, 2018

Contributor

Shouldn't you have the params, to be sure it's skipped?
This case isn't doing anything anyway now because of doc/signature mismatch.

Same for the following 3.

This comment has been minimized.

@jg-development

jg-development Oct 23, 2018

yeah, you are right I fix this

'<?php interface Foo { /** @param Bar $bar */ function my_foo(Bar $bar); }',
'<?php interface Foo { /** @param Bar $bar */ function my_foo($bar); }',
],
'invalid void param on ^7.1' => [

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Oct 23, 2018

Contributor

You say ^7.1 however you pass no version (contrary to the next case). One of them has got to be wrong.

'iterable return on ^7.1' => [
'<?php /** @param iterable $counter */ function my_foo(iterable $counter) {}',
'<?php /** @param iterable $counter */ function my_foo($counter) {}',
70100,

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Oct 23, 2018

Contributor

You test iterable and void, thereby proving that the version-specific types work. I do wonder, do we want object tested as well, just because it has a different version?

This comment has been minimized.

@jg-development

jg-development Oct 23, 2018

added an object test for 7.2

/**
* @var string
*/
private $classRegex = '/^\\\\?[a-zA-Z_\\x7f-\\xff](?:\\\\?[a-zA-Z0-9_\\x7f-\\xff]+)*(?<array>\[\])*$/';

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Oct 23, 2018

Contributor

Is this something we have/want in a more general place? I can't imagine it's only used here, especially seeing there already is a PhpdocToReturnTypeFixer.

After checking, I see the exact regex is listed there, and who knows where more. This is just duplication.

This comment has been minimized.

@jg-development

jg-development Oct 23, 2018

Correct, this is a duplication from PhpdocToReturnTypeFixer, but not anywhere else.
As long as there aren`t more fixers I would not create an abstract, parent or whatever class for one line.

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Oct 23, 2018

Contributor

I'm not talking about creating a new class, I'm saying maybe there already is a class where this is more in place. Like for example, would it make sense to add it to Annotation? Just thinking out loud here.

This comment has been minimized.

@TomasVotruba

TomasVotruba Dec 1, 2018

Contributor

Like for example, would it make sense to add it to Annotation?

👍 Or even better AnnotationRegex or similar, to state purporse of such class.

Also, this should be a constant, since code should not change this value.

AnnotationRegex::CLASS_NAME
* @var array
*/
private $versionSpecificTypes = [
'void' => 70100,

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Oct 23, 2018

Contributor

I don't know about others, but I like lists like these to be sorted, preferably by key.
If the sorting by version is wanted though, please sort the same versions by key.

edit: same goes for the skipped types and blacklisted functions, please sort them.

This comment has been minimized.

@jg-development

jg-development Oct 23, 2018

The sorting is like in the PhpdocToReturnTypeFixer Class. If we want to change the sorting, it should be in all classes or not?

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Oct 23, 2018

Contributor

I'm a simple man, I see new code, I comment on it. If I had reviewed that fixer I had commented the same. Do we want it consistenly everywhere? Imho yes. Does that mean you change it now in existing code? No, guidelines say 1 PR per "issue" so no boyscouting in untouched files. Doesn't mean it shouldn't be done correctly here.

But now you mention it, this is even more duplicated code which I'm unhappy about. Don't know exactly how to proceed in solving that though.

This comment has been minimized.

@jg-development

jg-development Oct 23, 2018

In that case ... OK from my side... sorted it by key + value

@@ -194,6 +194,7 @@ public function provideFixersPriorityCases()
[$fixers['phpdoc_to_return_type'], $fixers['fully_qualified_strict_types']],
[$fixers['phpdoc_to_return_type'], $fixers['no_superfluous_phpdoc_tags']],
[$fixers['phpdoc_to_return_type'], $fixers['return_type_declaration']],
[$fixers['phpdoc_to_param_type'], $fixers['no_superfluous_phpdoc_tags']],

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Oct 23, 2018

Contributor

p comes before r 😉

This comment has been minimized.

@jg-development
class Foo
{
/** @param $bar Bar */

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Oct 26, 2018

Contributor

This annotation is the wrong way around, is that on purpose?

This comment has been minimized.

@jg-development

jg-development Oct 26, 2018

a little bit, but I think I can split this test in 2

{
return [
'no phpdoc param' => [
'<?php function my_foo() {}',

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Oct 26, 2018

Contributor

For this and the next 3 cases: yes they are all invalid, however, there are no parameters to typehint anyway, so the fixer can't do anything anyway. Shouldn't they have a parameter?

jg-development added some commits Oct 26, 2018

@jg-development

This comment has been minimized.

jg-development commented Nov 1, 2018

any open tasks or comments for me?

jg-development added a commit to jg-development/PHP-CS-Fixer that referenced this pull request Nov 8, 2018

@jg-development jg-development force-pushed the jg-development:master branch from 17fe7cf to 063ce09 Nov 8, 2018

@jg-development

This comment has been minimized.

jg-development commented Nov 8, 2018

I updated a fix for stop searching the param variable with the last token.
I tested the fixer with several projects and everything was working fine.
Can somebody merge the request please?

@dmvdbrugge

This comment has been minimized.

Contributor

dmvdbrugge commented Nov 8, 2018

Patience my friend, so far, only 1 contributor (me) has looked at your code, no actual maintainers seem to have taken interest yet.

jg-development added some commits Nov 25, 2018

@jg-development

This comment has been minimized.

jg-development commented Nov 26, 2018

@julienfalque :
you are right, i added the changes regarding your comments

@TomasVotruba

This comment has been minimized.

Contributor

TomasVotruba commented Nov 27, 2018

Just a note from using TypeHintDeclarationSniff that does the same job.

It works great, but only with local scope. Thus it's buggy in cases with parent class or interfaces:

3rd party code

<?php

interface Sniff
{
    /**
     * @param int $position
     */
    public function proces(File $file, $position);
}

My code "fixed", but broken:

 <?php

 final class MySniff implements Sniff
 {
     /**
      * @param int $position
      */
-    public function proces(File $file, $position)
+    public function proces(File $file, int $position)
     {
     }
 }

Does this fixer deals with that?

@jg-development

This comment has been minimized.

jg-development commented Nov 30, 2018

Hi @TomasVotruba :
The fixer would fix the interface and the class seperatly. (3rdParty not, if not in scope)

The fixer itself does not look for a parent class or interface method and compare the new output.
In general fixers are called by file, there is no option to "lookup" for the parent class.
I think this cannot be done by the fixer alone, this is a designchange of the php-cs-fixer.
The only quick possible solution I can think of, is an option to add namespaces like "\3rdParty"
and if an file uses this namespace, it will be ignored.
But automatic lookup by the fixer itself is not possible in this structure.
Greetings Jan

@TomasVotruba

This comment has been minimized.

Contributor

TomasVotruba commented Nov 30, 2018

Hi @jg-development, thanks for the answer. So it will break the code in such case?

It's really time exhausting to go through PHP errors in the code and manually each one of them, if there is a conflict with parent class/interface/trait.

@TomasVotruba TomasVotruba referenced this pull request Nov 30, 2018

Merged

[PHP] Add ParamAndReturnScalarTypehintsRector #795

10 of 10 tasks complete
@jg-development

This comment has been minimized.

jg-development commented Nov 30, 2018

yes it does

@julienfalque

This comment has been minimized.

Member

julienfalque commented Dec 1, 2018

This PR is looking pretty good but the fixer seems to have a lot of code from PhpdocToReturnTypeFixer. Wouldn't it be better to refactor those fixers to avoid duplicating the code?

'<?php /** @param Foo[] $foo */ function my_foo($foo) {}',
],
'nullable array of types' => [
'<?php /** @param null|Foo[] $foo */ function my_foo(?array $foo) {}',

This comment has been minimized.

@TomasVotruba

TomasVotruba Dec 1, 2018

Contributor

What about:

  • null|Foo[]|array?array
  • null|Foo[]|Bar[]?array
  • Foo[][]array

Also this:

  • Foo[]|iterablearray
@jg-development

This comment has been minimized.

jg-development commented Dec 1, 2018

@TomasVotruba :
Some kinds of param can be done, but not something like this:
Foo[][]
The problem is the DocBlock\Annotation Class. The regex for content and types is not ready for such cases.

@TomasVotruba

This comment has been minimized.

Contributor

TomasVotruba commented Dec 1, 2018

Why not extend it? Foo[][] is pretty clear

@jg-development

This comment has been minimized.

jg-development commented Dec 1, 2018

possible, but in my feeling not part of this story/task. another issue with extending the annotation class to return the correct types and this can be done.
And I think there could be some other fixers, that could use the change, too.

Personally, than I would change the fixer to identify all types and generate the annotation from the result. Like $hasNull + $hasIterable = ?array and so on.

@TomasVotruba

This comment has been minimized.

Contributor

TomasVotruba commented Dec 1, 2018

Well, when null|Foo[]?array is already supported,
I'd expect null|Foo[][]?array to work as well.

But I understand you want to merge this huge PR and this might be changed on @return as well, thus scope for another PR.

For reliable annotation parsing you can use https://github.com/phpstan/phpdoc-parser
It works very well with all the cases I've mentioned above (format preserving printing also supported in another package).

@jg-development

This comment has been minimized.

jg-development commented Dec 1, 2018

Ok, I did a refactor of the fixer and modified the annotation class for multiple array annotations.
This had an impact on the phpdoctoreturntypefixer as well (imho positive).

New covered use cases are:

            'nullable and mixed types of arrays' => [
                '<?php /** @param null|Foo[]|Bar[] $foo */ function my_foo(?array $foo) {}',
                '<?php /** @param null|Foo[]|Bar[] $foo */ function my_foo($foo) {}',
                70100,
            ],
            'nullable and array and array of types' => [
                '<?php /** @param null|Foo[]|array $foo */ function my_foo(?array $foo) {}',
                '<?php /** @param null|Foo[]|array $foo */ function my_foo($foo) {}',
                70100,
            ],
            'nullable array of array of types' => [
                '<?php /** @param null|Foo[][] $foo */ function my_foo(?array $foo) {}',
                '<?php /** @param null|Foo[][] $foo */ function my_foo($foo) {}',
                70100,
            ],
            'array and iterable param' => [
                '<?php /** @param Foo[]|iterable $foo */ function my_foo(array $foo) {}',
                '<?php /** @param Foo[]|iterable $foo */ function my_foo($foo) {}',
                70100,
            ],

jg-development added a commit to jg-development/PHP-CS-Fixer that referenced this pull request Dec 1, 2018

@TomasVotruba

This comment has been minimized.

Contributor

TomasVotruba commented Dec 2, 2018

@jg-development Amazing work 👏

@jg-development jg-development force-pushed the jg-development:master branch from 5392a66 to 4c9f85b Dec 2, 2018

jg-development added a commit to jg-development/PHP-CS-Fixer that referenced this pull request Dec 2, 2018

@jg-development jg-development force-pushed the jg-development:master branch from 4c9f85b to 9c0f0fc Dec 2, 2018

@jg-development

This comment has been minimized.

jg-development commented Dec 2, 2018

ok ... was not aware that switch case is not allowed .... using kind of polymorphism now

@SpacePossum SpacePossum changed the title from add PhpdocToParamTypeFixer to PhpdocToParamTypeFixer - Introduction Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment