Skip to content
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

Update tests and add PHP 8.3 to the CI matrix #3299

Merged
merged 6 commits into from
Dec 14, 2023

Conversation

akrabat
Copy link
Member

@akrabat akrabat commented Nov 23, 2023

A few things that we were doing in the tests are deprecated and so have been updated to be correct.

  • We now use setStaticPropertyValue() to set static properties on a reflected class rather than setValue() on a reflected property.
  • We no longer dynamically add a request property to the response class in BodyParsingMiddlewareTest, instead we add it to our handler class.
  • We no longer create an unused reflected property in ErrorHandlerTest.

I have also added PHP 8.3 to the CI matrix and moved our static analysis tests to PHP 8.2.

Note that we'll need to wait for phpspec/prophecy to support PHP 8.3 before we can merge.

@akrabat akrabat requested a review from l0gicgate November 23, 2023 12:50
@coveralls
Copy link

coveralls commented Nov 23, 2023

Coverage Status

coverage: 99.427% (-0.002%) from 99.429%
when pulling 4bfb12a on akrabat:update-for-php83
into ca951fb on slimphp:4.x.

From PHP 8.3, calling ReflectionProperty::setValue() with a single value
is deprecated, so we use ReflectionClass::setStaticPropertyValue()
instead.
It's not used and causes a warning.
Dynamically adding a property to a class is deprecated and so rather
than adding the request to the response from the handler, we now add it
to the handler itself and test it there.
Also use 8.2 for the CS and static analysis tests.
@l0gicgate
Copy link
Member

l0gicgate commented Nov 24, 2023

@akrabat it appears installing the dependencies fails on PHP 8.3
CleanShot 2023-11-23 at 22 45 36@2x

@akrabat
Copy link
Member Author

akrabat commented Nov 24, 2023

@akrabat it appears installing the dependencies fails on PHP 8.3

Yes. See "note" in PR description :)

@l0gicgate
Copy link
Member

@akrabat my apologies 😅 I guess we'll just have to be patient!

@stemd
Copy link

stemd commented Dec 11, 2023

Seems that phpspec/prophecy now supports PHP 8.3 (commit 3 days ago):

phpspec/prophecy@03cfe36?diff=unified&w=0

@akrabat
Copy link
Member Author

akrabat commented Dec 11, 2023

Thanks @stemd. I have re-run the PHP 8.3 CI test and it passed. Over to @l0gicgate for review.

@l0gicgate l0gicgate added this to the 4.13.0 milestone Dec 12, 2023
Copy link
Member

@l0gicgate l0gicgate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For cohesion, can we please import ReflectionClass in the import statements instead of using \ReflectionClass. Looks great otherwise, thank you for this contribution.

@akrabat
Copy link
Member Author

akrabat commented Dec 12, 2023

For cohesion, can we please import ReflectionClass in the import statements instead of using \ReflectionClass.

Done.

@l0gicgate l0gicgate merged commit 56ead41 into slimphp:4.x Dec 14, 2023
5 of 6 checks passed
@akrabat akrabat deleted the update-for-php83 branch June 13, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants