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

[CS] Implement new php-cs-fixer rules and related code cleanup #1040

Merged
merged 2 commits into from Feb 18, 2018
Merged

[CS] Implement new php-cs-fixer rules and related code cleanup #1040

merged 2 commits into from Feb 18, 2018

Conversation

sebastianblum
Copy link
Contributor

@sebastianblum sebastianblum commented Jan 20, 2018

Q A
Branch? 2.0
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? -
Fixed tickets -
License MIT
Doc PR -

While fixing the tests in #1035 I saw a lot of wrong namespaces. Trying to run php-cs-fixer returns an error, so I updated the php-cs-fixer configuration.

The main rules are coming directly vom symfony/demo, https://github.com/symfony/demo/blob/master/.php_cs.dist

The question is, what merge strategy from 1.x to 2.x will be chosen and which rules make sense.

After the have defined the rules, I will add the changes to the php files and try to fix the scrutiniser checks.

@sebastianblum
Copy link
Contributor Author

sebastianblum commented Jan 20, 2018

we should discuss the use of native_function_invocation to speed up the project. Since we require php 7.1, I would enable this.

@lsmith77 lsmith77 added this to the 2.0.0 milestone Jan 20, 2018
@robfrawley robfrawley added State: Reviewing This item is being reviewed to determine if it should be accepted. Type: Configuration This item pertains to configuration of this project. labels Jan 21, 2018
@sebastianblum
Copy link
Contributor Author

I see a lot commits adding the backslash for performance reasons: symfony/symfony#25854

@robfrawley
Copy link
Collaborator

My vote for native_function_invocation is 👎 as it reduces readability for the sake of (negligible) speed.

@dbu
Copy link
Member

dbu commented Jan 30, 2018

php-cs-fixer is not currently installed in the 2.0 branch. i add it back in #1045

should we run the php-cs-fixer in travis-ci? if not, we won't see whether the configuration for styleci and php-cs-fixer conflict...

@sebastianblum
Copy link
Contributor Author

@dbu I will add it after the new build matrix is merged, see #1035

We can add it in require-dev in composer.json and remove it for php 7.3 / nightly

@robfrawley robfrawley changed the title [2.0] new php-cs-fixer configuration [CS] Implement explicit php-cs-fixer configuration Jan 31, 2018
@sebastianblum
Copy link
Contributor Author

PHP-CS-Fixer is now integrated in travis-ci

here are the results https://travis-ci.org/liip/LiipImagineBundle/jobs/336491426

@dbu
Copy link
Member

dbu commented Feb 5, 2018

interesting. i think we miss a couple styleci rules, notably yoda. in the end, if php-cs-fixer is slightly stricter than styleci, i think its acceptable, but those things we can we should also put into styleci so that automatic fixing is available. or we drop styleci completely and have only fixing by the user in their cli as an option.

@robfrawley
Copy link
Collaborator

@dbu This is a new set of styles (compared to our old rule set this one adds a number of additional fixers), so there are some drastic changes this will apply outside the current styleci rules, but in the past, the two services have been aligned. Moving forward, I agree we need to update styleci to reflect this new ruleset, assuming everyone signs off on these changes. I don't think the solution is to remove styleci completely, as it's great to have the badge on our readme to show our project complies with respectable code style standards at first glance.

@sebastianblum You should add the yoda rule, assuming phpcsfixer has a compatible fixer (I believe it does).

@sebastianblum
Copy link
Contributor Author

sebastianblum commented Feb 5, 2018

@robfrawley Yoda style is included in @symfony already.

I created this pull request to create the php-cs-fixer configuration and if @robfrawley and @dbu agree, I will create a new pull request optimizing the code.
In my plan we do these steps:

  1. define rule set - [CS] Implement new php-cs-fixer rules and related code cleanup #1040
  2. optimize code - [CS] Changes to support new php-cs-fixer rule set #1046
  3. apply coding style - TODO
  4. activate php-cs-fixer in travis configuration - [CI] Add php-cs-fixer check to Travis builds #1049

But we have to discuss how to merge new features from 1.0 to 2.0. Do we have a timeline for supporting 1.0?

@robfrawley
Copy link
Collaborator

robfrawley commented Feb 5, 2018

@sebastianblum About a week ago @lsmith77 and I spoke briefly about support for 1.0 and my understanding is that 1.0 will go into bug-fix only mode the moment the first 2.0 release is tagged. Before we can get this PR merged, can you separate the php-cs-fixer Travis addition into its own PR?

Your plan outline looks good to me.

@sebastianblum
Copy link
Contributor Author

Good, I created #1049 for the future. Should we do the style fixing in this pull request or in a new one?

@dbu
Copy link
Member

dbu commented Feb 5, 2018

the final call is up to you @sebastianblum and @robfrawley but the style rules seem all sensible to me.

if you add the rules in version 1 too, its probably easier to merge 1.0 to 2.0 when you want to merge bugfixes upstream.

@robfrawley
Copy link
Collaborator

I'll handle merging this, running the new styles, and updating styleci in one at the same time. I think your work is done! I'll get this done shortly.

@robfrawley
Copy link
Collaborator

@dbu Good call about applying them to 1.0 to avoid merge conflicts. Will handle that too then.

@robfrawley robfrawley self-assigned this Feb 10, 2018
@robfrawley robfrawley added the Attn: Minor This issue or PR is a minor problem or minor change. label Feb 10, 2018
Sebastian Blum and others added 2 commits February 18, 2018 03:35
- created new rule set as discussed by community
- removed broken php-cs-fixer-styleci-bridge usage
- add additional rules and sort all alphabetically
- fix code broken by new rules
- cleanup .php_cs.dist file and align with cs standards
- add .php_cs.cache to .gitignore
@robfrawley robfrawley changed the title [CS] Implement explicit php-cs-fixer configuration [CS] Implement new php-cs-fixer rules and related code cleanup Feb 18, 2018
@robfrawley robfrawley merged commit ef42b7a into liip:2.0 Feb 18, 2018
@sebastianblum sebastianblum deleted the php-cs-fixer branch February 18, 2018 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Attn: Minor This issue or PR is a minor problem or minor change. State: Reviewing This item is being reviewed to determine if it should be accepted. Type: Configuration This item pertains to configuration of this project.
Projects
2.x Sprint 001
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

4 participants