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

Add PSR12 ruleset #4943

Merged
merged 1 commit into from
Jan 18, 2021
Merged

Add PSR12 ruleset #4943

merged 1 commit into from
Jan 18, 2021

Conversation

julienfalque
Copy link
Member

Ref: #4502.

@drupol
Copy link
Contributor

drupol commented May 6, 2020

Can't wait for this :)

@drupol
Copy link
Contributor

drupol commented May 7, 2020

Just FYI,

On my side, I'm using a package of mine implementing this ruleset among some others.

This is completely in sync with PHPCS and ECS.

The configuration for PSR12 is here, maybe that will help.

The scripts i'm using in composer.json:

  "scripts": {
    "phpcsfixer": "vendor/bin/php-cs-fixer  --dry-run --allow-risky=yes --config=./vendor/drupol/php-conventions/config/psr12/php_cs_fixer.config.php --using-cache=no --verbose fix",
  }

@julienfalque
Copy link
Member Author

Thanks @drupol, I updated the ruleset :)

@drupol
Copy link
Contributor

drupol commented May 8, 2020

Great, glad it helped :)

@drupol
Copy link
Contributor

drupol commented May 8, 2020

Weird issue.

There was 1 failure:

1) PhpCsFixer\Tests\RuleSetTest::testDuplicateRuleConfigurationInSetDefinitions

"@PSR12" defines rules the same as it extends from:
- "visibility_required" is also in "@PSR2"

"@Symfony" defines rules the same as it extends from:
- "visibility_required" is also in "@PSR12"

/home/travis/build/FriendsOfPHP/PHP-CS-Fixer/tests/RuleSetTest.php:639

Overriding rules is not allowed ?!

@julienfalque
Copy link
Member Author

Our tests prevent having a ruleset that overrides a rule with the exact same config.

@julienfalque
Copy link
Member Author

@nicolas-grekas @fabpot This introduces changes in the @Symfony ruleset, are you ok with those?

@nicolas-grekas
Copy link

I ran this PR on the Symfony, there are some suspicious changes, but looks good otherwise.

Here are the changes that I think should not happen:

image

image

image

@drupol
Copy link
Contributor

drupol commented May 8, 2020

Hi,

Just for the record, here are the rules (and the ruleset that uses them) from your screenshots.

image

ordered_imports [@Symfony, @PhpCsFixer]

image

full_opening_tag [@PSR1, @PSR2, @Symfony, @PhpCsFixer]

image

global_namespace_import

@julienfalque
Copy link
Member Author

@nicolas-grekas Having trait imports at the very beginning of classes is required by PSR-12, do you mean you don't want Symfony's coding style to follow that specific rule?

The second change looks like a bug unrelated to this PR, <?= is supposed to be allowed. In which file was that change introduced?

The last change looks unrelated to this PR, global_namespace_import isn't part of any ruleset.

@fabpot
Copy link
Contributor

fabpot commented May 8, 2020

Having trait imports at the beginning of the classes looks ok to me.

@kubawerlos
Copy link
Contributor

@nicolas-grekas @julienfalque last change is related to "strict" flag for NativeConstantInvocationFixer which will be released in 2.17.

@nicolas-grekas
Copy link

last change is related to "strict" flag for NativeConstantInvocationFixer which will be released in 2.17.

OK, then that's something that NativeConstantInvocationFixer should not do for some selected constants. PHP_INT_SIZE is one of them: while for most other constants we don't care, this one, but also e.g. PHP_VERSION_ID, is used by PHP at compile time to optimize the compiled opcodes. Removing the \ breaks the optimization.

I think looking at which constants have a leading \ in the codebase could give the list. PHP_DIRECTORY_SEPARATOR is one of them.

@kubawerlos
Copy link
Contributor

Like these https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/v2.16.3/src/RuleSet.php#L193?

The only update in 2.17 would be making it strict.

@nicolas-grekas
Copy link

Yes, this. PHP_INT_SIZE should be added to the list.

SpacePossum added a commit that referenced this pull request May 14, 2020
This PR was merged into the 2.15 branch.

Discussion
----------

Allow "const" option on PHP <7.1

This allows passing `const` to option `elements` in rulesets without runtime check like in #4943. It's also consistent with the fact that most fixers don't throw exceptions when passed allowed values that don't make sense (e.g. passing an empty array to that option).

Commits
-------

1895c2a Allow "const" option on PHP <7.1
SpacePossum added a commit that referenced this pull request May 23, 2020
…e set (kubawerlos)

This PR was merged into the 2.17-dev branch.

Discussion
----------

NativeConstantInvocation - Add "PHP_INT_SIZE" to SF rule set

As [requested](#4943 (comment)) by @nicolas-grekas.

Added as a feature because it is [change](#3127 (comment)) request.

Commits
-------

fd92a7e Add "PHP_INT_SIZE" to "native_constant_invocation" in Symfony rule set configuration
src/RuleSet.php Outdated Show resolved Hide resolved
This was referenced Mar 11, 2021
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

10 participants