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

Add ReturnTypeWillChange to FrameCollection #706

Merged
merged 2 commits into from Sep 19, 2021

Conversation

kylekatarnls
Copy link
Contributor

Fix PHP 8.1 compatibility

Copy link
Collaborator

@denis-sokolov denis-sokolov left a comment

Choose a reason for hiding this comment

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

Thank you, @kylekatarnls! Indeed, we want something like that.

Comment on lines +181 to +189
public function __serialize(): array
{
return $this->frames;
}

public function __unserialize(array $serializedFrames): void
{
$this->frames = $serializedFrames;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these related to the ReturnTypeWillChange or is this an accidental addition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also needed for PHP 8.1 compatibility because Serializable is deprecated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

@SvenRtbg
Copy link
Contributor

SvenRtbg commented Oct 2, 2021

Guys, this merge destroys PHP 5 compatibility because of the return types.

@kylekatarnls
Copy link
Contributor Author

kylekatarnls commented Oct 4, 2021

GitHub Actions started from PHP 7.0, I didn't notice the composer.json still supports PHP 5.

If PHP 5 is still meant to be supported, then it should be added to GitHub Actions to be tested.

But maybe it's a mistake in the composer.json. Actually I don't think it will be possible to be compatible with both PHP 5 and 8.1 without deprecation/warning notices in the same branch.

And I know it's not what you want to hear but PHP 8 is out now, so it's really time to drop PHP 5.

If you decide not to upgrade PHP, then the more consistent move could be also to lock the filp/whoops version to something like ~2.13.0

@SvenRtbg
Copy link
Contributor

SvenRtbg commented Oct 4, 2021

I agree that the existing support for PHP 5, even though untested, is surprising, but it is what is announced in the composer.json.

As you can see from #504 it has been discussed long ago to drop PHP 5 support, but somehow never happened.

@AmraniCh
Copy link
Contributor

AmraniCh commented Oct 5, 2021

I think whoops must be able to run in all PHP versions (from PHP 5), PHP 5 is still there in the market and a lot of websites still use it specially in development environment (including me).

@staabm
Copy link
Contributor

staabm commented Oct 5, 2021

just have a look at the master branch. denis already added testcoverage 7bf80a3

@kylekatarnls
Copy link
Contributor Author

PHP 5 is still there in the market and a lot of websites still use it specially in development environment (including me).

"A lot" is probably exaggerated:
https://blog.packagist.com/php-versions-stats-2021-1-edition/

It's completely negligible in proportion. And if you don't feel using PHP 5 is a problem, you should feel OK to not be up to date with the latest filp/whoops neither 🤷

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.

None yet

5 participants