Fix issue 414, add tests + add php8 to travis (but not composer) #415
Conversation
@grymmare Thanks for your first opensource contribution! I added a code review for minor improvements. ;-) No worries about these minor mistakes, happens to everyone the first time. ^^ |
|
Not sure why Travis is not running on this PR. I don't think with the current setup of dependencies this is actually installable on PHP8 at the moment. See #413 for the initial discussion & errors. |
It seems to have been run now. With errors though, but they are psalm related (pure function returning member not marked as pure) |
|
I only noticed now this is targetting the master branch (v8) and not the 7.x branch :) |
| - DEPENDENCIES="--ignore-platform-req=php" | ||
| - php: 8.0 | ||
| env: | ||
| - DEPENDENCIES="--ignore-platform-req=php --prefer-lowest --prefer-stable" |
There was a problem hiding this comment.
I don't think we should ignore these here. Better fix these issues properly. Not doing so makes this either not installable for normal users or it forces them to use this flag on their project too.
There was a problem hiding this comment.
Sure, makes sense, but without the flag, we cannot run the tests for php 8 until all dependencies supports php 8. What if we don't add php8 to composer.json, that way we don't officially support php8, but can still run tests.
The tests that fail (in addition to dependencies not supporting php8 yet) are related to the vimeo/psalm version.
- Psalm 3.11.2 is successful
- Psalm 3.18.2 fails. Seems that marking a class with @psalm-immutable conflicts with @psalm-pure somehow. Maybe this can be resolved by specifying a version constraint, but I guess this depends on how psalm develops, or removing all @psalm-pure annotations in classes annotated with @psalm-immutable
|
As php-cs-fixer can be installed with PHP 8, but it's not possible to run yet, we need to disable these checks on travis until it's supported to check on PHP 8 as well. |
But, doesn't this only apply if we would start using php 8 features, which we cannot if we want to support 7.4? The tests that fail are the tests that uses the latest dependencies, so if they were successful before, probably they fail now due to changes in psalm and php-cs-fixer, and so they will fail in any new commit. Mostly psalm related and fails for both PHP 7.4 and PHP 8.0 (not related to this PR, but has to be fixed) The php-cs-fixer failures is minor (easy to fix and is in my added test file) @prolic what do you think? EDIT: I now see that due to how travis is configured, the cs check (and test-coverage) is only done for php 7.4. |
…omposer to not signal that it is supported
grymmare
changed the title
composer.json
Outdated
| @@ -28,7 +28,7 @@ | |||
| "phpspec/prophecy": "^1.10.3", | |||
| "phpunit/phpunit": "^9.1", | |||
| "prooph/php-cs-fixer-config": "^0.3.1", | |||
| "vimeo/psalm": "^3.11.2" | |||
| "vimeo/psalm": "^3.11.2,<3.12" | |||
There was a problem hiding this comment.
Because later versions adds psalm warnings which fails the build. (see log here: https://travis-ci.com/github/prooph/event-store/jobs/461215042)
My suggestion is to fix this in another PR. If you disagree though, we need to make the psalm checks successful. I havn't found any other solution than removing all the affected @psalm-pure annotations.
There was a problem hiding this comment.
Found this: vimeo/psalm#4114
Instead of @psalm-pure, use @psalm-mutation-free
I can change this throughout the code (more than 100 places I think) and fix some other psalm related stuff. Just let me know.
Thanks. Updated to use this version |
|
@grymmare sorry I didn't check this earlier. My bad. |
substrDateTimeStringBugWorkaroundPlease go gentle on my, this is my first PR in the open source community. Please let me know if I can improve anything. I'd like to continue to contribute 😄