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.

Sign up for GitHub

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 error handling in Stream::getContents() #252

Merged
merged 1 commit into from
Nov 13, 2023
Merged

Fix error handling in Stream::getContents() #252

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/Stream.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,11 +260,19 @@ public function getContents(): string
throw new \RuntimeException('Stream is detached');
}

if (false === $contents = @\stream_get_contents($this->stream)) {
throw new \RuntimeException('Unable to read stream contents: ' . (\error_get_last()['message'] ?? ''));
$exception = null;

\set_error_handler(static function ($type, $message) use (&$exception) {
throw $exception = new \RuntimeException('Unable to read stream contents: ' . $message);
});

try {
return \stream_get_contents($this->stream);
} catch (\Throwable $e) {
throw $e === $exception ? $e : new \RuntimeException('Unable to read stream contents: ' . $e->getMessage(), 0, $e);
} finally {
\restore_error_handler();
Comment on lines +265 to +274
Copy link

Choose a reason for hiding this comment

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

I don't understand how setting an error handler fixes this issue when other tools and even Guzzle check for a false return. Nothing about this code seems like it will actually solve the issue, because the return value of stream_get_contents is not being checked.

Copy link
Collaborator Author

@nicolas-grekas nicolas-grekas Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When stream_get_contents returns false, it first always triggers a PHP warning. Since we catch those in the error handler, the function will never really return false.

Copy link

Choose a reason for hiding this comment

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

Considering that such behavior is not documented, why take that approach over checking the return value is false? Every other fix I've looked at around this issue is doing a $ret === false check...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be dead code. Many things are not documented, especially regarding error handling. Returning false always means an error, and PHP has to trigger a warning to tell about the error so this is still solid IMHO. Of course, feel free to add the check on your side if you're not comfortable with the logic.

Copy link

Choose a reason for hiding this comment

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

That would be dead code.

Are you saying that checking === false does not work, or are you saying that this monkey patching with the error handler identifies the impending return false more quickly?

Copy link

@odan odan Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have a question to understand this patch a little better.
Why do you declare an "$exception" variable for the Exception although the catch block gets the same Exception instance? Is there some PHP specific logic / behavior behind this (that is not documented)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we remove this check, we would always wrap the exception thrown in the error handler in another exception. This check allows removing this needless wrapping exception.

Copy link

@odan odan Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, thanks. Then my next question is, why not move the set_error_handler into the try-block to handle that (scope) in the same place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if set_error_handler fails for any reasons, we don't want restore_error_handler to be called

Copy link

Choose a reason for hiding this comment

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

And in PHP, try/catch does not create a scope anyway.

}

return $contents;
}

/**
Expand Down
Loading