Ability to pass filename on CLI when checking STDIN

[choma]

I'm not sure if this is a duplicate of #873, so sorry if I make noise.

I'm using emacs flycheck plugin to sniff code with Cakephp codesniffer.
The reports with this plugin were different from what I had in the shell. I noted that the plugin uses < in the command. Then I checked in the CLI, and using file path like this:

phpcs --standard=path/to/cakephp-codesniffer/CakePHP path/to/SomeClass.php

is different from using STDIN like this:

phpcs --standard=/path/to/cakephp-codesniffer/CakePHP < /path/to/SomeClass.php
# or this:
cat /path/to/SomeClass.php | phpcs --standard=/path/to/cakephp-codesniffer/CakePHP

As an example take this file:

<?php
namespace App\Controller\Admin;

/**
 * Users Controller
 *
 * @property \App\Model\Table\UsersTable $Users
 */
class UsersController extends AppController
{

    /**
     * @return void Redirects.
     */
    public function login()
    {
        return $this->something();
    }

    /**
     * @return boolean.
     */
    protected function _setCookie()
    {
        return true;
    }

    /**
     * @return void.
     */
    public function forgotPassword()
    {
        return $this->anything();
    }
}

If use file path, it will report (using -s option):

FILE: ...ww/fecen_v2/webroot/src/Controller/Admin/UsersControllerTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 13 | WARNING | Function return type is void, but function contains
    |         | return statement
    |         | (CakePHP.Commenting.FunctionComment.InvalidReturnVoid)
----------------------------------------------------------------------

Time: 66ms; Memory: 5.5Mb

If I use STDIN, it will report:

FILE: STDIN
----------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
 13 | WARNING | Function return type is void, but function contains
    |         | return statement
    |         | (CakePHP.Commenting.FunctionComment.InvalidReturnVoid)
 23 | ERROR   | Protected method name "UsersController::_setCookie" must
    |         | not be prefixed with an underscore
    |         | (Squiz.NamingConventions.ValidFunctionName.PublicUnderscore)
 23 | ERROR   | Protected method name "UsersController::_setCookie" must
    |         | not be prefixed with an underscore
    |         | (PEAR.NamingConventions.ValidFunctionName.PublicUnderscore)
----------------------------------------------------------------------

Time: 47ms; Memory: 5.75Mb

Is it the intended behaviour?
Is there a way to use only Cakephp rules when using STDIN?

My system info:

phpcs --version
# PHP_CodeSniffer version 2.5.1 (stable) by Squiz (http://www.squiz.net)

php -v
# PHP 5.6.19-1 (cli) 
# Copyright (c) 1997-2016 The PHP Group
# Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies
#    with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2016, by Zend Technologies

Thanks in advanced!

[gsherwood]

All those methods work exactly the same. I think you're being confused by the CakePHP standard itself.

Check here: https://github.com/cakephp/cakephp-codesniffer/blob/master/CakePHP/ruleset.xml#L102
and here: https://github.com/cakephp/cakephp-codesniffer/blob/master/CakePHP/ruleset.xml#L110

You can see that the CakePHP standard does include those ValidFunctionName sniffs from the Squiz and PEAR standards. So the CakePHP standard is absolutely being run in all your test cases.

But also note that both those sniffs are excluded if the file exists in src/ or tests/. When you're using STDIN, PHPCS has no idea where the file is located. When you pass the file path in, it can apply those exclusion rules. In your case, it excluded those two sniffs because your file path contained src/.

In a future PHPCS version (3.0, still unreleased) you'll be able to get filtering working correctly as long as your editor plugin is telling PHPCS where the file actually exists. There is more info on the feature request here: #733

Does it make sense why using STDIN is showing different results now?

[choma]

Thanks!!
That was perfectly explained. I'll use this issue to suggest flycheck developers to stop using STDIN for this checker, at least until 3.0 be released.

Thank you again for your work!

[swsnr]

@gsherwood Speaking on behalf of Flycheck, we'd prefer if we could keep standard input for phpcs, as it makes many things simpler. It'd be great if we had a way to give phpcs a filename it should assume for standard input. Many other popular linters (rubocop, eslint, etc.) provide a command line option for this purpose.

[choma]

As I understand from this issue, and what I can understand from this code, you have to add phpcs_input_file: [file path] as the first line of content.
I've just tested it and it works great!

[gsherwood]

@lunaryorn Support for STDIN isn't being removed. It's actually being strengthened in the next releases. PHPCS has (since version 2.0) a way to tell PHPCS what the path of a file is when you pass STDIN, which is the method @choma mentioned.

The problem being reported in this issue is that the ruleset itself does not use this file path to exclude sniffs from being checked. This is something I have rectified in the 3.0 branch, but not something I can backport to 2.x.

So I recommend continuing to use STDIN for checking file contents in editors and plugins as this allows for unsaved content to also be checked. But I do recommend adding the phpcs_input_file line before the content so that PHPCS (and any custom sniffs) know what the file path is.

[swsnr]

@gsherwood I was referring to standard input support for phpcs in Flycheck. I'm sorry, my comment was ambiguous.

Anyway, I'm not too happy about the proposed solution. Of all the 80 or so tools that we support in Flycheck phpcs is the only one with this peculiar interface. All others take file names as options.

Hence we've no infrastructure in Flycheck to send arbitrary dynamic text over standard input before the actual contents, and I hesitate to add it for a single syntax checker.

I'd be awesome if phpcs could also provide a command line option to set the file name.

[gsherwood]

I'd be awesome if phpcs could also provide a command line option to set the file name.

I can certainly do that. Was just pointing out that the feature has been available for a couple of years and I'm not planing on removing support for such a feature. Adding to it is certainly something I would do.

I'll reopen this as a feature request to pass a filename as an argument, which is only used when checking STDIN.

[swsnr]

I can certainly do that.

That'd really be awesome 😍 Thank you very much!

[gsherwood]

This is done now and will be available from 2.6.0. Example usage:
cat temp.php | phpcs --standard=PSR2 --stdin-path=/var/www/project/index.php