Enhancement: Enable no_alias_function and trailing_comma_in_multiline_array fixer #491
Conversation
.php_cs.dist
Outdated
| )) | ||
| ->setRiskyAllowed(true) |
There was a problem hiding this comment.
I find this concerning - there have been a number of times in the past when php-cs-fixer has shipped changes that resulted in it altering the execution logic of the project code, rather than making purely cosmetic changes. Thankfully, this caused the testsuite to fail, so nothing broken made it through review, but still. Anything that php-cs-fixer labels risky, or has the potential to change execution logic, worries me.
There was a problem hiding this comment.
To clarify - php-cs-fixer has altered logic with no warning, and no changes in configuration. This behaviour is IMO unacceptable. I don't have an issue with the join -> implode changes it's made this time, but if we're going to commit this, I'd like some certainty it won't make changes in the future that break things (e.g. by replacing deprecated function names that are required for PHP 5.3 compatibility).
There was a problem hiding this comment.
That's why I would suggest to pin friendsofphp/php-cs-fixer to at least a specific minor version, what do you think?
There was a problem hiding this comment.
I think pinning is a very good idea. It doesn't entirely solve the issue, but it will help.
@mathroc - php-cs-fixer is yours, how do you feel about this?
.php_cs.dist
Outdated
| 'simplified_null_return' => false, | ||
| 'trailing_comma_in_multiline_array' => false, |
There was a problem hiding this comment.
While we're moving it, can we please turn this rule on? A trailing comma is desirable, as it makes git blame more obvious.
localheinz
force-pushed
the
feature/no-alias-function
branch
2 times, most recently
from
ead8c8a to
531ff9f
Compare
localheinz
changed the title
localheinz
force-pushed
the
feature/no-alias-function
branch
2 times, most recently
from
3ad766b to
c4c6bcd
Compare
localheinz
force-pushed
the
feature/no-alias-function
branch
2 times, most recently
from
2cb0bef to
7662df2
Compare
|
Rebased! Apologies for the delay. |
|
@mathroc please see the conversation on the initial commit of this PR - 4740600#r159148409 |
|
@erayd @bighappyface I think there's been a misunderstanding, why do you think php-cs-fixer is mine ? |
|
@mathroc @bighappyface This is my error sorry - looks like I tagged the wrong person. The person who was supposed to have been tagged was @alcohol. |
|
Can I help in any way? |
|
Uhm, mine? What? |
|
@alcohol Yours, because you added it to this project in the first place. I was wanting to know what your thoughts are on whether pinning it is an acceptable solution to the problem of php-cs-fixer making unexpected and unannounced changes to the execution logic, as I assume you had reasons for setting it up the way you did originally. @localheinz Testing this PR against the latest version [of php-cs-fixer] would be helpful. It's been close to a year since this PR was initially opened, and I don't have time to do that at present (plus I'm not terribly familiar with php-cs-fixer anyway). |
|
Oh, I see. Having read up on the discussion (including the hidden comments), I can add the following: pinning is fine. Though it would be a good idea to occasionally check for updates using |
localheinz
force-pushed
the
feature/no-alias-function
branch
from
7662df2 to
8de75c1
Compare
|
Just rebased and pushed, let me know what I can do! |
|
You did not actually pin php-cs-fixer to a minor version yet though? I would suggest changing "friendsofphp/php-cs-fixer": "^2.1",to "friendsofphp/php-cs-fixer": "~2.2.20", |
|
No, that was never my intention with this PR, but can do in a separate PR. |
|
@alcohol @localheinz PHP-CS-Fixer has been pinned per #537 |
localheinz
force-pushed
the
feature/no-alias-function
branch
3 times, most recently
from
b908726 to
3171d9a
Compare
|
Rebased! |
|
@localheinz please rebase once more. I merged #535. |
localheinz
force-pushed
the
feature/no-alias-function
branch
from
6b0e068 to
53a519e
Compare
|
Done! 🤓 |
localheinz
force-pushed
the
feature/no-alias-function
branch
from
ecf1ce6 to
3a565fa
Compare
localheinz
force-pushed
the
feature/no-alias-function
branch
from
3a565fa to
7898741
Compare
|
Closing in favour of sorting out other issues with the |
This PR
no_alias_functionas well as thetrailing_comma_in_multiline_arrayfixercomposer style-fix💁♂️ For reference, see https://github.com/FriendsOfPHP/PHP-CS-Fixer/tree/2.9#usage: