PHPStan setup

[uuf6429]

Based on #277.

This PR sets up PHPStan without any fixes (hence the large baseline file).

The plan is to fix the problems separately later on.

[uuf6429]

@acoulton one motivation for adding PHPStan is that during the recent title change, I realised that there is a big discrepancy in types. For example:

class SomeRecord
{
    /** @var string */
    private $keyword;                                         // 1️⃣

    /** @var int */
    private $line;                                            // 1️⃣

    /**
     * @param string $keyword
     * @param int $line
     */
    public function __construct($keyword, $line) {            // 2️⃣
        $this->keyword = $keyword;
        $this->line = $line;
    }

    /** @return string */                                     // 3️⃣
    public function getKeyword(){ return $this->keyword; }    // 4️⃣

    /** @return int */                                        // 3️⃣
    public function getLine(){ return $this->line; }          // 4️⃣
}

class SomeRecordTest extends TestCase
{
    public function testSomething(): void
    {
        $r = new SomeRecord('word', null);                    // 5️⃣
        ...
    }
}

The main problem: $line is defined as int, but many tests pass a null. This happens a lot, for various other properties.

I would like to fix that and declare types in a backword compatible way:

• 1️⃣ type can be safely defined (for private props), but not sure if it should be nullable because of the test (see point 5)
• 2️⃣ type can probably be defined - it will affect users passing invalid types (question from 1️⃣ about null also applies)
• 3️⃣ if tests are to be kept as is, then null would be a valid value and the PHPDoc should be updated.
• 4️⃣ probably we cannot declare the return type without a BC break.
• 5️⃣ from my understanding, null should not be allowed and a valid value - such as -1 or 0 should be used instead.

Alternatively, we could avoid the BC headache and define all the types correctly for the next major release.

What do you think?

[acoulton]

@uuf6429 I haven't looked at this or your PHP-CS pulls in detail as I realised they're marked draft and overlap a bit with the PHPUnit / docs PR so I assume we should review & merge those first.

On the type-safety, I think (if we're certain it is only tests that are passing nulls):

• I agree, those tests are invalid and should be passing a value that mirrors real-world usage (so e.g. 1).
• It is definitely fine to put a typehint on the private properties
• It's also fine to put a typehint on the constructor params - if the phpdoc typed them as int then IMO it is not a BC break to start enforcing that (and it will be enforced by adding a typehint to the property anyway, so might as well be done on the constructor)
• As you say, we can't add concrete return types without breaking BC - unless the class is final.

[carlos-granados]

Regarding this draft pull request:

In the main Behat repository we use Psalm, while this PR introduces PHPStan in this one. I believe that it would be better if we unified this and used a single static analysis tool. Otherwise this can be confusing when you are working in one code base or the other and you need to follow different processes depending on which repo you are on. So I believe that we have two options:

• Update the main Behat repo to use PHPStan instead of Psalm. I believe that right now there are many more projects using PHPStan than using PSalm, so this may also help with developers adding code to the project as it is more probable that they are familiar with PHPStan than with PSalm. Our current PSalm level is not too high and we almost don't use any PSalm specific annotations so I believe that a transition to PHPStan should not be too difficult
• Or use PSalm in this repository as well

Also, I am not a big fan of baseline files. They have a number of drawbacks:

• Accumulation of Ignored Errors: Over time, the baseline file can grow as it accumulates ignored errors, making it challenging to manage and potentially leading to overlooked issues.
• Stale Error Records: As the code evolves, some errors in the baseline may become obsolete. PHPStan doesn't automatically remove these outdated entries, requiring manual intervention to keep the baseline current.
• Potential for Complacency: Relying heavily on the baseline might lead to a false sense of code quality, as existing issues remain unaddressed.
• Complexity in Management: In projects with multiple developers or extensive codebases, managing the baseline can become complex
  I prefer to start with a low level of rules and slowly update the code until we reach a level which gives us enough confidence in it

[uuf6429]

@carlos-granados as you know, this is just a draft, so I wouldn't focus too much on it yet. But to answer your points:

1. We should just switch to PHPStan in my opinion, especially since we have already implemented it in a few other behat/mink project (e.g. some mink drivers).
2. I agree about the baseline - the point here is that it is a workaround so that this PR could be merged separately (or at least reviewed so). Otherwise it will end up containing a lot of fixes and/or a broken or incomplete pipeline.

The next point would be if we merge this PR and the fixes PR into one (to avoid polluting github) or if we keep them separate (and e.g. the baseline would be stored somewhere in the git history). This can be discussed later though.

PS: A small comment on one of your points: if a baselined error becomes obsolete, PHPStan will fail automatically (unless disabled by a specific setting). Just an FYI.

[acoulton]

I'd agree with just using PHPStan - I noticed the doctrine project have just decided to drop psalm and use PHPStan-only in future.

Also agree with @carlos-granados that my preference would be to introduce it at the lowest-compatible level (without a baseline) and then gradually increase the level in parallel with fixing the issues. This also means it's easy to see from the config which level we have actually achieved.