User tests: Successful: Unsuccessful:
Pull Request for Issue #41420 .
adds native_function_invocation to the php-cs-fixer
Add leading \ before function invocation to speed up resolving.
see https://tideways.com/profiler/blog/compiler-optimized-php-functions
also see https://cs.symfony.com/doc/rules/function_notation/native_function_invocation.html
Please select:
Documentation link for docs.joomla.org:
No documentation changes for docs.joomla.org needed
Pull Request link for manual.joomla.org:
No documentation changes for manual.joomla.org needed
Category | ⇒ | Administration com_admin com_ajax com_associations com_banners com_cache com_categories com_checkin com_config com_contact |
Status | New | ⇒ | Pending |
iirc for most of all other files it should be already used.
it is technicaly different as explained in the referenced docs
from a code review perspective we currently have a mix of native_function_invocation where sometimes its used and sometimes its not. all in the same file. the entire point of coding standards is to avoid that.
So I would adjust your pr to now add a backlash to function calls in files which are not namespaced
assuming that is a typo and should be not and not now
the rule can be adjusted to ['scope' => 'namespaced'].
iirc for most of all other files it should be already used.
with the namepsaced rule there are 585 changed files
some of the changes made when the rule is namespaced is to remove existing \
the non namespaced variant would make me more happy but for now I have to concentrate on alpha 4 released today. It's also discussed in the maintainer chat and will be a topic for tomorrows meeting.
Labels |
Added:
PR-5.0-dev
|
Why should it only be done in namespace files? In template files it is also useful to use the optimized functions.
Why should it only be done in namespace files? In template files it is also useful to use the optimized functions.
I have no idea at all and it appears to make the cs more confusing
the reason is because it's only relevant to namespace files, that's how the php compiler/optimizer works
if you are not in a namespaced file, php doesn't need to search for the function in the local namespace (because it doesn't exists) so it directly looks up in the global namespace.
never the less, yesterdays maintainer meeting decided to change all files (including template which makes it more complicated for people changing templates... but hey we get once again every file changed notification).
The time when this is merged is not set yet maybe rc1.
The time when this is merged is not set yet maybe rc1.
why the delay?
you change 800 files which automatically raises a merge conflicts, it's more important to get the other prs ready and merged then this automatically created optimization (same thing like psr12, there we did it as late to the release as possible).
Also upmerges from 4.4 are not so funny if you have to solve many merge conflicts by hand.
funny that the objection last time was that it was too late in the release cycle
funny that the objection last time was that it was too late in the release cycle
what you mean?
When this is eventually merged then .git-blame-ignore-revs will need to be updated
Labels |
Added:
Release Blocker
|
I would not update the ignire refs. Devs should see why we did that.
I would not update the ignire refs. Devs should see why we did that.
my thinking was that it was the same as the psr12 changes that were excluded. (personaly dont care)
ok - will keep my eye out for that to be merged
Labels |
Added:
Feature
bug
|
@HLeithner done as requested
Status | Pending | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-09-30 22:23:28 |
Closed_By | ⇒ | HLeithner |
thanks
I didn't checked the complete pr but the first files ware all files without a namespace that means, importing it or directly prefix it with \ is not necessary afaik. That's point one. And point 2 it make php code less readable if we do it when it's not needed.
Which is at least for me doing code reviews is important. If it's technical no difference I prefer to have the code better readable.
So I would adjust your pr to now add a backlash to function calls in files which are not namespaced