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

Fix expected error code in tests when ext-sockets is not enabled #532

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

lucasnetau
Copy link
Contributor

The expected error code when ext-sockets is not enabled (SOCKET_EMSGSIZE is not defined) is 90 per the two places that OverFlowException is created in class Transaction

http/src/Io/Transaction.php

Lines 173 to 176 in 212382c

return reject(new \OverflowException(
'Response body size of ' . $size . ' bytes exceeds maximum of ' . $this->maximumSize . ' bytes',
\defined('SOCKET_EMSGSIZE') ? \SOCKET_EMSGSIZE : 90
));

http/src/Io/Transaction.php

Lines 205 to 208 in 212382c

$reject(new \OverflowException(
'Response body size exceeds maximum of ' . $this->maximumSize . ' bytes',
\defined('SOCKET_EMSGSIZE') ? \SOCKET_EMSGSIZE : 90
));

@SimonFrings
Copy link
Member

@lucasnetau Thanks for bringing this up 👍

Seems like a reasonable change to me, but I don't have at much insights on this as @clue and @WyriHaximus, so interested in their opinion on this.

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.

@lucasnetau Thank you for looking into this, changes LGTM! :shipit:

This fixes a small oversight in our test suite only that I've introduced when removing the dependency on reactphp/promise-stream (#482).

Once merged, we should probably backport the same changes to the 1.x branch as well 👍

@clue clue added this to the v3.0.0 milestone Aug 10, 2024
WyriHaximus added a commit to WyriHaximus-labs/http that referenced this pull request Aug 27, 2024
This is a backport of reactphp#532 and corrects an oversight introduced in reactphp#482.
@WyriHaximus
Copy link
Member

Once merged, we should probably backport the same changes to the 1.x branch as well 👍

Just filed #539 to address this in the 1.x branch.

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to file this fix, LGTM 👍

@WyriHaximus WyriHaximus merged commit fc09a64 into reactphp:3.x Aug 27, 2024
8 checks passed
WyriHaximus pushed a commit to WyriHaximus-labs/http that referenced this pull request Aug 30, 2024
This is a backport of reactphp#532 and corrects an oversight introduced in reactphp#482.
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.

4 participants