-
-
Notifications
You must be signed in to change notification settings - Fork 454
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
Support PHP 8.1 for sentry-php 2.x #1247
Conversation
@@ -149,7 +149,7 @@ public function offsetExists($offset): bool | |||
/** | |||
* {@inheritdoc} | |||
*/ | |||
public function offsetGet($offset) | |||
public function offsetGet($offset): mixed |
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.
This is not a feasible change, it would break the app for everyone below PHP 8.1
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.
Are you sure about that?
According to the composer.json the minimal PHP version is 7.1 ("php": "^7.1",
). Return type declarations are supported since PHP 7.0 and these declarations can be overridden in child classes.
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.
The mixed
type was implemented in this RFC for PHP 8.
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 that's true...
The alternative is to add the #[\ReturnTypeWillChange]
attribute, but that will break compatibility for < PHP 8.0. 😬
Fatal error: During inheritance of ArrayAccess: Uncaught ErrorException: Return type of Sentry\Context\Context::offsetGet($offset) should either be compatible with ArrayAccess::offsetGet(mixed $offset): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/vendor/sentry/sentry/src/Context/Context.php:152
There is already open #1245 for PHP 8.1, I will keep working on it once I have some spare time |
Thanks for the update! This PR however targets an older version: sentry-php 2.x. |
True, I didn't notice it. However, version 2.x only supports PHP 7.x and this decision was deliberate. If you want to use the SDK on PHP8, please update to the newer version |
We can't update to sentry-php 3.x, because of this issue: getsentry/sentry-symfony#421.
With this change we can still use Sentry and upgrade to the latest PHP 8.1. 🙂