Add native typhints to all hamcrest classes (+ install phpstan)

[pscheit]

Hey there 👋 ,

I felt so free to PR native typehints for this library. I noticed that most of the Matchers:: and other methods are not type hinted at all, which makes it really difficult to use the library in other typed libraries (e.g. https://github.com/webforge-labs/object-asserter).

However I went through everything in ./hamcrest with the help of phpstan and added typehints where they were missing.

Added the few "real issues" I found into the baseline.

What do you think, would this worth it to create a Version 3 of this library? Because everything I made is highly non backwards compatible, once got was extended by third parties.
For user the library api shouldn't change.

I added phpstan because that way those refactorings are much easier and easier to spot if mistakes are made.

While going through all that found some few other quirks I will address separately - which would even be backwards compatible.

Hope you like it, if not - don't sweat it, since we didnt talk about it before.

Best regards,
Philipp

Btw: you wrote about codestyle in the CONTRIBUTING notes, but couldnt find a guide?

• fqn / importing is a bit mixed
• sometimes the docblocks look a bit odd cause they are broken down at 120 chars? (imho code can be wider these days)
• found a new line here and there that doesnt belong, but nothing big. Left them untouched.

[aik099]

If possible please use actually used type/types here and other DocBlocks.

[pscheit]

mixed only got a native typehint in php 8.0.

PHP 7.4:

/**
 * @var mixed $_keyMatcher
 */
 private $_keyMatcher;

and have to use docblocks for this.

PHP 8.x:

 private mixed $_keyMatcher;

Idk if you were following the PHP update discussion, I think version 3.0 with typehints could have a minimum version of PHP 8.0 if it would be my library.
https://www.youtube.com/watch?v=iVaSGD2fCXU

[pscheit]

Oh sorry, I think I understood your comment only now.

Fixing..

[aik099]

I've meant use @var string|integer|etc $_keyMatcher instead of @var mixed $_keyMatcher in DocBlocks only. Hopefully that would make PHPStan happier.

[pscheit]

Thanks for the review!

So got a few questions now

• Codestyle: do you want to generally import FQNs or keep FQNS. I felt like often there could be more imports. In general I like keeping either FQNS or importing (with use) everything, not mixing.
• Do you want to bump version 3.x to php >=8.0 so that we can use mixed as native type hints
• all the issues in the baseline could be fixed by asserting more: should this done by throwing an Exception or else?

I will:

• have a look at the generated files how to fix them
• attack the issues in the baseline as you suggested separately
• remove the Justfile (or add it in another PR if you like that) and cleanup the .gitignore
• check the codestyle in several places (if you care that much about codestyle, I would suggest to create a php cs fixer config in the repo)
• add a PR for the missing extensions in composer.json

I could:

• harmonize (separate PR) how arrays and/or lists are defined (not using Matcher[] anymore)

[pscheit]

Actually, never mind about the mixing FQNS and not. I understood it now! Because the generator doesn't support imports/use statements, so this one needs to be FQDNs, because it will be taken for code generation ✅

[aik099]

• Codestyle: do you want to generally import FQNs or keep FQNS. I felt like often there could be more imports. In general I like keeping either FQNS or importing (with use) everything, not mixing.

I personally prefer to import everything, but this change should be done in a separate PR.

• Do you want to bump version 3.x to php >=8.0 so that we can use mixed as native type hints

No.

• all the issues in the baseline could be fixed by asserting more: should this done by throwing an Exception or else?

Throw an exception probably or use an assert(...) function.

• harmonize (separate PR) how arrays and/or lists are defined (not using Matcher[] anymore)

Sure. Please do this.

• attack the issues in the baseline as you suggested separately

OK.

Actually, never mind about the mixing FQNS and not. I understood it now! Because the generator doesn't support imports/use statements, so this one needs to be FQDNs, because it will be taken for code generation ✅

If you'd like (in separate PR) we can teach generator to understand imports.

[pscheit]

No need to review, unless I request a review, because this will make things even more complicated I think

[pscheit]

Hey, I did prioritize some other Open Source work before getting back to this.

To not have to leave this open for months, I would suggest to accept a non-empty phpstan baseline and start the journey of typed code with a little bit of legacy code. This is the way to at least not create new issues while the repository lives on.

Let me know what you would prefer.

[aik099]

No need to review, unless I request a review, because this will make things even more complicated I think

My apologies. I'll wait for time, when you request a review then.

To not have to leave this open for months, I would suggest to accept a non-empty phpstan baseline and start the journey of typed code with a little bit of legacy code. This is the way to at least not create new issues while the repository lives on.

Then please do this:

1. rebase this PR (because it has merge conflicts)
2. solve the 1 issue not related to the PHPStan baseline
3. create separate PR for each of the non-resolved PHPStan baseline-related issues
4. send this PR for review

[pscheit]

Mhh. Maybe we missunderstood.

3. create separate PR for each of the non-resolved PHPStan baseline-related issues

I was suggesting to not do this (now).

[aik099]

Mhh. Maybe we missunderstood.

3. create separate PR for each of the non-resolved PHPStan baseline-related issues

I was suggesting to not do this (now).

Then create a task(s) explaining the idea behind each of the issues hidden in the PHPStan baseline and how it is supposed to be fixed in the future.

This way we won't lose the fact that something needs to be fixed after this PR is merged.

[pscheit]

Created and linked all three issues from the latest phpstan run without the baseline - they stayed the same.

Resolved the conflicts with upstream (was just the re-generation of the generated files)

[aik099]

Why the symfony/var-dumper is needed?

[pscheit]

sorry, my mistake. I removed it

[aik099]

I recommend adding a GitHub Action job to run the PHPStan automatically. Example: https://github.com/minkphp/driver-testsuite/blob/master/.github/workflows/ci.yml .

[pscheit]

@aik099 yep, I recommend that, too ;) You have to do this in a separate PR.

Honestly this is a bit hard to do as a contributor without the checks running automatically

[aik099]

@aik099 yep, I recommend that, too ;) You have to do this in a separate PR.

Honestly this is a bit hard to do as a contributor without the checks running automatically

Since PhpStan is added in this PR, then GitHub Action changes related to it should be here as well.

[pscheit]

Added the config

[pscheit]

ah, now the checks ran directly, didn't notice it the first time. Well then i was wrong: this was easy

[aik099]

Please do this:

1. move this job into an existing YML file instead of adding a new one
2. for an existing YML file:
   1. rename into ci.yml
   2. replace name: tests with name: ci
   3. replace name: PHP ${{ matrix.php }} into name: Tests ${{ matrix.php }}

This way we'll have all CI runs in one place. Right now I have to jump through the hooks to see static analysis and tests of the same PR on the https://github.com/hamcrest/hamcrest-php/actions page.

Example result: https://github.com/minkphp/Mink/actions/runs/13197030721.

[pscheit]

done

[aik099]

Merged. Thank you @pscheit .