Problems with PHP-CS-Fixer

yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework

http://www.yiiframework.com

BSD 3-Clause "New" or "Revised" License

14.24k stars 6.91k forks source link

Problems with PHP-CS-Fixer #14500

Closed cebe closed 7 years ago

cebe commented 7 years ago

Because of #14492 I have reviewed the changes made by PHP-CS-Fixer in ba0ab403b52124c941dbeb46fbd9efdc12252a5d by @rob006 again and found the following issues. Listing them here because navigating comments in such large PRs is a pain on github:

[x] https://github.com/yiisoft/yii2/commit/ba0ab403b52124c941dbeb46fbd9efdc12252a5d#commitcomment-23214791 (int) cast has been lost.
[x] https://github.com/yiisoft/yii2/commit/ba0ab403b52124c941dbeb46fbd9efdc12252a5d#commitcomment-23214855 if statement with PHP-code only should not be complicated in views.
[x] https://github.com/yiisoft/yii2/commit/ba0ab403b52124c941dbeb46fbd9efdc12252a5d#commitcomment-23214839 misses an echo, see #14492
[x] https://github.com/yiisoft/yii2/commit/ba0ab403b52124c941dbeb46fbd9efdc12252a5d#commitcomment-23214862 no need to close and reopen PHP tag. in PHP code in views
[x] https://github.com/yiisoft/yii2/commit/ba0ab403b52124c941dbeb46fbd9efdc12252a5d#commitcomment-23214878 misses an echo, see #14492

samdark commented 7 years ago

@rob006

samdark commented 7 years ago

I've fixed print_r related regressions and (int) cast in master. These are definitely bugs in CS fixer or rules so it should be fixed there as well.

If statements changes in views are looking OK to me though.

rob006 commented 7 years ago

Most of these changes are done by me, not php-cs-fixer. As far as I remember it has some indentation issues with multiline if so I converted these views to follow Plates rules (it seems that Yii follows them in other places anyway). Personally I think that these rules makes views more readable and forces good practices on you - you should avoid having complicated logic in views, and it is really hard to having it and following Plates rules in the same time. Personally it helps me detect CS issues in views in my projects and and make them more readable, but this is your choice, you can revert it (and then you should probably exclude these files from php-cs-fixer processing). But inline ifs like if ($domething) $this->doSomething() is a nightmare, I see no reason to make exception and allow them in views.

This print_r bug is definitely my fault, sorry about that. :/

About (int) typecast - do we really need this? + 1 should typecast this to int anyway.

samdark commented 7 years ago

Views part is fine. I remember (int) typecast was there for a reason but have no time do dig into it now...

samdark commented 7 years ago

Was print_r done by you or by CS fixer?

rob006 commented 7 years ago

Probably me, but I don't remember exactly what happened then. Try run php-cs-fixer on master and if it does not break it again, I would assume that was my change.

samdark commented 7 years ago

Done. Seems these either were changed made before adjusting current rules or were made manually. Anyway, current fixer rules set is fine with that case.