Php-cs-fixer: YodaStyleFixer with function calls

[pocketrocket]

The PHP version you are using:

$ php -v
PHP 7.1.8 (cli) (built: Aug 16 2017 17:19:54) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.1.0, Copyright (c) 1998-2017 Zend Technologies
    with Xdebug v2.5.5, Copyright (c) 2002-2017, by Derick Rethans

PHP CS Fixer version you are using:

$ php-cs-fixer -v
PHP CS Fixer 2.6.0 Local Tavern by Fabien Potencier and Dariusz Ruminski

The command you use to run the fixer (and the configuration file you are using if any).

$ php-cs-fixer fix --verbose --rules='{"yoda_style":true}' [PATH_TO_FILE]

Before Fixer:

<?php
$foo = 123;
$bar = $foo === count($foo);

After Fixer (actually):

<?php
$foo = 123;
$bar = $foo === count($foo);

After Fixer (expected):

<?php
$foo = 123;
$bar = count($foo) === $foo;

[SpacePossum]

Hi and thanks for the report.

The behavior of the YodaStyle fixer you observed is intended. It was discussed in various PR's and issue, both in this repo and over at Symfony.
There are requests for adding more options to this fixer to change its behavior, for example a similar discussion is going on here https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3121

Please let us know if you've time to help out!

[julienfalque]

@SpacePossum Are you sure this is expected? I think the fixer is supposed to move the variable from the left side to the right side of the operator.

3674 Fixes this issue.

[SpacePossum]

Pretty sure, the fixer is the result of a very long discussion over multiple PR's and reverts, ~3121~ ~https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/2446~ https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/701 is a nice start/end on the process. I'm not in any way saying it is the _only_ good way the fixer could work and I would welcome new options, however we should be careful not to change the default (and prevent doing the whole specification discussion again ;))
Part of the confusion and the long discussion also resulted in less good naming in the current implementation, such as the method name isVariable used to detect that, but ended up detected candidates (which are not always variables only).

edit:
https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/701 is a better start to understand why this was done as it is

[SpacePossum]

(https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/3505 is however a proof of a real implementation issue and the related issue is a valid bug report)

[julienfalque]

If I understand correctly, you are referring to https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/693#discussion_r19977208, right?

I realize that the current behavior of the fixer is pretty opaque, I thought it was following these rules to apply Yoda style:

• if left side is a variable and right side is not, move the variable to the right (initial purpose of Yoda style);
• else if there are no variable but right side is a simple scalar constant, move it to the left (to ease readability, as most developers that apply Yoda style probably expect the scalar value to be on left side).

I submitted #3674 with those rules in mind. IMO the first rule is mandatory and should not have exception. I think this is the behavior most users expect and should be the default.

I'm a bit confused about what to do now: should I keep #3674 as is (bringing behavior of the fixer closer to the rules above)? Should the default remain unchanged and rules above applied strictly with a new option only? Something else?

[SpacePossum]

It is hard to pin it down to one comment, as the discussion went over multiple PR's, both here and on the SF repo and in gitter as well IIRC.
To me it is not about what the current users would expect or not. Back in the day a compromise was reached between a lot of parties and I see no need to change the default. Now we have a nice configuration system that can bring opt-in/out functionality, which was lacking back than.
Therefore IMHO I think the best way forward is fixing the clear bug and everything else would be nice to bring in with new config options.
Discussing one functional-optional-change at the time in one PR would be the best and the _only_ way forward _IMHO_ :)

[julienfalque]

Ok, WDYT about:

• reducing #3674 scope by keeping the fix for #3335 _only_;
• submitting a new PR to master introducing new options (e.g. always_move_variable and always_move_scalar_constant) which would make the fixer follow the rules I mentionned above and thus cover the case described here?

[SpacePossum]

sounds like a solid plan to me :)

[julienfalque]

See #3698.