Various quality fixes

[meyerbaptiste]

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	n/a

[soyuka]

I'm -1 on this public const thing it's just noise IMO. Symfony is not using it for now?

[meyerbaptiste]

We should now pay attention to the visibility of our constants and define them explicitly. It's by default in the PHP-CS-Fixer ruleset @PHP71Migration.

[teohhanhui]

It's by default in the PHP-CS-Fixer ruleset @PHP71Migration.

But we are already using both @PHP71Migration and @PHP71Migration:risky rulesets?

[teohhanhui]

visibility_required [@PSR2, @Symfony, @PhpCsFixer, @PHP71Migration]

Visibility MUST be declared on all properties and methods; abstract and final MUST be declared before the visibility; static MUST be declared after the visibility.

Configuration options:

- elements (a subset of ['property', 'method', 'const']): the structural elements to fix (PHP >= 7.1 required for const); defaults to ['property', 'method']

So it's not enabled by default. If we're going to make this change, we must change that option.

[soyuka]

By default constants are public and it won't change, we should define them if they're not public IMO.

https://secure.php.net/manual/en/migration71.new-features.php#migration71.new-features.class-constant-visibility

[meyerbaptiste]

But we are already using both @PHP71Migration and @PHP71Migration:risky rulesets?

Yep but because of the alphabetical order in our configuration, the @Symfony ruleset, that uses the @PSR2 ruleset, has the advantage and uses the default configuration: method + property vs. method + property + const for the @PHP71Migration ruleset.

• https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/2.14/src/RuleSet.php#L57
• https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/2.14/src/RuleSet.php#L309-L315

[meyerbaptiste]

By default constants are public and it won't change

By default methods are public but you define a visibility explicitly even if it is public.

[teohhanhui]

Okay, let's add the correct configuration for this rule then.

[teohhanhui]

Yep but because of the alphabetical order in our configuration, the @Symfony ruleset has the advantage

That is indeed unexpected, but might as well since I don't think we would ever find a "correct" order. 😆

[meyerbaptiste]

I just updated the .php_cs.dist file.

[teohhanhui]

Anyway, I suspect this PR should target 2.4 first (you could open a separate PR for master after this one is merged into master, as needed), to reduce future merge conflicts.

[meyerbaptiste]

Base branch changed.

[dunglas]

Thanks @meyerbaptiste