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

PHP8 is_resource behavioral change affects AbstractMonitoringMiddleware #2527

Closed
mhsdef opened this issue Sep 21, 2022 · 5 comments
Closed
Assignees
Labels
bug This issue is a bug. needs-review p2 This is a standard priority issue

Comments

@mhsdef
Copy link

mhsdef commented Sep 21, 2022

Describe the bug

AbstractMonitoringMiddleware::prepareSocket's is_resource check (#):

private function prepareSocket($forceNewConnection = false)
{
    if (!is_resource(self::$socket)
        || $forceNewConnection
        || socket_last_error(self::$socket)
    ) {
        self::$socket = socket_create(AF_INET, SOCK_DGRAM, SOL_UDP);
        socket_clear_error(self::$socket);
        socket_connect(self::$socket, $this->getHost(), $this->getPort());
    }

    return self::$socket;
} 

Looks affected by https://www.php.net/manual/en/migration80.incompatible.php#migration80.incompatible.resource2object

Expected Behavior

The check to cover both PHP7 and PHP8 behaviors.

Current Behavior

The check only covers <PHP8 behavior.

Reproduction Steps

See this:
https://onlinephp.io/c/3574d

Possible Solution

Check for !self::$socket instanceof Socket as well to cover PHP8.

Additional Information/Context

No response

SDK version used

master

Environment details (Version of PHP (php -v)? OS name and version, etc.)

PHP 8.0.23

@mhsdef mhsdef added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 21, 2022
@yenfryherrerafeliz yenfryherrerafeliz self-assigned this Sep 21, 2022
@yenfryherrerafeliz yenfryherrerafeliz added p2 This is a standard priority issue s Effort estimation: small and removed s Effort estimation: small labels Nov 3, 2022
@yenfryherrerafeliz
Copy link
Contributor

Hi @hewsut, thanks for opening this issue. I agree with the suggested change. I will mark this issue to be reviewed so we can address it further.

Thanks!

@yenfryherrerafeliz yenfryherrerafeliz added needs-review and removed needs-triage This issue or PR still needs to be triaged. labels Dec 7, 2022
@MattPearce
Copy link
Contributor

MattPearce commented Jan 19, 2024

@yenfryherrerafeliz this issue is blocking our PHP v8 upgrade, this PR will fix the issue if you could please merge it:

#2865

@yenfryherrerafeliz
Copy link
Contributor

Hi @MattPearce, I have left a comment on the PR. Would you please be able to take look to it.

Thanks!

@yenfryherrerafeliz
Copy link
Contributor

This issue has been fixed by the following PR. Closing for now.

Thanks!

Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. needs-review p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

3 participants