Feature Release Blocker bug PR-5.0-dev Pending

User tests: Successful: Unsuccessful:

avatar brianteeman
brianteeman
22 Aug 2023

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

avatar joomla-cms-bot joomla-cms-bot - change - 22 Aug 2023
Category Administration com_admin com_ajax com_associations com_banners com_cache com_categories com_checkin com_config com_contact
avatar brianteeman brianteeman - open - 22 Aug 2023
avatar brianteeman brianteeman - change - 22 Aug 2023
Status New Pending
avatar HLeithner
HLeithner - comment - 22 Aug 2023

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

avatar HLeithner
HLeithner - comment - 22 Aug 2023

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

avatar brianteeman
brianteeman - comment - 22 Aug 2023

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 \

avatar HLeithner
HLeithner - comment - 22 Aug 2023

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.

avatar brianteeman brianteeman - change - 23 Aug 2023
Labels Added: PR-5.0-dev
avatar brianteeman
brianteeman - comment - 24 Aug 2023

alternative namespaced only version #41436

avatar laoneo
laoneo - comment - 24 Aug 2023

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

avatar brianteeman
brianteeman - comment - 24 Aug 2023

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

avatar HLeithner
HLeithner - comment - 24 Aug 2023

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.

avatar brianteeman
brianteeman - comment - 24 Aug 2023

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

why the delay?

avatar HLeithner
HLeithner - comment - 24 Aug 2023

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.

avatar brianteeman
brianteeman - comment - 24 Aug 2023

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

avatar HLeithner
HLeithner - comment - 24 Aug 2023

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

what you mean?

avatar brianteeman
brianteeman - comment - 2 Sep 2023

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

avatar brianteeman brianteeman - change - 2 Sep 2023
Labels Added: Release Blocker
avatar laoneo
laoneo - comment - 2 Sep 2023

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

avatar brianteeman
brianteeman - comment - 2 Sep 2023

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)

avatar HLeithner
HLeithner - comment - 30 Sep 2023

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

avatar brianteeman
brianteeman - comment - 30 Sep 2023

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

avatar brianteeman brianteeman - change - 30 Sep 2023
Labels Added: Feature bug
avatar brianteeman
brianteeman - comment - 30 Sep 2023

@HLeithner done as requested

avatar HLeithner HLeithner - close - 30 Sep 2023
avatar HLeithner HLeithner - merge - 30 Sep 2023
avatar HLeithner HLeithner - change - 30 Sep 2023
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2023-09-30 22:23:28
Closed_By HLeithner
avatar brianteeman
brianteeman - comment - 30 Sep 2023

thanks

Add a Comment

Login with GitHub to post a comment