[5.0] native_function_invocation

[brianteeman]

Pull Request for Issue #41420 .

Summary of Changes

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

cc @HLeithner @wilsonge

Link to documentations

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

[HLeithner]

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

[HLeithner]

iirc for most of all other files it should be already used.

[brianteeman]

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 \

[HLeithner]

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.

[brianteeman]

alternative namespaced only version #41436

[laoneo]

Why should it only be done in namespace files? In template files it is also useful to use the optimized functions.

[brianteeman]

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

[HLeithner]

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.

[brianteeman]

The time when this is merged is not set yet maybe rc1.

why the delay?

[HLeithner]

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.

[brianteeman]

funny that the objection last time was that it was too late in the release cycle

[HLeithner]

funny that the objection last time was that it was too late in the release cycle

what you mean?

[brianteeman]

When this is eventually merged then .git-blame-ignore-revs will need to be updated

[laoneo]

I would not update the ignire refs. Devs should see why we did that.

[brianteeman]

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)

[wilsonge]

Looks reasonable to me

[HLeithner]

#42001 is the last PR for 5.0 after it is merged you can rebuild this pr if you have time for it. I

[brianteeman]

ok - will keep my eye out for that to be merged

[brianteeman]

@HLeithner done as requested

[brianteeman]

thanks