minor: "Callback" must not be fixed to "callback" by default

[mvorisek]

Callback is very legit class name [1] and alias group should not be fixed by default by any rule/ruleset by default.

In the past it was maybe helpful, but nowdays static analysers can detect invalid class name easily (like Callback) and advise the user to fix it manually to callable.

callback is still fixed to callable by phpdoc_scalar rule.

[1] Used for ex. in https://github.com/atk4/ui/blob/4.0.0/src/Callback.php project heavily and almost any project that depends on it refers to this classname, ie. all projects had to disable the the rule explicitly - https://github.com/atk4/ui/blob/4.0.0/.php-cs-fixer.dist.php#L24 - which is definitely not expected from any default config

[Wirone]

I think this PR solves more your project's issue than issue in general.. Changing how fixer works by default should be done only if there's valid reason for that. Right now you can just configure this fixer explicitly and achieve the same result, without breaking backward compatibility for other users (fixer is included in @PhpCsFixer and @Symfony rulesets, so it would potentially affect many users).

It would be better to ad some configuration option to the fixer so single types could be excluded from the list, without the need to disable whole group.

Alternatively, fixer could check imports in the file and if there's class with such name imported, it shouldn't modify the casing of phpDoc type.

[SpacePossum]

IIUC this also applies for mixed as Mixed is a valid classname, which is in the meta configuration option.

It would be better to ad some configuration option to the fixer so single types could be excluded from the list, without the need to disable whole group.

👍 it would provide the flexibility for the endusers to just pick and choose what to fix and what to leave alone.

[mvorisek]

IIUC this also applies for mixed as Mixed is a valid classname, which is in the meta configuration option.

It would be better to ad some configuration option to the fixer so single types could be excluded from the list, without the need to disable whole group.

👍 it would provide the flexibility for the endusers to just pick and choose what to fix and what to leave alone.

No, mixed - https://3v4l.org/eWEfi and other types from simple/meta groups are reserved keywords of php-src engine and fixing the case is safe transformation.

Fixing Callback to callback has however no reason from php-src engine perspective. It is simply some unsafe/unreasoned assumption by the author of this rule and I belive it should be dropped.

[Wirone]

@mvorisek yeah, I did nod find any reference to callback in phpDocumentor or PHPStan types list, so probably better solution would be to remove it from aliases list instead of removing this group from default configuration?

[SpacePossum]

Think I tried to say is, Mixed is allowed on 7.4 https://3v4l.org/WEal7#v7.4.33 which is also still supported by this tool.
Also Scalar of meta as class name is valid on https://3v4l.org/gMI5X#v8.2.6 8.2.

[mvorisek]

@mvorisek yeah, I did nod find any reference to callback in phpDocumentor or PHPStan types list, so probably better solution would be to remove it from aliases list instead of removing this group from default configuration?

What about other alias types, do you prefer to remove Callback only or the alias group completely - https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/a6e4da5d/src/Fixer/Phpdoc/PhpdocTypesFixer.php#L52-L56 (same reasoning)?

[Wirone]

@mvorisek integer, boolean are supported by PHPStan and phpDocumentor, double by the former, so I would keep them. Only real seems unused, I don't know the reasoning for it.

[mvorisek]

@mvorisek integer, boolean are supported by PHPStan and phpDocumentor, double by the former, so I would keep them. Only real seems unused, I don't know the reasoning for it.

real is very, very historical php-src alias of float - https://www.php.net/manual/en/function.is-real.php. I will update this PR shortly and remove callback and real.

[Wirone]

To be clear: I know that real is a float, I just don't know why it was here, on the aliases list 😅.

[mvorisek]

@mvorisek integer, boolean are supported by PHPStan and phpDocumentor, double by the former, so I would keep them. Only real seems unused, I don't know the reasoning for it.

PR is updated, callback and real aliases are removed as both are not valid php types, nor reserved php-src keywords nor recognized by static analysers - good discovery @Wirone 👍.

[Wirone]

👍 from me, but I would like to hear word from someone with bigger experience here.

[SpacePossum]

I would prefer to leave these in and instead; deprecate all options, create a new option called types, make that accept all current values, than people can pick and choose.
For example, as stated, Scalar is a valid class name as well (and even more in the list), so I think this doesn't end with callable.

[mvorisek]

@SpacePossum this PR is based on currect static analysers support, scalar and integer are indeed valid class names, but they are understood by static analysers, so it is good to keep them. The only two unreasoned names are callback and real, these two names are not recognized as aliases by static analysers, so these two names are removed.

[kubawerlos]

No, mixed - https://3v4l.org/eWEfi and other types from simple/meta groups are reserved keywords of php-src engine and fixing the case is safe transformation.

Not really true, resource and (as already mentioned) scalar are not reserved keywords, the same as all 5 aliases: https://3v4l.org/NDi77

Please, update the test case with config as after this change the callback type there could be misleading.

[mvorisek]

No, mixed - https://3v4l.org/eWEfi and other types from simple/meta groups are reserved keywords of php-src engine and fixing the case is safe transformation.

Not really true, resource and (as already mentioned) scalar are not reserved keywords, the same as all 5 aliases: https://3v4l.org/NDi77

real and callback are names NOT recognized by major static analysers, other (kept) names are.

Please, update the test case with config as after this change the callback type there could be misleading.

done

[kubawerlos]

👍🏼

[Wirone]

Thanks @mvorisek 🍻