Use default native_constant_invocation rule for PhpCsFixer:risky ruleset#5469
Use default native_constant_invocation rule for PhpCsFixer:risky ruleset#5469mvorisek wants to merge 2 commits intoPHP-CS-Fixer:2.18from
Conversation
1598838 to
bcf21d4
Compare
| '@Symfony:risky' => true, | ||
| 'comment_to_phpdoc' => true, | ||
| 'final_internal_class' => true, | ||
| // @TODO: consider switching to `true`, like in @Symfony |
There was a problem hiding this comment.
the decision for this action was not yet done.
merging this PR as-is will cause a lot of conflicts between 2.x <> 3.x and lead to conflicts on PRs too
There was a problem hiding this comment.
also, default config for this rule changed in v3: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/3.0/UPGRADE-v3.md#changed-default-values-of-options
There was a problem hiding this comment.
In Symfony:risky ruleset this was changed in 2.x. Thus I propose to change PhpCsFixer:risky ruleset in 2.x as well.
There was a problem hiding this comment.
I understood that intention from PR title,
unfortunately, your comment does not address my concerns
There was a problem hiding this comment.
Can I help addressing your concerns? You want this change in 2.x or 3.x only?
There was a problem hiding this comment.
Due to amount of conflicts it gonna create for us, syncing between v2<>v3 and all open PRs, I don't see big value of having this change merged right now.
What would be the benefit overcoming the downsides?
There was a problem hiding this comment.
For these reasons, I opened this PR into v2. I belive, FQCN are important as they can be optimized at compile time and a reason why it is already default in the Symfony ruleset.
There was a problem hiding this comment.
it does not solve the concern for conflicts for all open PRs.
Let's reject the PR for now and re-evaluate this change after we decrease amount of open PRs or stop v2 support
|
@keradus why close? The same as we optimize Zend optimized functions to use FQCN, the same we must optimize core php constants. |
|
@keradus can I please get your reaction on this? I think constants should be really FQN, like functions. |
|
You can read here why @keradus closed the PR. For the project we've to consider the sparse time maintainers have and try to keep away from big conflicts. This is than weighted against the possible upsides of the changes.
The ruleset
You can configure it for you own projects, it is not the reason why the PR is merged or not. |
|
I understand the concerns, but conflicts can be solved easily by applying CS fixer to both branches prior merging easily. See https://3v4l.org/VcUv2/vld#output and https://3v4l.org/npt1J/vld#output opcodes generated. Using FQ constant names is important for performance as if not used absolutely, constant names are resolved on every access (not even once per file load!) which causes unneeded overhead. Here is a functional benchmark: https://3v4l.org/7E3Af and https://3v4l.org/26W95 . Please reconsider this once again. Even if php performance will improve 10x, this overhead will probably stay fixed as php engine can not optimize it more. And it will be consistent with functions, which are already FQ/fixed. The 2nd benchmark shows large improvement as the constants can be resolved on compile time which is very helpful for internal php optimizations and JIT. |
as in Symfony:risky, fix todo
2nd commit is generated - pure CS fix