-
-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for PHP 8, fix tests #57
Conversation
Still some test errors because of needing to change testing for private/protected properties with PHPUnit 9 Signed-off-by: Drew Robinson <[email protected]>
Signed-off-by: Drew Robinson <[email protected]>
Thanks to the PR from @gennadigennadigennadi I've incorporated those fixes into this PR to get PHPUnit 9 support. Signed-off-by: Drew Robinson <[email protected]>
Can't figure out how to deal with the `getClass()` deprecations cleanly in all instances in the files in this PR. Signed-off-by: Drew Robinson <[email protected]>
Signed-off-by: Drew Robinson <[email protected]>
The problematic area I need help with is around |
@ocean this patch helps for me:
|
Thanks https://github.com/snapshotpl Signed-off-by: Drew Robinson <[email protected]>
Perhaps test should not look at this, given it's ignored by the class? Signed-off-by: Drew Robinson <[email protected]>
@froschdesign This is ready for review now 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! 👍
Two changes are necessary and I have added a question.
@@ -171,7 +174,7 @@ public function testFactoryCanSupplyAMixOfParameterTypes() | |||
self::assertInstanceOf(TestAsset\ClassWithMixedConstructorParameters::class, $instance); | |||
|
|||
self::assertEquals($config, $instance->config); | |||
self::assertEquals([], $instance->options); | |||
self::assertEquals(null, $instance->options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change mean that there was an error before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there was an error in the tests when I ran them locally. I thought I saw a comment something about $options
always being treated as null
, but now I can't find it. I'll revert this change and then you can see the error that shows up in Travis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@froschdesign Yep, in the test the options
instance property is set to null
, not to an empty array, see:
https://github.com/laminas/laminas-servicemanager/blob/3.5.x/test/AbstractFactory/TestAsset/ClassWithMixedConstructorParameters.php#L24
So either the assertion or the test class constructor needs changing I guess.
Signed-off-by: Drew Robinson <[email protected]>
Signed-off-by: Drew Robinson <[email protected]>
Signed-off-by: Drew Robinson <[email protected]>
Could you tag a new release for this feature? |
@FabianKoestring done |
@FabianKoestring beware: due to #59, this has been reverted for now, and will therefore be delayed. |
Targeting issue #55
Still some test errors because of needing to change testing
for private/protected properties with PHPUnit 9 - I'm not sure how to test these properties now.
Signed-off-by: Drew Robinson [email protected]