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

Regression on fully_qualified_strict_types from v3.5 -> v3.6 #6285

Closed
jtreminio opened this issue Feb 12, 2022 · 5 comments
Closed

Regression on fully_qualified_strict_types from v3.5 -> v3.6 #6285

jtreminio opened this issue Feb 12, 2022 · 5 comments
Labels
kind/enhancement topic/fqcn Fully Qualified Class Name usage and conversions

Comments

@jtreminio
Copy link

jtreminio commented Feb 12, 2022

Bug report

fully_qualified_strict_types no longer removes the FQDN from method parameters as of v3.6.

composer.json

{
    "require-dev": {
        "friendsofphp/php-cs-fixer": "3.6"
    },
    "autoload": {
        "psr-4": { "FQST\\" : "src/" }
    }
}

src/Api/Bar.php

<?php

namespace FQST\Api;

use FQST\Model;

class Bar
{
    public function qwe(\FQST\Model\Foo $foo)
    {
        // ...
    }
}

src/Model/Foo.php

<?php

namespace FQST\Model;

class Foo
{
}

.php-cs-fixer.php

<?php

$finder = PhpCsFixer\Finder::create()
    ->in(__DIR__);

$config = new PhpCsFixer\Config();
return $config->setRules([
    'fully_qualified_strict_types' => true,
])
    ->setFinder($finder)
    ->setRiskyAllowed(true);
./vendor/bin/php-cs-fixer fix --verbose
PHP CS Fixer 3.6.0 Roe Deer by Fabien Potencier and Dariusz Ruminski.
PHP runtime: 7.4.27
Loaded config default from "/app/.php-cs-fixer.php".
..                                                                                                                                                      2 / 2 (100%)
Legend: ?-unknown, I-invalid file syntax (file ignored), S-skipped (cached or empty file), .-no changes, F-fixed, E-error

Fixed all files in 0.066 seconds, 6.000 MB memory used

I expected public function qwe(\FQST\Model\Foo $foo) to be changed to public function qwe(Model\Foo $foo).

This is the behavior seen on v3.5:

./vendor/bin/php-cs-fixer fix --verbose
PHP CS Fixer 3.5.0 The Creation by Fabien Potencier and Dariusz Ruminski.
PHP runtime: 7.4.27
Loaded config default from "/app/.php-cs-fixer.php".
F.                                                                                                                                                      2 / 2 (100%)
Legend: ?-unknown, I-invalid file syntax (file ignored), S-skipped (cached or empty file), .-no changes, F-fixed, E-error
   1) src/Api/Bar.php (fully_qualified_strict_types)

Fixed all files in 0.077 seconds, 6.000 MB memory used

results in src/Api/Bar.php

<?php

namespace FQST\Api;

use FQST\Model;

class Bar
{
    public function qwe(Model\Foo $foo)
    {
        // ...
    }
}
@Wirone Wirone added the status/to verify issue needs to be confirmed or analysed to continue label May 16, 2023
@Wirone Wirone added the topic/fqcn Fully Qualified Class Name usage and conversions label Jun 16, 2023
@Wirone
Copy link
Member

Wirone commented Aug 20, 2023

I'm going to close this one in favor of #5199 which describes the need for this behavior with less cognitive load 😉.

BTW. This behavior was changed in #6197 (in v3.6.0) with this comment and I am not completely sure if it was a bug that should have been fixed or unfortunate breaking change 🤷‍♂️. @SpacePossum can you explain why it was changed (if you remember after year and a half 😅)?

@Wirone Wirone closed this as not planned Won't fix, can't repro, duplicate, stale Aug 20, 2023
@Wirone Wirone removed the status/to verify issue needs to be confirmed or analysed to continue label Aug 20, 2023
@SpacePossum
Copy link
Contributor

SpacePossum commented Aug 21, 2023

consider this:

<?php

namespace B\C {
    echo B\Z::Foo;
    echo \B\Z::Foo;
}

namespace B\C\B {
    class Z {
        public const Foo = 1;
    }
}

namespace B {
    class Z {
        public const Foo = 2;
    }
}

results: 12

https://3v4l.org/TJMAe

so removing the leading \ just because the first part of the namespace (B here) is the same as the current namespace is risky

@SpacePossum
Copy link
Contributor

SpacePossum commented Aug 21, 2023

note that the example in #5199 is very different than this issue and the one linked to my comment, as 5199 shows imports in the root namespace (which is also a far more rare case)

@Wirone
Copy link
Member

Wirone commented Aug 22, 2023

@SpacePossum do you think this issue should be reopened then? Or maybe we just can add info to the 5199 to handle both scenarios (root and custom namespace)?

@SpacePossum
Copy link
Contributor

SpacePossum commented Aug 22, 2023

I think the fix that has been done is correct, so if this issue is to be reopened than we need a better example of what is wrong.
5199 can be done by its own I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement topic/fqcn Fully Qualified Class Name usage and conversions
Projects
None yet
Development

No branches or pull requests

3 participants