ImageStream implementation #147

weierophinney · 2023-05-02T13:24:45Z

Q	A
Documentation	no
Bugfix	yes
BC Break	future
New Feature	yes
RFC	no
QA	no

Description

Per discussion in #57, this patch does the following:

Introduces the ImageStream class as an extension of Stream that only works with GdImage resources.
Updates Stream to emit an E_USER_DEPRECATED error when used with a GdImage resource, pointing users to the new ImageStream implementation.
Updates StreamFactory::createStreamFromResource() to detect GdImage resources, and to create and return an ImageStream when it does.

The plan will be for 3.0.0 to remove support for GdImage from Stream.

`ImageStream` is an extension of `Stream`, with the following key differences: - The `$resource` property is documented as being `null|GdIamge` - The constructor accepts a `GdImage` **only**, and assigns it to the `$resource` property - The `detach()` method no declares a return type of `null|GdImage` - The `attach()` method documents via annotation that the `$resource` it accepts can only be of type `GdImage`; the method raises an exception for any other argument. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>

When a `GdImage` is used, the class will now issue an `E_USER_DEPRECATED` indicating the user should use `ImageStream` instead. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>

…with GdImage Updates `createStreamFromResource()` to detect `GdImage` resources, and, when detected, create and return an `ImageStream` instead of a `Stream`. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>

Xerkus · 2023-05-02T13:55:48Z

src/ImageStream.php

+ * allow it to extend Stream. If defined here with a type, PHP will raise a
+ * fatal error complaining that it must not define a type for the property.
+ */
+class ImageStream extends Stream


It should not extend Stream at all. It is not compatible with any of its methods.

I agree here, extending Stream makes no sense as we have to handle almost every method in a specific way.
Especially due to the GdImage/resource problematic, we could also just provide GdImageStream as it is for GdImage only.
Not sure if some1 wants to use this for anything else, so renaming it might be helpful but I do not insist on this.

Xerkus · 2023-05-02T13:56:44Z

src/Stream.php

@@ -359,6 +361,14 @@ private function isValidStreamResourceType(mixed $resource): bool
        }

        if ($resource instanceof GdImage) {


I get type errors for pretty much all methods in Stream when GdImage is used as resource.

Uncaught TypeError: stream_get_contents(): Argument #1 ($stream) must be of type resource, GdImage given Uncaught TypeError: stream_get_meta_data(): Argument #1 ($stream) must be of type resource, GdImage given Uncaught TypeError: fseek(): Argument #1 ($stream) must be of type resource, GdImage given

and so on

It did not work at all for a long time. I suggest "support" is dropped without deprecation as no actual users will be affected.

Yah, there were no proper tests for PHP 8.1
Thats why I raised the whole concerns in the first place.

Also stream seeking on gdimage resources never worked and plenty of other stuff (i.e. writing to that stream).

Thats why the new ImageStream implementation has to return false for isSeekable, isWritable, etc.

Has to throw exceptions for getMetadata, etc.

Xerkus · 2023-05-02T14:23:10Z

src/StreamFactory.php

+     *
+     * @param resource|GdImage $resource
     */


I am not sure if this is good. Expanding parameter types is considered type safe. The problem is anyone needing GdImage support can no longer depend on PSR factory interface because that would not be type safe.

Wouldn't that go against interoperability goal between libraries since this will be creating dependency on diactoros and this implementation?

GdImage is considered an opaque resource type. The general assumption of PSR-7 and PSR-17's stream support is that it can work with resources of any type. However, both pre-date the opaque resource types.

This suggests two things to me:

They should allow usage with opaque resource types where they make sense. Since you can have image resources spit out content, I'd argue they make sense here, and, in some ways, are better than normal file-based resources where you lose idempotency.

Alternately, PSR-7 and/or PSR-17 need updates to either explicitly allow or disallow usage with opaque resource types.

For now, I don't think this patch violates interoperability, as PSR-7 and PSR-17 do talk about resources, and this is a type of resource.

I would not support GdImage via StreamFactory TBH

PSR is clear here:
https://github.com/php-fig/http-factory/blob/e616d01114759c4c489f93b099585439f795fe35/src/StreamFactoryInterface.php#L44

If some1 wants to use GdImage with StreamFactoryInterface they are already lost. If they hardly depend on laminas as an implementation, they don't need PSR and thus can directly instantiate ImageStream rather than using the interchangeable factory. The whole point of using the StreamFactory is to provide interchangeability and if we now decide to support GdImage that would only fork for laminas but not for guzzle for example. Having these kind of incompatibility is super annoying and thus, I'd say we should not allow that at all.

boesing · 2023-05-02T14:54:52Z

test/StreamTest.php

@@ -67,12 +71,29 @@ public function testCanInstantiateWithStreamResource(): void
        $this->assertInstanceOf(Stream::class, $stream);
    }

+    /**
+     * @todo For version 3, this test should verify that a GdImage raises an  exception.


with v3 we should verify if either string or resource is passed, everything else should throw an exception.

boesing · 2023-05-02T14:59:21Z

src/Stream.php

@@ -359,6 +361,14 @@ private function isValidStreamResourceType(mixed $resource): bool
        }

        if ($resource instanceof GdImage) {


Yah, there were no proper tests for PHP 8.1
Thats why I raised the whole concerns in the first place.

Also stream seeking on gdimage resources never worked and plenty of other stuff (i.e. writing to that stream).

Thats why the new ImageStream implementation has to return false for isSeekable, isWritable, etc.

Has to throw exceptions for getMetadata, etc.

boesing · 2023-05-02T15:02:16Z

src/StreamFactory.php

+     *
+     * @param resource|GdImage $resource
     */


I would not support GdImage via StreamFactory TBH

PSR is clear here:
https://github.com/php-fig/http-factory/blob/e616d01114759c4c489f93b099585439f795fe35/src/StreamFactoryInterface.php#L44

If some1 wants to use GdImage with StreamFactoryInterface they are already lost. If they hardly depend on laminas as an implementation, they don't need PSR and thus can directly instantiate ImageStream rather than using the interchangeable factory. The whole point of using the StreamFactory is to provide interchangeability and if we now decide to support GdImage that would only fork for laminas but not for guzzle for example. Having these kind of incompatibility is super annoying and thus, I'd say we should not allow that at all.

boesing · 2023-05-02T15:39:49Z

Summing up Slack discussion:

Currently, neither PHP 7.4 nor 8.0 with both resource or GdImage are properly functioning as both GdImage and the resource which are passed do not contain the image type. So should the image created as an PNG or a JPEG?
Lacking this information, using stream_get_contents on the gdimage resource will result in:

Warning: stream_get_contents(): supplied resource is not a valid stream resource

For PHP 8.0 and GdImage, that error will slightly change to Warning: Uncaught TypeError: stream_get_contents(): Argument #1 ($stream) must be of type resource, GdImage given.

So the whole image feature is non-existent and therefore never worked. Won't be even a BC break by removing this feature tho.

weierophinney · 2023-05-02T16:22:54Z

Based on the Slack conversation and the feedback here, I'm going to close this patch, and then open a new one against 3.0.x to drop support for GD images in Stream.

weierophinney added 3 commits May 1, 2023 16:28

refactor: deprecate usage of GdImage in Stream class

632265b

When a `GdImage` is used, the class will now issue an `E_USER_DEPRECATED` indicating the user should use `ImageStream` instead. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>

weierophinney added this to the 2.26.0 milestone May 2, 2023

weierophinney added the Enhancement label May 2, 2023

weierophinney requested a review from boesing May 2, 2023 13:25

Xerkus reviewed May 2, 2023

View reviewed changes

weierophinney added the Work In Progress label May 2, 2023

boesing requested changes May 2, 2023

View reviewed changes

weierophinney closed this May 2, 2023

weierophinney removed this from the 2.26.0 milestone May 2, 2023

weierophinney added Won't Fix This will not be worked on and removed Work In Progress labels May 2, 2023

weierophinney mentioned this pull request May 2, 2023

Stream is not fully compatible with GD resource #57

Closed

weierophinney deleted the feature/image-stream-deprecation branch May 2, 2023 16:24

weierophinney mentioned this pull request May 2, 2023

Remove support for GD images in Stream implementation #148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ImageStream implementation #147

ImageStream implementation #147

weierophinney commented May 2, 2023

Xerkus May 2, 2023

boesing May 2, 2023

Xerkus May 2, 2023

Xerkus May 2, 2023

boesing May 2, 2023

Xerkus May 2, 2023 •

edited

Loading

weierophinney May 2, 2023

boesing May 2, 2023

boesing May 2, 2023

boesing May 2, 2023

boesing May 2, 2023

boesing commented May 2, 2023

weierophinney commented May 2, 2023

		@@ -359,6 +361,14 @@ private function isValidStreamResourceType(mixed $resource): bool
		}

		if ($resource instanceof GdImage) {

ImageStream implementation #147

ImageStream implementation #147

Conversation

weierophinney commented May 2, 2023

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xerkus May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boesing commented May 2, 2023

weierophinney commented May 2, 2023

Xerkus May 2, 2023 •

edited

Loading