Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change default config of native_function_invocation #3995

Merged
merged 1 commit into from Oct 8, 2019

Conversation

dunglas
Copy link
Contributor

@dunglas dunglas commented Sep 18, 2018

According to the following comment of @nicolas-grekas, the default configuration of native_function_invocation should be changed to prepend a backslash only to functions that can be optimized by the PHP engine:

Adding more functions as opcodes would make the engine slower, because routing more opcodes requires more CPU. So this will not happen :)
The policy is to never add , except when it provides a significant advantage, i.e. for those special functions that php-cs-fixer/fabbot handle.

symfony/panther#61 (comment)

This PR changes the default config accordingly.

@dmvdbrugge
Copy link
Contributor

Not sure if I personally agree to this, but if this is what's wanted, it's a bc-break so it has to target 3.0 not master.

@dunglas dunglas changed the base branch from master to 3.0 September 18, 2018 11:55
@keradus
Copy link
Member

keradus commented Sep 18, 2018

Changing default configuration of rule is BC breaker indeed, @dmvdbrugge , good spot 👍

Sadly, I see sth wrong happen with this branch - I see lot of not related changes in diff.

@dmvdbrugge
Copy link
Contributor

Probably because he just targeted the other branch, instead of rebasing first

@dmvdbrugge
Copy link
Contributor

It also shows @keradus that you have not yet merged the 2.13 release into 3.0

@dunglas
Copy link
Contributor Author

dunglas commented Sep 18, 2018

Sadly, I see sth wrong happen with this branch - I see lot of not related changes in diff.

I tried the "change base" button provided by GitHub, but it doesn't rebase automatically. This is fixed now.

@keradus
Copy link
Member

keradus commented Sep 18, 2018

@dunglas
Copy link
Contributor Author

dunglas commented Sep 18, 2018

@keradus done

@SpacePossum
Copy link
Contributor

sadly the tests are failure stopping this PR from being merged

@julienfalque julienfalque removed the WIP label Oct 7, 2019
@SpacePossum SpacePossum added the RTM Ready To Merge label Oct 8, 2019
@SpacePossum
Copy link
Contributor

Thank you @dunglas.

SpacePossum added a commit that referenced this pull request Oct 8, 2019
…las, SpacePossum)

This PR was squashed before being merged into the 3.0 branch (closes #3995).

Discussion
----------

Change default config of native_function_invocation

According to the following comment of @nicolas-grekas, the default configuration of `native_function_invocation` should be changed to prepend a backslash only to functions that can be optimized by the PHP engine:

> Adding more functions as opcodes would make the engine slower, because routing more opcodes requires more CPU. So this will not happen :)
> The policy is to never add \, except when it provides a significant advantage, i.e. for those special functions that php-cs-fixer/fabbot handle.

symfony/panther#61 (comment)

This PR changes the default config accordingly.

Commits
-------

39116ba Change default config of native_function_invocation
@SpacePossum SpacePossum merged commit 39116ba into PHP-CS-Fixer:3.0 Oct 8, 2019
@SpacePossum SpacePossum removed the RTM Ready To Merge label Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants