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

php-cs-fixer fix . --rules=native_function_invocation #31198

Merged
merged 3 commits into from
Apr 21, 2018

Conversation

DeepDiver1975
Copy link
Member

Description

php functions should be called with their full namespace
one of many resource about the details: https://veewee.github.io/blog/optimizing-php-performance-by-fq-function-calls/

Motivation and Context

  • Write proper code
  • Give the code some speedup

How Has This Been Tested?

Pure unit testing

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@DeepDiver1975 DeepDiver1975 added this to the development milestone Apr 19, 2018
@DeepDiver1975 DeepDiver1975 self-assigned this Apr 19, 2018
@codecov
Copy link

codecov bot commented Apr 19, 2018

Codecov Report

Merging #31198 into master will increase coverage by <.01%.
The diff coverage is 65%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #31198      +/-   ##
============================================
+ Coverage     62.57%   62.57%   +<.01%     
+ Complexity    18412    18234     -178     
============================================
  Files          1145     1145              
  Lines         68416    68396      -20     
  Branches       1234     1234              
============================================
- Hits          42809    42799      -10     
+ Misses        25246    25236      -10     
  Partials        361      361
Flag Coverage Δ Complexity Δ
#javascript 52.05% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 63.77% <65%> (ø) 18234 <25> (-178) ⬇️
Impacted Files Coverage Δ Complexity Δ
...deratedfilesharing/templates/settings-personal.php 100% <ø> (ø) 0 <0> (ø) ⬇️
apps/dav/lib/CalDAV/CalendarObject.php 96.77% <ø> (ø) 18 <0> (ø) ⬇️
...s/dav/appinfo/Migrations/Version20170519091921.php 0% <ø> (ø) 1 <0> (ø) ⬇️
apps/files_sharing/templates/public.php 0% <ø> (ø) 0 <0> (ø) ⬇️
apps/dav/lib/Files/RootCollection.php 15.38% <0%> (ø) 4 <0> (ø) ⬇️
apps/dav/lib/CalDAV/Plugin.php 0% <0%> (ø) 2 <0> (ø) ⬇️
apps/files_sharing/lib/Activity.php 5.34% <0%> (ø) 97 <0> (ø) ⬇️
apps/files_trashbin/appinfo/update.php 0% <0%> (ø) 0 <0> (ø) ⬇️
apps/files_trashbin/appinfo/app.php 54.54% <0%> (ø) 0 <0> (ø) ⬇️
apps/files/lib/ActivityHelper.php 0% <0%> (ø) 8 <0> (ø) ⬇️
... and 180 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10b91a7...f929720. Read the comment docs.

@DeepDiver1975 DeepDiver1975 force-pushed the code-quality/fully-qualified-function-calls branch from 8908f52 to fa32be5 Compare April 20, 2018 06:38
@mmattel
Copy link
Contributor

mmattel commented Apr 20, 2018

WOW
If this gets merged, we need to add a note in the developer guide !
I file a small reminder in documentation
@settermjd

@mmattel
Copy link
Contributor

mmattel commented Apr 20, 2018

@DeepDiver1975 if this gets merged, we will have code added in the meanwhile which does not have the namespace added. How can we make sure we can
a.) identify these elements and
b.) post-change them
I guess that the process of a+b will be needed to rerun from time to time...

@DeepDiver1975
Copy link
Member Author

I guess that the process of a+b will be needed to rerun from time to time...

I did add the php-cs-fixer as drone step - as soon as anybody violates the rules - the build will fail

@DeepDiver1975 DeepDiver1975 force-pushed the code-quality/fully-qualified-function-calls branch from fa32be5 to 1959cc9 Compare April 20, 2018 07:55
@mmattel
Copy link
Contributor

mmattel commented Apr 20, 2018

This is great for new PR´s an thanks for higlighting this as this is a very important doc-note !

But this also means a one time delta job for those PR´s which are not covered by this one but merged in between. May I assume that you volunteer to fix them 😄

@DeepDiver1975
Copy link
Member Author

May I assume that you volunteer to fix them smile

anybody can easily run the php-cs-fixer to not only detect the changes but also to apply them.
we will fix this which in a short period of time

@mmattel
Copy link
Contributor

mmattel commented Apr 20, 2018

Now I see it, php-cs-fixer is in composer.lock

Copy link
Member

@butonic butonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@DeepDiver1975 DeepDiver1975 force-pushed the code-quality/fully-qualified-function-calls branch 3 times, most recently from ca9eeee to f670116 Compare April 20, 2018 12:55
Copy link
Member

@butonic butonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and some more ...

composer.lock Outdated
},
{
"name": "paragonie/random_compat",
"version": "v2.0.12",
"version": "v2.0.11",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DeepDiver1975 downgrade?

composer.lock Outdated
},
{
"name": "phpseclib/phpseclib",
"version": "2.0.11",
"version": "2.0.10",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downgrade?

composer.lock Outdated
},
{
"name": "sabre/vobject",
"version": "4.1.6",
"version": "4.1.5",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downgrade?

composer.lock Outdated
},
{
"name": "symfony/console",
"version": "v3.4.8",
"version": "v3.4.6",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downgrade?

composer.lock Outdated
},
{
"name": "symfony/debug",
"version": "v3.4.8",
"version": "v3.4.6",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downgrade?

composer.lock Outdated
"shasum": ""
},
"require": {
"php": "^5.5.9|>=7.0.8"
},
"conflict": {
"symfony/config": "<3.3.1",
"symfony/config": "<2.8",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downgrade?

composer.lock Outdated
},
{
"name": "symfony/translation",
"version": "v3.4.8",
"version": "v3.4.6",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downgrade?

composer.lock Outdated
},
{
"name": "zendframework/zend-filter",
"version": "2.8.0",
"version": "2.7.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downgrade?

composer.lock Outdated
},
"conflict": {
"zendframework/zend-validator": "<2.10.1"
"php": "^5.5 || ^7.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downgrade?

composer.lock Outdated
"conflict": {
"zendframework/zend-validator": "<2.10.1"
"php": "^5.5 || ^7.0",
"zendframework/zend-stdlib": "^2.7 || ^3.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downgrade?

@butonic butonic force-pushed the code-quality/fully-qualified-function-calls branch from ac94ad1 to 73b02b1 Compare April 20, 2018 19:35
@DeepDiver1975 DeepDiver1975 force-pushed the code-quality/fully-qualified-function-calls branch from cc3f83d to 2537c57 Compare April 20, 2018 22:03
@mmattel
Copy link
Contributor

mmattel commented Apr 21, 2018

@DeepDiver1975 question: what is the impact to other repositories if there is one, lets take as an example oauth2 (could be any other). How can those be improved by this approach ?

@DeepDiver1975 DeepDiver1975 force-pushed the code-quality/fully-qualified-function-calls branch from 2537c57 to f929720 Compare April 21, 2018 09:01
@DeepDiver1975
Copy link
Member Author

@DeepDiver1975 question: what is the impact to other repositories if there is one, lets take as an example oauth2 (could be any other). How can those be improved by this approach ?

we will step by step introduce php-cs-fixer to all other repos as well.

@DeepDiver1975 DeepDiver1975 merged commit f2543f6 into master Apr 21, 2018
@DeepDiver1975 DeepDiver1975 deleted the code-quality/fully-qualified-function-calls branch April 21, 2018 09:54
@patrickjahns
Copy link
Contributor

Backport to stable10 please - keep coding style consistent

@phil-davis
Copy link
Contributor

Yes, and it will minimize pain for future backports.

@patrickjahns
Copy link
Contributor

patrickjahns commented Apr 21, 2018

Before Backporting:

I had a bit of more time to think and check other projects using / implementing it - and I would actually prefer the approach the doctrine project went ahead:

instead of using \ for all global functions (i.e. \ltrim()...) they use function imports

use function ltrim;
ltrim()

ref: doctrine/orm#6807

Example: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Query.php

while this is merged now it doesn't mean we have to stick with it - we could create codesniffer rules: https://github.com/doctrine/coding-standard/pull/15/files

@phil-davis
Copy link
Contributor

Note: backport of this is likely to be superceded by the "ownCloud coding standard" implementation in PR #31442

@phil-davis
Copy link
Contributor

Effective backport is included in #31453

@lock
Copy link

lock bot commented Jul 31, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants