Stream is not fully compatible with GD resource

[boesing]

Bug Report

Q	A
Version(s)	2.4.0

Summary

Using a GD resource works fine until there is interaction with the stream.

Current behavior

When working with GD Images, most of the methods of the Stream (with PHP 8.0 its GDImage) will result in errors.

How to reproduce

    public function testIsReableWithGdResource()
    {
        $resource = imagecreate(1, 1);
        $stream   = new Stream($resource);
        $this->assertTrue($stream->isReadable());
    }

    public function testCloseWithGdResource()
    {
        $resource = imagecreate(1, 1);
        $stream   = new Stream($resource);
        $this->assertNull($stream->close());
    }

result in

1) LaminasTest\Diactoros\StreamTest::testIsReableWithGdResource
stream_get_meta_data(): supplied resource is not a valid stream resource

2) LaminasTest\Diactoros\StreamTest::testCloseWithGdResource
fclose(): supplied resource is not a valid stream resource

So the existing code is already broken and doesn't work with image resources.

Expected behavior

Using methods return proper informations such as the GD Image/Resource is not writable, seekable, e.g.

Originally posted by @ADmad in https://github.com/laminas/laminas-diactoros/pull/46#r516719793

[settermjd]

I'm happy to work on this

[settermjd]

I don't know that it's possible to fix this, because stream_get_meta_data only works with streams created by fopen(), fsockopen(), and pfsockopen(). I'm open to learning about workarounds, but don't know of any atm.

[ADmad]

I don't think the issue is fixable either.

Two options I see are:

1. Deprecate and drop support for using GD resource / GDImage instance with Stream.
2. Throw an exception when trying to do operations like seeking which are not possible with a GDImage instance.

[settermjd]

Might be the case. I have to add some feedback from @weierophinney, tomorrow.

[settermjd]

Just found it:

As of PHP 8, GD resources are represented by a GdImage class, allowing you to do instanceof checks. (It's one of a small handful of resource types that got this treatment; there's also CurlHandle, Socket, and AddressInfo.) And in PHP 8... is_resource() fails with those types, and get_resource_type() will raise an error, IIRC. So you MUST do instanceof checks for those. (The benefit, though, is that you get dedicated types for these resources.)

[settermjd]

Sounds like the right approach, @ADmad

[Xerkus]

Is it a correct approach to feed GD resource at all? Http message is meant to work with streams, not php resources in general

[Xerkus]

From what I can see, GD resource is not meant to be used as data stream and one of image*() functions is meant to be used to output the image from GD resource to stdout, file or a stream resource, eg: https://www.php.net/manual/en/function.imagewebp.php

I would say this is not a bug and won't fix.

[ADmad]

I would say this is not a bug and won't fix.

How is it not a bug when the class accepts GD resources as argument for the constructor but generates errors when you try to use various functions of the class? A proper exception would have been at least acceptable.

https://github.com/laminas/laminas-diactoros/blob/4f1d12676a2c402fc8c02e7436cf6d80604ef611/src/Stream.php#L44-L46

Errors are generated for PHP 7 too, not just GdImage instances of PHP 8.

[Xerkus]

Oof. I did not know it was specifically whitelisted here. I was looking at the GD docs to see where it was meant to be a valid binary data stream of the image or just happened to be that way.

[boesing]

Is it a correct approach to feed GD resource at all? Http message is meant to work with streams, not php resources in general

I'd say no but that ship has sailed. Was added in #45 but I dont get all the reasons why this had to be added in the first place. Probably @settermjd can give us some more background. After this, we might want to deprecate GD usage and remove it in next major. Until then, we should throw proper diactoros exceptions when methods are called which use unsupported functions on the GD resource / GDImage (luckily, GDImage is not supported yet and should not be supported in any upcoming version) rather than triggering errors.

[boesing]

@settermjd

Do you still work on this or can we somehow deprecate the usage of GD resources? AFAIR you added this for something you were working on? Do you still use this? Do we know others are using it actively since it was only noted as a small hint within the API documentation?

I'd prefer not having some runtime deprecation and thus, just adding a NOTE to one of the next releases might suffice while also providing an example on how to create a stream from a gd resource before passing it as some kind of StreamInterface somewhere?

Since the whole gd infrastructure started to return GdImage with PHP 8+ anyways, there is no way to create the diactoros Stream without directly instantiating it anymore. Sure, one might ignore the fact that the StreamFactory actually just does not verify the argument type (resource) but that might be more of a bug rather than a feature.

---

So for me, there are 2 things TODO for v3:

• remove support for GD at all
• add strict check for argument type in StreamFactory#createFromResource before passing the value to Stream::__construct

(and maybe add some additional documentation on how to create some lazy stream or stuff like this especially for GdImage or resource).

[settermjd]

Sorry, @boesing, I dropped off on this one. Will try and set time aside to get back into it.

[settermjd]

Would love to chat to you about this, @boesing

[boesing]

@settermjd Sure, I am on holidays right now. Will be back the end of this month. We can have a chat then if we find some time 🤙🏻

[weierophinney]

My recommendation:

• We create a separate "ImageStream" implementation that works with GD resources.
• We update StreamFactory::createFromResource() to detect if the resource is a GD resource, and, if so, return an ImageStream implementation.
• We deprecate usage of GD streams within the Stream class immediately, and, when PHP 8.0 or above is detected, throw an exception immediately that indicates users should switch to ImageStream.
• In v3, we drop the ability to stream images from the main Stream implementation.

This should address the bulk of BC concerns, address the fact that the current implementation is broken in PHP 8+, and provide a roadmap for improving support.

[weierophinney]

Consensus after attempting to do this in #147 is that we should drop GD support from streams entirely. That will happen with 3.0.0.

[weierophinney]

Completed with #148