Skip to content

Coding standard violations in protected branches #40196

Closed
@localheinz

Description

@localheinz

Symfony version(s) affected: 4.4.19, 5.2.3

Description

When I check out any branch, e.g.,

  • 4.4
  • 5.2
  • 5.x

and - following https://symfony.com/doc/current/contributing/code/standards.html#making-your-code-follow-the-coding-standards - run

$ php-cs-fixer fix

then all of these branches contain coding standard violations.

How to reproduce

Run

$ git clone git@github.com:symfony/symfony.git
$ composer global require friendsofphp/php-cs-fixer
$ php-cs-fixer fix --dry-run

When opening a pull request, I have also repeatedly observed that fabbot.io complains about coding standard violations that are unrelated to the changes proposed in a pull request. However, fabbot.io also only complains about a subset of the the errors reported by running friendsofphp/php-cs-fixer locally.

To me, that looks like a broken process.

As a contributor, when I check out any of the protected branches and run

$ php-cs-fixer fix --dry-run

I would not expect the process to complete with a non-zero exit code.

Additionally, running

$ php-cs-fixer fix --dry-run

also proposes fixes in some components that should not be fixed, for example, when running it in src/Symfony/Bridge/PhpUnit, references to class names from phpunit/phpunit that use pseudo-class names are adjusted to namespaced class names.

--- Original
+++ New
@@ @@

-if (class_exists(\PHPUnit_Runner_Version::class) && version_compare(\PHPUnit_Runner_Version::id(), '6.0.0', '<')) {
+if (class_exists(\PHPUnit\Runner\Version::class) && version_compare(\PHPUnit\Runner\Version::id(), '6.0.0', '<')) {
@@ @@
 // Detect if we're loaded by an actual run of phpunit
-if (!defined('PHPUNIT_COMPOSER_INSTALL') && !class_exists(\PHPUnit_TextUI_Command::class, false) && !class_exists(\PHPUnit\TextUI\Command::class, false)) {
+if (!defined('PHPUNIT_COMPOSER_INSTALL') && !class_exists(\PHPUnit\TextUI\Command::class, false) && !class_exists(\PHPUnit\TextUI\Command::class, false)) {
     return;
 }

Possible Solution

  • Allow components to declare their own configuration.
  • Give developers simple tools (a Makefile or composer scripts ) to run php-cs-fixer against all components, with their corresponding configurations
  • Run fabbot.io against all code, not only against a subset.
  • Do not merge pull requests where fabbot.io reports coding standard violations.
  • Keep all protected branches in a green state.

Additional context

For examples, see the following pull requests, and when available, additional error messages reported by fabbot.io:

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions