Add a "getConfig" method to make it easier to extends

[BafS]

Currently it's pretty difficult to run PHP code sniffer programmatically, often it's about changing the configuration but unfortunately there is no dependency injection, this PR makes it a bit easier to extends the config without BC.

[BafS]

It would be great to use dependency injection for v4 to make it easier to work with :)

[BafS]

Could anyone help me merging this PR @gsherwood ?

[BafS]

@gsherwood any chance? Is there someone else I could ping? Could we add more contributor to the project potentially?

[BafS]

@gsherwood it would be great to have some feedback or more contributors to the project, do you have any plan?

[BafS]

@gsherwood could you help me to merge it?

[jrfnl]

@BafS As far as I can see, the problem with this PR is that the use-case is unclear. What are you trying to do ? What is blocking you from doing that ? And how does this PR solve your problem ?

I mean, from a PHPCS perspective, there is absolutely no reason to accept this PR as it doesn't solve anything for PHPCS itself.

[BafS]

I was trying to use a custom configuration. For the moment it's hardcoded directly so there is no way to change this programmatically. The best would be to use dependency inversion but it would be more complex and potentially a BC. Instead this is a solution where we can extends the class to override the default configuration.

[jrfnl]

Ok... but why ? I mean, I understand the literal "what", but I still don't get the bigger picture "what".

[BafS]

Because we have a tool to get the code quality from a web interface and it doesn't use the default options. For example we want the colors and the report in JSON to be able to show it nicely in the UI.

The workaround is to do something like that

        $_SERVER['argv'] = [
            '-i',
            '--standard=Proton',
            '--colors',
            '--extensions=php',
            '--report=json',
            $this->folder,
        ];

        $runner = new Runner();

        ob_start();
        $runner->runPHPCS();
        $data = json_decode(ob_get_clean());

[jrfnl]

Because we have a tool to get the code quality from a web interface and it doesn't use the default options. For example we want the colors and the report in JSON to be able to show it nicely in the UI.

The workaround is to do something like that

These options can also (nearly) all be set in a CodeSniffer.conf file, which would make the default config exactly what you want:

<?php
$phpCodeSnifferConfig = array (
  'default_standard' => 'Proton',
  'report_format' => 'json',
  'colors' => '1',
);

They can also all be set in a phpcs.xml.dist file or even in a custom standard (Proton-Web, which could then be set as the 'default_standard').

I'm not trying to be difficult, but I'm just still not sure why the currently available options would not be sufficient.

Keep in mind: allowing the Config to be overloaded like this, would mean that the PHPCS application and external standards would now run the risk of throwing errors for incorrectly overloaded Config objects as we can no longer be sure that the input validation in the Config file will be executed. While, obviously, this is the risk of the extender, bug reports would still likely be filed in this repo and will consume time and effort from volunteers here

[BafS]

Thanks for your answer, but where do you pass this array? In the example I used fixed values but it could be dynamic too (from $_GET for example). If there is a way to pass the array in your example in the runner then we don't need to extends it but I don't see it (https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Runner.php#L69).

Regarding the extending argument, you should use final classes if you want to control more what devs can do or not.

Also I'm OK to do another merge request where we inject the Config in the constructor of Runner (which is how it should be to have dependency injection) and we set it null by default to not have any BC.

[BafS]

I did another PR to use injection: https://github.com/squizlabs/PHP_CodeSniffer/pull/3882

[jrfnl]

but where do you pass this array?

@BafS You don't. As I said above, you save it as a CodeSniffer.conf file (in the PHPCS directory). The PHPCS Config class automatically looks for it and will use the values from that array.

If there is a way to pass the array in your example in the runner then we don't need to extends it but I don't see it

Of course there is, but I presumed you'd actually looked at the code in detail before opening a PR. I suppose I was wrong.

Here's (yet) another way to do it: https://gist.github.com/gsherwood/aafd2c16631a8a872f0c4a23916962ac

Regarding the extending argument, you should use final classes if you want to control more what devs can do or not.

If it were up to me, the codebase would use more final classes, but this is an old code base from well before the final keyword was available in PHP. Introducing it now is a BC break, which needs careful consideration.

Also I'm OK to do another merge request where we inject the Config in the constructor of Runner (which is how it should be to have dependency injection) and we set it null by default to not have any BC.

To be honest, I'm inclined to close both PRs. This discussion should be in an issue, not in PRs.

[BafS]

@BafS You don't. As I said above, you save it as a CodeSniffer.conf file (in the PHPCS directory). The PHPCS Config class automatically looks for it and will use the values from that array.

But this is super hacky and not explicit. On top of that you cannot pass a variable from the business logic down there so you need to write the file.

Also this file doesn't return anything (like return [...]), it works because of variable name which is confusing.

Of course there is, but I presumed you'd actually looked at the code in detail before opening a PR. I suppose I was wrong.

Here's (yet) another way to do it: https://gist.github.com/gsherwood/aafd2c16631a8a872f0c4a23916962ac

I did look but the runner has 2 factories, in this snippet you don't have the full logic from the runPHPCS method, but sure it is a way. Unfortunately nothing is injected and public fields are used, which is not a good encapsulation. That's why injecting the config would be a better option and like that we can still use run* methods. It's possible to do it without any BC. It would make it easier to use the library programmatically.

If it were up to me, the codebase would use more final classes, but this is an old code base from well before the final keyword was available in PHP. Introducing it now is a BC break, which needs careful consideration.

Why not using the phpdoc @final? Like that the intent is clear and the next major (v4) can actually do it.

To be honest, I'm inclined to close both PRs. This discussion should be in an issue, not in PRs.

I'm just trying to help, there is many improvement possible, should I target v4?

[jrfnl]

there is many improvement possible, should I target v4?

Yes, this is an old code base (started in 2005/6), of course there is lots of things which could be improved, but those shouldn't be done willy-nilly.

Well reasoned proposals for structural improvement are welcome in an issue where they can be discussed and evaluated on their merit and use-case. Please do not send in PRs for those type of things without discussing the proposed change first as I wouldn't want you to waste your time.