Add PHPCS and PHPCBF as Composer scripts

I think this is a great idea, it's just one thing less to learn about, and instead you can just use it.

❤️❤️❤️

I really like the general direction!

Without this adaption:

$ composer phpcs modules/menu_link
> phpcs --standard=core/phpcs.xml.dist --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md -- 'modules/menu_link'

FILE: /Users/dawehner/www/d8/modules/menu_link/README.txt
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | WARNING | No PHP code was found in this file and short open tags
   |         | are not allowed by this install of PHP. This file may
   |         | be using short open tags but PHP does not allow them.
----------------------------------------------------------------------

Time: 442ms; Memory: 8Mb

With this adaption:

$ composer phpcs modules/menu_link
> phpcs --standard=core/phpcs.xml.dist -- 'modules/menu_link'

There is similar work in #2902381: Revert phpcs.xml.dist changes from "Ensure that we're using the right ruleset for coding standards checking" - i think we need to borrow from that and add the option --runtime-set installed_paths $(pwd)/../vendor/drupal/coder/coder_sniffer to both commands. This ensures that the correct ruleset will be used.

One problem with this approach is that it assumes the location of vendor :( not sure yet how to get around that.

Here's a fix that addresses #7.

Manually tested, works great. I think this is ready?

That vendor export trick is #$%^ing awesome! I don't know that people do weird things with vendor other than in Drupal otherwise I'd suggest you should make that into a package people can steal. :-D

Another trick to get vendor root.

- $COMPOSER_VENDOR_DIR
+ $($COMPOSER_BINARY config vendor-dir)

@Chi that's not just another way - that's a better way! Thanks. Added @Chi to the issue credit.

mind blown.

Nice!

Love the enthusiasm and collaboration here!

Add -ps to the phpcs command line please, so we're not staring at an unmoving screen for 10 minutes and don't have info about which sniffs are failing.

Doesn't work from contrib, but the history is that it's not supposed to. :-)

That's a good idea!

For those like me who don't know what -ps does:

        -s            Show sniff codes in all reports
…
        -p            Show progress of the run

Makes sense!

phpcbf doesn't have -ps. :-)

#19: that is … unexpected.

@Mile23 what does

Doesn't work from contrib, but the history is that it's not supposed to. :-)

mean?

With the current command I can do composer run-script phpcs ./modules/redirect and it works just fine.

+1 to adding -ps to the phpcs command since you can't add them yourself. You get a [Symfony\Component\Console\Exception\RuntimeException] The "-p" option does not exist..

With the current command I can do composer run-script phpcs ./modules/redirect and it works just fine.

True, but then if the module has a phpcs.xml(.dist) then you're not honoring it. For instance:

$ composer run phpcs modules/examples/
> phpcs -ps --standard=core/phpcs.xml.dist --runtime-set installed_paths $($COMPOSER_BINARY config vendor-dir)/drupal/coder/coder_sniffer -- 'modules/examples/'
.....E......E.........E........................E............  60 / 174 (34%)
E...E.E..E.............E...E......E......E..............EE.W 120 / 174 (69%)
.W.........................S..........S..E............
[etc]

This is fine, as far as I'm concerned, and shouldn't hold up this issue. Having a one-liner that you don't have to think too much about for running phpcs in core is the goal here. Examples (or other contrib) could roll their own phpcs composer command if they wanted.

oops, I guess I should have tried both commands :)

Examples (or other contrib) could roll their own phpcs composer command if they wanted.

Do you know of a page where best practises like that for module authors are documented? This could be really useful/powerful.

Do you know of a page where best practises like that for module authors are documented? This could be really useful/powerful.

More research needed. Looks doubtful, actually. Changing to the module directory and then saying composer run someScript works, but composer config vendor-dir only gives a relative path, so we can't resolve it for core's vendor/ directory.

Contrib authors can say --runtime-set on the command line, just like the testbot.

Everything else looks good.

Can I request we remove the progress and document how you can provide them to the composer script? You can add arguments but you can't take them away.

composer phpcs -- -ps

@neclimdul
What is the usecase of not having process? As far as I understand you could do -q on CI environments.

In my mind these shortcuts are for humans and not CI scripts.

It worth mentioning that these options can be hardcoded in phpcs.xml.dist like follows <arg value="ps"/>.

I agree with @alexpott, we should optimize for the common usecase. You probably configure a CI system way less often, than typing things into your shell.

+1 I agree with @alexpott too !! Great initiative :)

I've to this day never run the progress with the sole exception of testing cases for related issues and don't plan on adding it to my usage since i'm just going to minimize the console and do something else until its done. Its just adding junk to my terminal backlog. So not my common use case and I do wonder if -q breaks the ability to use other tags. It definitely breaks the ability to use -v per the argument documentation.

In any case I'm happy to get this in and unblock other issues and deal with the side effects elsewhere.

Really like this issue and hope #23 can get in soon.
RTBC +1

I discussed this with @xjm and @catch.

We feel that adding the -ps options into this patch is out of scope and there isn't consensus on it per #26. #21 gives a good reason to add it by claiming that it can't be added after the fact on the command line but #26 contradicts that.

Is #26 accurate that there is a reasonable way to add the -ps options yourself on the command line? If so, can we remove them from this patch and have a follow-up issue to discuss adding them?

Addressing #34

Is #26 accurate that there is a reasonable way to add the -ps options yourself on the command line? If so, can we remove them from this patch and have a follow-up issue to discuss adding them?

Yes, composer phpcs -- -ps works like a charm.

One last thing - should we also change process timeout in composer.json (i.e. https://getcomposer.org/doc/06-config.md#process-timeout)? It defaults to 300 seconds which may be not enough to run full core test and results with:

[Symfony\Component\Process\Exception\ProcessTimedOutException]                                                                                                  
  The process "phpcs --standard=core/phpcs.xml.dist --runtime-set installed_paths $($COMPOSER_BINARY config vendor-dir)/drupal/coder/coder_sniffer -- '-ps'" exc  
  eeded the timeout of 300 seconds. 

Asking because I have experienced it today 😃

Maybe it needs mention in README or change record to announce?

#37: +1: done!

I guess composer phpcs -- -ps is better than adding --runtime-set or whatever else.

There's no appropriate readme-esque text in core where we can document this.

It is in the help though which is where I found it.

$ composer phpcs --help
Usage:
  phpcs [options] [--] [<args>]...

Unfortunately the phpcs arguments are still obfuscated so you have to look that up.

I think this is RTBC worthy personally but I don't know if we need to address #36 here.

To be fair, #36 have happened because I had xdebug installed and active. With xdebug off single phpcs run takes only 20% of the time when xdebug is on (~80s rather than ~400 on my machine).

sounds like a docbook entry then. "hey, if you run into this you might want to disable xdebug or adjust the timeout entry in your php.ini"

Thanks. Looks great to me. Ticking credit boxes for reviewers.

Pushed to 8.5.x. If anyone wants to open a followup to add back in the -ps into composer.json or an issue on how to document how each user can do so on the command line, please go for it.

🎉🎶

Why there's no change record? Also it probably need to be added to 8.5 release notes

it probably need to be added to 8.5 release notes

This issue is already tagged "8.5.0 highlights" which I think is the correct tag per #2840392-85: Enable BigPipe by default in the Standard install profile.

Why there's no change record?

A new change record might be helpful to some people. And/or, https://www.drupal.org/node/2839574 could use an update to reflect the new best-practice way to use it for 8.5. Anyone up for doing either or both?

This does not work when composer is installed via homebrew.

tim@Porkbelly:~/www/d8$ which composer
/usr/local/bin/composer
tim@Porkbelly:~/www/d8$ composer phpcs -- -p
> phpcs --standard=core/phpcs.xml.dist --runtime-set installed_paths $($COMPOSER_BINARY config vendor-dir)/drupal/coder/coder_sniffer -- '-p'
sh: /usr/local/Cellar/composer/1.5.2/libexec/composer.phar: Permission denied

Fatal error: Uncaught UnexpectedValueException: DirectoryIterator::__construct(/drupal/coder/coder_sniffer): failed to open dir: No such file or directory in /Users/tim/www/d8/vendor/squizlabs/php_codesniffer/CodeSniffer.php on line 2250

UnexpectedValueException: DirectoryIterator::__construct(/drupal/coder/coder_sniffer): failed to open dir: No such file or directory in /Users/tim/www/d8/vendor/squizlabs/php_codesniffer/CodeSniffer.php on line 2250

Call Stack:
    0.0009     357856   1. {main}() /Users/tim/www/d8/vendor/squizlabs/php_codesniffer/scripts/phpcs:0
    0.0140    2050728   2. PHP_CodeSniffer_CLI->runphpcs() /Users/tim/www/d8/vendor/squizlabs/php_codesniffer/scripts/phpcs:25
    0.0144    2095120   3. PHP_CodeSniffer_CLI->process(???) /Users/tim/www/d8/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php:113
    0.0149    2097744   4. PHP_CodeSniffer_CLI->validateStandard(???) /Users/tim/www/d8/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php:935
    0.0149    2097744   5. PHP_CodeSniffer::getInstalledStandards(???, ???) /Users/tim/www/d8/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php:1176
    0.0152    2101904   6. DirectoryIterator->__construct(???) /Users/tim/www/d8/vendor/squizlabs/php_codesniffer/CodeSniffer.php:2250

Script phpcs --standard=core/phpcs.xml.dist --runtime-set installed_paths $($COMPOSER_BINARY config vendor-dir)/drupal/coder/coder_sniffer -- handling the phpcs event returned with error code 255
tim@Porkbelly:~/www/d8$ 

It fails to set $COMPOSER_BINARY to anything, and tries to work from the /drupal/coder/coder_sniffer directory.

export COMPOSER_BINARY=`which composer` does not help.

Getting a composer.phar into my D8 directory, this does work:
php composer.phar phpcs -- -p

yeah, setting the environment wouldn't help because the variable is still going to be set in php.

Based on the fact that you're executing composer and the error is complaining about composer.phar, it seems the composer binary(composer.phar in this case) is not executable and homebrew is using some sort of wrapper script. That's both kinda odd and kind of curious since composer.phar is its own wrapper. Can you chmod the phar and see what happens? Also should we open a new issue to support homebrew's edge case?