Transform Fully Qualified parameters and return types to short version #3276

veewee · 2017-11-24T19:17:06Z

This PR removes fully qualified parameters and return types to the short versions when they are being imported / in de same namespace:

Result:

<?php

use Foo\Bar;
use Foo\Bar\Baz;

class SomeClass
{
    public function doSomething(Bar $foo): Baz
    {
    }
}

Source:

<?php

use Foo\Bar;
use Foo\Bar\Baz;

class SomeClass
{
    public function doSomething(\Foo\Bar $foo): \Foo\Bar\Baz
    {
    }
}

In older codebases (early namespaces), this was frequently used. Besides that, it can be used to fix autogenerated code from zend-code (zendframework/zend-code#76) and crappy docblocks in the #3110 PR I am working on:

keradus · 2017-11-24T19:21:48Z

what about going even forward and having input like:

<?php

class SomeClass
{
    public function doSomething(\Foo\Bar $foo): \Foo\Bar\Baz
    {
    }
}

so even add imports when they are missing

keradus · 2017-11-24T19:22:09Z

src/Fixer/Import/FullyQualifiedStrictTypesFixer.php

@@ -0,0 +1,219 @@
+<?php
+
+declare(strict_types=1);


please drop it. we supports 5.6

veewee · 2017-11-24T19:29:04Z

Going forward doesnt seems like a good idea since I am changing more tokens into less tokens right?
I was indeed thinking of adding the use statements, but the problem is that I don't know where the imports should be added at te moment. Even if you find the index of the last use statement: at that point you will be adding multiple tokens to the file which makes it hard to loop through the document. Any help is appreciated, but currently I try to focus on getting the basics ready :)

veewee · 2017-11-25T08:50:12Z

@keradus : Is it possible to partially mark a fixer for a specific PHP version? Strict return types are only available from PHP 7.0 on. Should I target PHP >= 7.0 or maybe it's better to split the fixer in 2 different fixers. how do you see this?

Slamdunk · 2017-11-25T17:24:43Z

@keradus I think it would raise a lot the complexity, since the fixer would also need to check already imported class to avoid conflicts, like:

use Foo\Bar;

var_dump(\Other\Bar::class);

@veewee I suggest to try to fix also:

Partial namespace imported:

use Foo\Bar;

// From
var_dump(\Foo\Bar\Baz::class);
// To
var_dump(Bar\Baz::class);

Partial namespace declared:

namespace Foo\Bar;

// From
var_dump(\Foo\Bar\Baz::class);
// To
var_dump(Baz::class);

keradus · 2017-11-26T17:22:04Z

very well, let us keep moving FQCN into imports as separated fixer (so feature request that would not be solved in your PR)

veewee · 2017-11-28T17:31:18Z

@Slamdunk : implemented the partial use + namespaces

keradus · 2017-11-28T21:35:08Z

src/Fixer/Import/FullyQualifiedStrictTypesFixer.php

+     */
+    public function isCandidate(Tokens $tokens)
+    {
+        return PHP_VERSION_ID >= 70000 && $tokens->isTokenKindFound(T_FUNCTION);


why don't fix params on 5.x ? definition says it shall

See question above. I changed the code to make it as PHP 5.6 compatible for parameters only.

Slamdunk · 2017-11-29T08:47:30Z

tests/Fixer/Import/FullyQualifiedStrictTypesFixerTest.php

+    {
+        if (PHP_VERSION_ID < 70000) {
+            $this->markTestSkipped('The strict return type : operator is only avaiable from PHP 7.0.');
+        }


Why not using the dedicated method DocBlock

/** * @requires PHP 7.0 */

?

keradus · 2017-12-02T16:50:08Z

README.rst

@@ -548,6 +548,10 @@ Choose from the list of available rules:
  PHP code must use the long ``<?php`` tags or short-echo ``<?=`` tags and not
  other tag variations.

+* **fully_qualified_strict_types**


please add it to local .php_cs.dist file

keradus · 2017-12-02T16:54:44Z

tests/Tokenizer/Analyzer/ArgumentsAnalyzerTest.php

+                'name_index' => 3,
+                'type' => '',
+                'type_index_start' => -1,
+                'type_index_end' => -1,


-1 is invalid index

keradus · 2017-12-02T16:55:12Z

tests/Tokenizer/Analyzer/ArgumentsAnalyzerTest.php

+                'default' => 'array(1,2)',
+                'name' => '$b',
+                'name_index' => 6,
+                'type' => '',


please use null instead of empty string.

keradus · 2017-12-02T16:57:00Z

src/Tokenizer/Analyzer/ArgumentsAnalyzer.php

+     * @param int    $argumentStart
+     * @param int    $argumentEnd
+     *
+     * @return array


this array looks complicated. I must admit that looking on implementation I have no idea what will be the output - like is name obligatory, or could be empty. when i will get -1 instead of proper index.
probably new DTO object would be good.

keradus · 2017-12-02T16:58:14Z

src/Tokenizer/Analyzer/NamespaceUsesAnalyzer.php

+/**
+ * @internal
+ */
+class NamespaceUsesAnalyzer


why not final ?

keradus · 2017-12-02T16:58:56Z

src/Fixer/Import/FullyQualifiedStrictTypesFixer.php

+    /**
+     * @param string $shortType
+     *
+     * @return array


Token[] perhaps ?

keradus · 2017-12-02T17:00:16Z

tests/Fixer/Import/FullyQualifiedStrictTypesFixerTest.php

+ */
+final class FullyQualifiedStrictTypesFixerTest extends AbstractFixerTestCase
+{
+    public function testImportedStrictTypesFixWithoutReturn()


please use single test method with data provider.
info in names is nice indeed, yet you could as well put that info in data provider

keradus · 2017-12-02T17:00:57Z

tests/Tokenizer/Analyzer/FunctionsAnalyzerTest.php

+     * @param string $code
+     * @param int    $methodIndex
+     * @param array  $expected
+     * @dataProvider provideFunctionsWithArgumentsCases


please separate params form data provider with empty line

keradus · 2017-12-02T17:02:55Z

code in PR is nice, yet reviewing it requires some time. I wonder, maybe you could extract sth from this PR to separated one (like extracting refactoring of analyzers) ?

veewee · 2017-12-05T07:36:59Z

Hi @keradus,

I've applied the remarks. I would prefer to keep both the fixer and the analyzers as one PR. Otherwise it would take me too much time to split them up and bring them together afterwards again.
The code inside the analyzers is code that was taken from existing fixers to make it more reusable.

Take all the time you need to review :)

(BTW: not sure why circle fails. You might want to add more memory?)

keradus · 2017-12-05T11:51:39Z

OK, thanks for update. Please be aware that I will rather not be able to do it this week. Sorry for that :(
Or, perhaps, someone else is willing to take a look as well ? ;)

(CircleCI: we faced the limit when we recently added new tests on 2.2, CirlceCI still works, merged 2.2->master, too much tests, CircleCI crashed. If you are willing to adjust memory, please raise new PR, thanks !)

This PR was merged into the 2.2 branch. Discussion ---------- PHPUnit - set memory limit This PR removes the circle-ci memory issue. (As discussed in #3276) Commits ------- 35c77b3 PHPUnit - set memory limit

veewee · 2017-12-29T08:15:47Z

@keradus : I don't understand why the test is failing. It worked before rebasing against master :)

SpacePossum · 2018-01-02T11:23:52Z

Hi @veewee,
the failing CiIntegrationTest::testIntegration with Broken pipe is not related to the changes you made in the PR, no worries about that one (it is an unstable test we haven't completely figured out yet)

TomasVotruba

@veewee I'm very hapy to see your work. Lot of inspiration for my code 👍

TomasVotruba · 2018-02-23T14:17:11Z

src/Tokenizer/Analyzer/Analysis/TypeAnalysis.php

+        'int',
+        'iteratable',
+        'float',
+        'string',


So beautiful :)

I'd also add mixed and object

http://php.net/manual/pl/function.is-scalar.php

scalar in PHP has a meaning already, reusing scalar word with different definition what is a scalar type actually gonna be super misleading

is_scalar() works only on variables, not strings

This is rather to exclude PHP-reserved types to avoid potential bugs and improve reuse of such class.

I got a fixer that changes varaible name to it's type.
And without mixed/object as scalar (or PHP-reserved types) it made mess like:

/** * @var mixed */ -private $variable; +private $mixed;

i'm not saying anything about calling the is_scalar.
i'm saying that scalar term already has a meaning in PHP as pseudotype, and it doesn't cover objects. reusing the term but with different meaning is super misleading

What term would you suggest?

change used term to some non-established in php internals and document what it is.
for now, i must admit that i have no clue what is this info suppose to be.
reverse of collection? but then why not resource ? no clue

@veewee Nice, thanks for the references in comments!

… terms

veewee · 2018-03-01T19:22:14Z

@keradus : I would love to finish this one up. What's the state of this PR from your point of view?

TomasVotruba · 2018-03-15T19:52:07Z

LGTM. What needs to be done here?

keradus · 2018-03-20T17:22:00Z

@veewee , I see amazing job of yours here and I do appreciate it fully ! It looks really great, and I know this fixer was highly awaited by community!
My point of view is always the same - community need to review, I don't insist it has to be me every single time ;)
Sadly, it's hard to me to keep an eye on every single PR, sorry for big delays here, I take them on my foot :(

@TomasVotruba , thanks for big help with reviewing the PR, it's highly appreciated as well!

keradus · 2018-03-20T17:31:51Z

Finally, merging :D

…o short version (veewee, keradus) This PR was squashed before being merged into the 2.11-dev branch (closes #3276). Discussion ---------- Transform Fully Qualified parameters and return types to short version This PR removes fully qualified parameters and return types to the short versions when they are being imported / in de same namespace: Result: ```php <?php use Foo\Bar; use Foo\Bar\Baz; class SomeClass { public function doSomething(Bar $foo): Baz { } } ``` Source: ```php <?php use Foo\Bar; use Foo\Bar\Baz; class SomeClass { public function doSomething(\Foo\Bar $foo): \Foo\Bar\Baz { } } ``` In older codebases (early namespaces), this was frequently used. Besides that, it can be used to fix autogenerated code from zend-code (zendframework/zend-code#76) and crappy docblocks in the #3110 PR I am working on: Commits ------- d01075c Transform Fully Qualified parameters and return types to short version

TomasVotruba · 2018-03-20T18:09:45Z

@veewee Great job, thank you for all the work.

@keradus Thanks for getting to this. I know you're not the superman, so much appreciated 👍

veewee · 2018-03-21T05:46:57Z

Thanks for all the feedback! Really appreciate it!

@return

This PR was squashed before being merged into the 2.12-dev branch (closes #3640). Discussion ---------- Add PhpdocToReturnTypeFixer ```diff /** * @return \My\Bar */ -function my_foo() +function my_foo(): \My\Bar {} ``` - [x] Mark as *risky* - [x] Require ^7.0 - [x] Fix ^7.1 [*void return*, *iterable*] - [x] Fix ^7.2 [*object type*] - [x] Fix ^7.1 [*nullable types*] - [x] Config to fix scalar types, default to **false** because of unexpected results if *stryct_types* are enabled or not - [x] `@return static` => `: self` - [x] ~Config to overwrite existing mismatching types: PHPDoc wins~ can't do this, because `FunctionNotation\ReturnTypeDeclarationFixer` changes the output of this fixer - [x] Ignore mixed types - [x] Ignore [generics](https://wiki.php.net/rfc/generics) and array of types - [x] Run before `FunctionNotation\ReturnTypeDeclarationFixer` #2207 - [x] Run before `Import\FullyQualifiedStrictTypesFixer` #3276 - [x] Run ~before~ integration test with `FunctionNotation\VoidReturnFixer` #2740 - [x] Run after `Phpdoc\PhpdocScalarFixer` Suggestions for the name of the fixer are welcome. Commits ------- d7a409c Add PhpdocToReturnTypeFixer

veewee changed the base branch from 2.8 to master November 24, 2017 19:19

keradus added the kind/feature label Nov 24, 2017

keradus reviewed Nov 24, 2017

View reviewed changes

veewee force-pushed the feature-import-fq-strict-types branch from 15fcf17 to 6b4d736 Compare November 28, 2017 15:32

keradus reviewed Nov 28, 2017

View reviewed changes

Slamdunk reviewed Nov 29, 2017

View reviewed changes

keradus reviewed Dec 2, 2017

View reviewed changes

src/Tokenizer/Analyzer/NamespaceUsesAnalyzer.php Outdated

/**

* @internal

*/

class NamespaceUsesAnalyzer

Copy link

Member

keradus Dec 2, 2017

Choose a reason for hiding this comment

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

why not final ?

keradus reviewed Dec 2, 2017

View reviewed changes

src/Fixer/Import/FullyQualifiedStrictTypesFixer.php Outdated

/**

* @param string $shortType

*

* @return array

Copy link

Member

keradus Dec 2, 2017

Choose a reason for hiding this comment

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

Token[] perhaps ?

keradus reviewed Dec 2, 2017

View reviewed changes

veewee force-pushed the feature-import-fq-strict-types branch from 5902ad5 to 74fe243 Compare December 5, 2017 07:11

veewee mentioned this pull request Dec 5, 2017

PHPUnit - set memory limit #3292

Merged

veewee mentioned this pull request Dec 29, 2017

Generated getResult() method is incompatible with ResultProviderInterface parent method due to missing return type declaration phpro/soap-client#113

Closed

veewee force-pushed the feature-import-fq-strict-types branch from 74fe243 to 1e82d05 Compare December 29, 2017 07:43

TomasVotruba approved these changes Feb 23, 2018

View reviewed changes

veewee added 2 commits February 26, 2018 08:53

Rename isScalar() to isReservedType() - makes a bit more sense in PHP…

3b95fca

… terms

Rename isScalar() to isReservedType() - makes a bit more sense in PHP…

d9289fd

… terms

veewee added 2 commits March 12, 2018 16:44

Merge conflicts

7af699b

Use new preg wrapper

cf329b9

Merge branch 'master' into feature-import-fq-strict-types

0be7640

keradus mentioned this pull request Mar 20, 2018

StrictMethodsFixer #3110

Closed

keradus closed this Mar 20, 2018

keradus added this to the 2.11.0 milestone Mar 20, 2018

This was referenced Mar 21, 2018

Bump friendsofphp/php-cs-fixer from 2.10.4 to 2.11.0 alcohol/pastebin-php#38

Merged

Bump friendsofphp/php-cs-fixer from 2.10.3 to 2.11.0 simplepie/simplepie-ng#94

Closed

This was referenced Mar 28, 2018

Add PhpdocToReturnTypeFixer #3640

Merged

Add PhpdocFullyQualifiedTypesFixer #3644

Closed

teohhanhui mentioned this pull request Jun 4, 2019

Fix serialization when using interface as resource api-platform/core#2822

Merged

1 task

veewee mentioned this pull request May 23, 2022

feature: FullyQualifiedStrictTypesFixer - Support implements, extends and catch #6395

Closed

Transform Fully Qualified parameters and return types to short version #3276

Transform Fully Qualified parameters and return types to short version #3276

Conversation

veewee commented Nov 24, 2017

keradus commented Nov 24, 2017

Choose a reason for hiding this comment

veewee commented Nov 24, 2017

veewee commented Nov 25, 2017

Slamdunk commented Nov 25, 2017

keradus commented Nov 26, 2017

veewee commented Nov 28, 2017

keradus Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keradus commented Dec 2, 2017

veewee commented Dec 5, 2017 • edited Loading

keradus commented Dec 5, 2017

veewee commented Dec 29, 2017

SpacePossum commented Jan 2, 2018

TomasVotruba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

keradus Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veewee commented Mar 1, 2018

TomasVotruba commented Mar 15, 2018

keradus commented Mar 20, 2018

keradus commented Mar 20, 2018

TomasVotruba commented Mar 20, 2018

veewee commented Mar 21, 2018

keradus Nov 28, 2017 •

edited

Loading

veewee commented Dec 5, 2017 •

edited

Loading

TomasVotruba Feb 23, 2018 •

edited

Loading

keradus Feb 23, 2018 •

edited

Loading