PHP code style check is checking data folder and apps2 and apps3 folders

[phil-davis]

Steps to reproduce

1. install development ownCloud in local git repo with the data folder in data of the local repo
2. as some user, upload some *.php code with crap style
3. make test-php-style

Expected behaviour

Style checks pass

Actual behaviour

Style check fails on the file(s) that are actually just test user data, not real ownCloud code

Server configuration

Local development environment.

[ownclouders]

GitMate.io thinks possibly related issues are #781 (php-ftp check), #24025 (OCI.php wrong Oracle privileges check), #19036 (Code Integrity Check), #29340 (Code Integrity check), and #29339 (Code Integrity Check).

[phil-davis]

If you put stuff in folders like apps2 or apps3 etc, then that gets scanned for code style also. We should exclude folders like that.

[phil-davis]

PR #31866 excludes the data folder.

[phil-davis]

PR #31868 excludes any apps* folders.

[PVince81]

closing as #31868 (review) was closed

I tried excluding the folders locally but I'm still getting a forest of "F"

[phil-davis]

PR #31868 was rejected. In that PR I was trying to make the excluded folders for the PHP code style checks match more closely to what is excluded in .gitignore It seems to me that if we provide a repo-wide .gitignore which excludes apps* (apps2 apps3 ...) from being considered for inclusion in commits, then it is logical to have a similar exclusion from the PHP code style and any other checks/parsing of "real code".

[PVince81]

I'm reopening this because even with .php_cs it doesn't work for me.

I tried dumping the array:

lib/composer/bin/php-cs-fixer fix -v --diff --diff-format udiff --dry-run --allow-risky yes
array(12) {
  [0]=>
  string(12) "lib/composer"
  [1]=>
  string(5) "build"
  [2]=>
  string(28) "apps/files_external/3rdparty"
  [3]=>
  string(6) "config"
  [4]=>
  string(4) "data"
  [5]=>
  string(5) "apps2"
  [6]=>
  string(5) "apps3"
  [7]=>
  string(16) "apps/objectstore"
  [8]=>
  string(18) "apps/theme-thunder"
  [9]=>
  string(15) "apps/theme-fire"
  [10]=>
  string(17) "apps/multidirtest"
  [11]=>
  string(13) "apps/contacts"
}
Loaded config ownCloud coding standard from "/srv/www/htdocs/owncloud/.php_cs".
FFFF.......................................................................................................................................................................................................................................................................................................................................................^Cmake: *** [Makefile:194: test-php-style] Interrupt

Since I run into an out of memory error even with 2GB, I cannot see where the errors are pointing to so will need to do a trial and error session to find out.

Maybe we should rather use inclusion instead of exclusion ?

This is a blocker for using git hooks #31837

[patrickjahns]

Can you provide your .php_cs file ?

git hooks are optional and up to each developer to use them - (we can't enforce them)

edit - my version of .php_cs - I have two extra golder apps-source and pjtmp

<?php

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

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

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

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)
    ->in(__DIR__);

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

works like a charm for me - btw we should add .php_cs to .gitignore

[PVince81]

I've gotten further now after deleting a stray "3rdparty" and "build/integration" folder. The former is from when switching from stable9.1 and the latter was likely already excluded.

Now I get some test results pointing at the files "tests/autoconfig-*.php". These were likely generated by test runs from "build/autotest.sh" as these files are not checked in.

I can submit a PR to exclude these files if needed

[patrickjahns]

I am fine with excluding the autoconfig-*.php files in the .php_cs.dist.

Just trying to separate what are core repo files generally - and what each developer does to the repo manually - the manual stuff (apps2/apps3/pjtmp) should be in the local override file

[PVince81]

Adding $excludedDirs[] = 'tests/autoconfig*'; doesn't make it exclude those files.

The alternative would be to have the Makefile rule of "test-php-style" automatically delete these files instead. I've deleted them now and it looks fine.

[PVince81]

found this... PHP-CS-Fixer/PHP-CS-Fixer#1767

will try

[PVince81]

PR here #31925

[patrickjahns]

found this... PHP-CS-Fixer/PHP-CS-Fixer#1767

cs-fixer uses https://symfony.com/components/Finder