Fix error handling in Stream::getContents()

[odan]

The current implementation of stream_get_contents() swallows errors, leading to issues like symfony/symfony#52490, where errors reported by the stream layer aren't propagated to user-land.

This is even in the C source:
https://github.com/php/php-src/blob/311cae03e730c76aed343312319ed8cf1c37ade0/main/streams/streams.c#L1512

Proposed Solution

To address the issue, @nicolas-grekas proposes replacing the usage of stream_get_contents() with fread(). This change will ensure that errors are not concealed, and they will be thrown as expected.

Using fread(), should fix this issue.

See here: Nyholm/psr7#252

Thanks to @nicolas-grekas

[nicolas-grekas]

Thanks for the follow up. Please don't merge now as I might change the patch a bit depending on my tests.

[nicolas-grekas]

Upstream PR updated, see Nyholm/psr7#252 (comment)

[odan]

@nicolas-grekas Ok thanks, we are waiting.

[nicolas-grekas]

Merged upstream, you can now sync the patch.

[shadowhand]

@nicolas-grekas this code checks for false return value as well... (ref Nyholm/psr7#252 (review))

[l0gicgate]

@odan it appears CI is failing on 8.2. The changes look good to me otherwise

[piotr-cz]

The value $this->resource in fact may be null in following edge case:

class StreamExtended extends Stream
{
    public function brokenMethod()
    {
        // This sets $this->resource to null...
        $this->detach();
        // ...and in effect this calls stream_get_contents with null parameter
        $this->getStreamContents();
    }
}

I see folowing possible solutions:

• move method body exactly where the $contents = $this->getStreamContents(); was
• throw an Exception then $this->resource === null
• force stream to be a type of resource by changing new method's signature to protected static getStreamContents(resource $stream): string;