PHP-CS-Fixer: style fixer for class is not backward-compatible

Bug report

PHP version: 7.4.16
PHP CS Fixer version: https://github.com/FriendsOfPHP/PHP-CS-Fixer/releases/tag/v2.18.4
the command used to run PHP CS Fixer (run with -vvv)

php -d zend.enable_gc=0 vendor-bin/owncloud-codestyle/vendor/bin/php-cs-fixer fix -v --diff --diff-format udiff --allow-risky yes --dry-run

<?php

$dirToParse = 'apps';
$dirIterator = new DirectoryIterator(__DIR__ . '/' . $dirToParse);

$bundledApps = [
	'comments',
	'dav',
	'encryption',
	'federatedfilesharing',
	'federation',
	'files',
	'files_external',
	'files_sharing',
	'files_trashbin',
	'files_versions',
	'provisioning_api',
	'systemtags',
	'updatenotification'
];

$excludeDirs = [
	'lib/composer',
	'build',
	'apps/files_external/3rdparty',
	'apps-external',
	'data',
	'3rdparty',
];

foreach ($dirIterator as $fileinfo) {
	$filename = $fileinfo->getFilename();
	if ($fileinfo->isDir() && !$fileinfo->isDot() && !in_array($filename, $bundledApps)) {
		$excludeDirs[] = $dirToParse . '/' . $filename;
	}
}

$finder = PhpCsFixer\Finder::create()
	->exclude($excludeDirs)
	->notPath('config/config.php')
	->notPath('config/config.backup.php')
	->notPath('tests/autoconfig*')
	->in(__DIR__);

$config = new OC\CodingStandard\Config();
$config->setFinder($finder);
return $config;

Code snippet that reproduces the problem

PR https://github.com/owncloud/core/pull/38560

The style check against current owncloud/core master passes with PHP-CS-Fixer 2.18.3.

But with no change to any settings, it fails with PHP-CS-Fixer 2.18.4.

It now wants to put the { on a new line after a class statement. That causes us to need to change 2144 files!

About this issue

Original URL
State: closed
Created 3 years ago
Reactions: 1
Comments: 16 (10 by maintainers)

Most upvoted comments

@phil-davis @prisis it’s already fixed with https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/5567, I’ve extended the priority test to explixitly cover scenario from this issue.

kubawerlos on Mar 25, 2021

Oups. 🙈 So well, then @keradus, are you planning for a release to fix that bug…? 😉 🙈

jotoeri on Apr 1, 2021

Yes, any chance to release 2.18.5 with the current code that is already merged?

phil-davis on Apr 1, 2021

@kubawerlos Are you planning to release a patch on this?

jotoeri on Mar 31, 2021

can you try to edit this file?

For any 0 or positive value returned by getPriority the “problem/feature” remains (e.g. 0, 5, 15, 25, 42, 99, 111). The class_definition rule puts the { on a new line.

For a negative value returned by getPriority the “problem/feature” does not happen (e.g. -1, -25).

I suspect that for negative priority values, the related fixer does not run at all. But looking at the source code, the priority value is used for sorting the order of fixers. I don’t see how the sorting can completely “lose” a fixer from the array of fixers to apply.

IMO for me the change in 2.18.4 is actually a fix. The class_definition rule now runs effectively. And actually my code does not meet the rule, and the changes it suggests are the correct ones. The fix is not obvious in the release notes - perhaps the fix is somewhat “accidental” as a result of those priority numbers changing? And that is why I raised this issue - from the release notes I was not expecting a fix like this.

@prisis you are able to cause a RuntimeException. I suspect that that will be some problem in a fixer that is now getting run (and maybe previously never even ran?) It might be best for you to create a separate issue that has the rule set that you are using.

phil-davis on Mar 24, 2021

With a config like this:

<?php
/**
 * @author Patrick Jahns <github@patrickjahns.de>
 * @author Thomas Müller <thomas.mueller@tmit.eu>
 *
 * @copyright Copyright (c) 2018, ownCloud GmbH
 * @license AGPL-3.0
 *
 * This code is free software: you can redistribute it and/or modify
 * it under the terms of the GNU Affero General Public License, version 3,
 * as published by the Free Software Foundation.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 * GNU Affero General Public License for more details.
 *
 * You should have received a copy of the GNU Affero General Public License, version 3,
 * along with this program.  If not, see <http://www.gnu.org/licenses/>
 *
 */

namespace OC\CodingStandard;

use PhpCsFixer\Config as BaseConfig;

class Config extends BaseConfig {
	public function __construct($name = 'default') {
		parent::__construct('ownCloud coding standard');
		$this->setIndent("\t");
	}

	public function getRules() {
		$rules = [
			'@PSR1' => true,
			'class_definition' => true,
        ];
		return $rules;
	}
}

And code:

<?php
class Controller {
}

Run the fixer (v2.18.4):

php vendor-bin/owncloud-codestyle/vendor/bin/php-cs-fixer fix --diff --dry-run settings/ChangePassword/Controller.php
Loaded config ownCloud coding standard from "/home/phil/git/owncloud/core/.php_cs.dist".
Using cache file ".php_cs.cache".
Paths from configuration file have been overridden by paths provided as command arguments.
   1) settings/ChangePassword/Controller.php
      ---------- begin diff ----------
--- Original
+++ New
@@ @@
 <?php
-class Controller {
+class Controller
+{
 }
 

      ----------- end diff -----------


Checked all files in 0.004 seconds, 12.000 MB memory used

It wants the { to be on a new line.

If I run the same with 2.18.3 it passes (the { on the same line is fine)

https://cs.symfony.com/doc/rules/class_notation/class_definition.html indicates that the class_definition rule always wants the { to be on a new line. That is the standard set by PSR-2 and PSR-12. So maybe the problem is that the class_definition rule was not effective in 2.18.3 and now it is effective. If that is the case, then I guess 2.18.4 has a bug fix, which causes our non-conforming code to need >2000 files fixed. And I need to decide to either remove the class_definition fixer, or to let it implement the >2000 fixes.

phil-davis on Mar 23, 2021