feat: add phpDoc support for fully_qualified_strict_types fixer

[Jadro007]

Fixes #3958.

I added support to fully_qualified_strict_types fixer to be able to work with PHPDoc, as discussed it #3958. This is my first PR to PHP-CS-Fixer, I tried to adhere to the contributing rules, hopefully I didn't miss anything important.

There is one small issue probably with integration of some other fixer, I hope we can resolve it.

[Wirone]

Looks interesting! @Jadro007 Any chance for a rebase? 🙂

[babeuloula]

Hello, @Jadro007 you're PR is very interesting, can you rebase ? I would like to use to it in my code.

[coveralls]

coverage: 94.917% (+0.003%) from 94.914%
when pulling dbe0e27 on Jadro007:feature-support-PHPDoc-fully-qualified-strict-types
into be0e60f on PHP-CS-Fixer:master.

[Wirone]

@kubawerlos @keradus (@mvorisek too): second pair of eyes needed 👀.

[GrahamCampbell]

This is a breaking change, and should have been behind config, defaulting to disabled in the current major version series.

[Wirone]

What's breaking here 🤔? The purpose of this fixer is well known, we only covered new places where FQCN can be shortened.

[GrahamCampbell]

But the fixer never used to mess with phpdoc comments. When other fixers added new functionality that was totally new, they added config, even when they had a vague description that could be interpreted to include the new functionality already, such as due to new language features.

[Wirone]

The fixer did not touch implements, extends, catch, static calls etc, do you consider this addition as BC break too? Previous supported code kinds were not configurable, why should new ones be 🤔? I don't agree this is BC break, it's just enhanced coverage for the single feature this fixer provides.

Importing symbols is something new, though, that's why it's behind the config option.

[GrahamCampbell]

Well, I just ran upgraded and had thousands of changes that I wasn't expecting. I'm reverting the code changes on the version I'm using for now, so I can continue to use the features that I want. Even if this is not a BC break, the likes of Laravel framework and the default Laravel app structure wants FQCNs in phpdoc but not elsewhere, so there is a big audience for making this configurable (and a big audience which may consider this change breaking).

[Wirone]

Why Laravel expects FQCNs in phpDoc? This should not make any difference, especially because this fixer requires imports to be in place to be shortened (unless import_symbols is enabled).

[keradus]

A reminder of some docs we have about feature vs bug: https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/master/feature-or-bug.rst

With that, it doesn't specify if new feature inside single fixer is consider BC break. It is not - similar to ruleset. With new feature implemented, fixer can cover more and more cases.

@Wirone , probably you won't change whatever status-quo of coding standard for whatever ecosystem. (Feel free to deep-dive into reasoning, but don't expect change in it)

@GrahamCampbell , if "big audience" wants sth differently, contributions are welcome.

[GrahamCampbell]

I am just discussing whether or not this should be behind a config flag at this point. If there is no agreement, there is no point in submitting a PR. I may submit a PR if there is agreement on the approach.

[Wirone]

I don't expect any change, I am just curious about what @GrahamCampbell said 🙂. There is already one exception in the fixer, it does not touch @covers because PHPUnit requires FQCN. If there are other scenarios where this is a strict requirement, we can (and should) cover it. But I don't agree this change is a BC break per se.

[GrahamCampbell]

OK, if this is a hard no, then Laravel will just continue to operate off of a fork with this PR reverted.

[GrahamCampbell]

You know, Laravel may eventually change it's code style, but there would need to be mechanism to still use the old style for older Laravel versions, regardless.

[keradus]

If there are other scenarios where this is a strict requirement, we can (and should) cover it. But I don't agree this change is a BC break per see.

OK, if this is a hard no, then (...)

I don't get how Wirone's "we can cover it" is a "hard no".

[GrahamCampbell]

He was asking about specific phpdoc annotations, to which my answer would be all of them, which is equivalent to reverting the PR.

[Wirone]

We can still cover it by introducing the config option as a feature, not as a BC break fix 😉. At any point I wasn't against introducing such opt-out, I was trying to understand why Laravel requires something that does not make sense (at least for me) 😅.

[stayallive]

For what it's worth, I have many projects where the code style is that phpdoc contains the full FQCN, now seeing a big flood of changes like:

-       /** @var \Stripe\SubscriptionItem $anySubscriptionItem */
+       /** @var SubscriptionItem $anySubscriptionItem */
        $anySubscriptionItem = $currentSubscriptionPlan ?? $currentSubscriptionAddons->first();

And also (file is in the root namespace, no use statements):

 *
-* @return \Stripe\StripeClient
+* @return Stripe\StripeClient
 */

Having a way to configure the behavior for phpdoc specifically would be very much appreciated to not have to modify our code style and can continue to use php-cs-fixer latest and greatest (it would be really nice if the full FQCN could even be enforced/fixed in the phpdoc but that might be a feature request all on it's own).

[Wirone]

@stayallive in #7628 I provided a way to configure how phpDocs are handled in terms of FQCN, so you will be able to disable it completely if you don't want to shorten types there. Also, leading_backslash_in_global_namespace option should prevent this fixer from trimming leading \ in global namespace - if it still trims the leading backslash with this option enabled, then it's a separate bug that needs to be fixed.