search

scrutinizer-ci / scrutinizer

Legacy repository - archives past feature requests/bug reports
https://scrutinizer-ci.com/docs
140 stars 36 forks source link

Global path overrides ones in PHP_CodeSniffer #201

Open aik099 opened 10 years ago

aik099 commented 10 years ago

I've set (in the ruleset.xml of used PHP_CodeSniffer standard) that some sniffs needs to be excluded based on filename. It appears though that the way how PHP_CodeSniffer is invoked on the Scrutinizer CI servers overwrites my exclusion and apples all PHPCS standard rules to every file.

Inspection: https://scrutinizer-ci.com/g/aik099/qa-tools/inspections/77dd8870-089a-4c79-ab1d-2a26d91d534b

aik099 commented 10 years ago

Running scrutinizer.phar run . locally does the same. But when I invoke PHP_CodeSniffer manually, e.g. phpcs -v --standard=../CodingStandard/CodingStandard . then all works just fine.

I've set exclude-pattern in the standard to allow some standards to be ignored in some files, but not to ignore some files completely.

Is there any way to see which phpcs command is executed by Scrutinizer?

aik099 commented 10 years ago

This method https://github.com/scrutinizer-ci/scrutinizer/blob/master/src/Scrutinizer/Analyzer/Php/CsAnalyzer.php#L1214 seems to be responsible for path building and it appears that phpcs command is invoked on per-file basis doesn't let PHP_CodeSniffer decide which files to process or not.

I've tried locally and even with a filename given directly the phpcs works correctly, but scrutinizer.phar isn't.

aik099 commented 10 years ago

@schmittjoh, any ideas?

schmittjoh commented 10 years ago

Ah yeah, we do not pass the filename to PHP Code Sniffer, but just the contents of the file.

aik099 commented 10 years ago

Why so? Isn't easier to give filename to code sniffer? So you create temp file with content to test? Still don't see why this is needed.

aik099 commented 10 years ago

Any estimates on this? I'm planning to simplify test suite in my library by removing PHPDocs from it, but currently that will result in hundreds of errors in Scrutinizer CI side, which of course isn't a desired result.

aik099 commented 10 years ago

Ping.

aik099 commented 10 years ago

After talking with @schmittjoh I've found out, that a temporary file is given to PHPCS instead of a real file from a git checkout for a reason. The content of a file might be changed by other tools on the Scrutinizer before and the analyzers should use new file content.

I don't know why the original file from a git checkout isn't changed directly, but only through a File object given to the Analyzer.

I will try to arrange a PR to the legacy branch with a fix. A fix is basically replication of original file name & path under them /tmp folder sub-folder to make PHPCS happy.

aik099 commented 10 years ago

PR created, ready for review.

aik099 commented 9 years ago

@schmittjoh log of https://scrutinizer-ci.com/g/qa-tools/qa-tools/inspections/c85fa3a0-f57f-489e-8544-e2e84a05ad4e sniff confirms, that my code changes are not present on live because executed command is:

/usr/share/scrutinizer/src/Scrutinizer/Analyzer/Php/../../../../vendor/bin/phpcs --standard='../CodingStandard/CodingStandard' --tab-width=0 --encoding=utf8 --report-checkstyle='/tmp/phpcsva0X6E' '/tmp/phpcs8Aw0Ko'

but /tmp/phpcs8Aw0Ko should have name and sub-path of file, that is originally checked.