Output warnings for deprecated rules and options

[julienfalque]

This PR adds warnings in the fix command output when using deprecated rules or deprecated options.

 $ ./php-cs-fixer fix --rules='{"is_null": {"use_yoda_style": true}, "method_separation": true}'       
Loaded config default from "/home/julien/Projects/php-cs-fixer/.php_cs.dist".
Option "use_yoda_style" for rule "is_null" is deprecated and will be removed in version 3.0. Use "yoda_style" fixer instead.
Rule "method_separation" is deprecated. Use "class_attributes_separation" instead.

Fixed all files in 3.532 seconds, 12.000 MB memory used

[keradus]

what's wrong with using trigger_error (E_USER_DEPRECATED) in combination of PHP_CS_FIXER_FUTURE_MODE ?
It will crash on deprecated rule/options, it might be adjusted to report to stderr without crashing.

I'm not feeling comfortable with adding event dispatcher to every single class we have :(

[julienfalque]

Using PHP_CS_FIXER_FUTURE_MODE requires to know that PHP_CS_FIXER_FUTURE_MODE exists. I'm pretty sure a lot of users don't.

I don't like to have the event dispatcher everywhere too but I couldn't find a better solution. I think we should introduce a dependency injection container but I'm not sure we can do that in a BC way easily.

[keradus]

Agree about DIC, and agree more that totally out of scope here.

So, we do already trigger errors for all deprecations?
let us register custom error handler, perhaps ?
probably it would have to be done under php-cs-fixer entry file to not break usage of this repo as library.

What I don't like is mostly the fact that we change all classes just to handle deprecations, which are not the base flow. What I was always following here was making the code nice for best case scenario, and uglier for edge cases. And deprecations are edge cases, as they gonna be removed (or actually, already are, on 3.x line)

[keradus]

As said, I'm not willing to inject EventDispatcher to every single fixer.
Do you want to adjust the proposal, or shall we drop it ?

[julienfalque]

Agreed injecting the event dispatcher everywhere for an edge case is definitely far from optimal solution. But I would like to avoid using a custom error handler as using they is sometimes tricky and can bring more problems than it actually solves.

We know we would like to have a dependency injection container in the future. Maybe we can introduce it here and use it only for the event dispatcher for now. We could then refactor gradually other parts of the code to use it as well later, rather than creating a dedicated PR that would probably be huge and hard to review. WDYT?

If you don't like the idea, I'll go with the custom error handler, I don't want to drop the proposal :)

[keradus]

that's the thing. I believe that deprecations shall be handled by E_USER_DEPRECATE, not random event dispatcher :(

[SpacePossum]

Thanks for picking this up 👍
I do however feel that deprecation handling is better to be done using the trigger_error mechanic. While this is not perfect, it is the most widely used and understood.
Still like the DI idea though, just not sure this PR would need it (ATM).

[julienfalque]

PR reworked. Still WIP as there is a few things left to do (mostly checking which deprecation notice is relevant for console output before printing it). Please let me know what you think about the general direction the PR is taking now :)

[keradus]

I like the direction @julienfalque ! 👍

[julienfalque]

Ready for review I think.

[keradus]

shouldn't set_error_handler be part of entry file (php-cs-fixer), not of Application class ?

[julienfalque]

That would require to expose $strErr somehow or pass an OutputInterface instance to $application->run().

[keradus]

like fwrite(STDERR, “sth”); ?

[julienfalque]

like fwrite(STDERR, “sth”); ?

meh :/

[keradus]

if we woud replace this method with

$stdErr->writeln("<bg=yellow;fg=black;>{$message}</>");
@trigger_error($message, E_USER_DEPRECATED);

we would need no custom error handles any more, don't we ?

[julienfalque]

This is not possible where $strErr is not available.

[julienfalque]

@fabpot It looks like fabbot reports a false positive regarding exception message formatting: https://fabbot.io/report/FriendsOfPHP/PHP-CS-Fixer/3699/1a9e0c7e80b5db6a4d1cfb773aa495daba188c5c. The first change is within a string.

[kubawerlos]

What is the status of this PR? Can we add some auto review tests to ensure no one will call trigger_error in fixer directly?

[julienfalque]

Can we add some auto review tests to ensure no one will call trigger_error in fixer directly?

This would fail because MethodArgumentSpaceFixer::fixSpace() is deprecated but the deprecation notice should not be reported to CLI users.

What is the status of this PR?

I think all review feedback has been addressed except @keradus' concern maybe?

[julienfalque]

@keradus @kubawerlos @SpacePossum @localheinz Shall we finish this PR prio to releasing 2.19? It might be interesting for 2.x users as there is a lot of deprecated things. If we merge this in 3.x, it won't be useful until with start deprecating things there.

[keradus]

I would suggest to go this way, @julienfalque
#5674