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

TimeoutException: fix defaults to avoid notices with PHP 8.1 #50

Merged

Conversation

Thomas-Gelf
Copy link
Contributor

When being strict about deprecations, once reaching a timeout(), your code will fail on PHP 8.1. That's what this pull request attempts to fix.

Hint: created two distinct commits, the first one allows to trigger the error. You can have a look at a failing test run here.

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@Thomas-Gelf Thanks for looking into this, your changes make a lot of sense to me! I've filed #49 a few days ago which adds documentation for the existing APIs and also stumbled upon this.

As a first step, I think adding the documentation for the existing API via #49 makes sense. Accordingly, the message can be given as a null value to the constructor (not suggesting it's a good idea, merely inspecting the existing API). As such, I think we should probably apply your changes to address the default type, but also type cast internally in case a null value is given explicitly.

What do you think about this?

@Thomas-Gelf Thomas-Gelf force-pushed the fix/php8.1-exception-signature branch from 0fa200e to d4a0b8c Compare December 3, 2021 08:00
@Thomas-Gelf
Copy link
Contributor Author

Hi @clue,

I love the improvements in #49. It's easier to read and to understand, and the type hints are very helpful. I adjusted the pull request to keep the method signature as is.

@clue clue added this to the v1.8.0 milestone Dec 4, 2021
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@Thomas-Gelf Awesome, thank you for the update, changes LGTM! :shipit:

Keep it up 👍

@clue
Copy link
Member

clue commented Dec 4, 2021

For future reference only: This PR builds on top of #49 and #48.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants