Add PSR-17 `StreamFactoryInterface` Support in `ImplicitHeadMiddleware` #48

gsteel · 2023-03-29T09:15:03Z

Q	A
BC Break	yes
New Feature	yes

Description

Replaces all callable parameters and properties with StreamFactoryInterface and ResponseFactoryInterface where appropriate and removes the callable compatability layer.

Closes #29

froschdesign · 2023-03-29T09:23:51Z

@gsteel
The target branch 4.0.x is missing.

gsteel · 2023-03-29T09:29:31Z

The target branch 4.0.x is missing.

I'll create the branch if this is approved 👍

boesing · 2023-03-29T09:33:43Z

Can we please go with the logic as in https://github.com/mezzio/mezzio-authorization/pull/11/files?
Forward compatibility in current major versions should be the goal to reduce the amount of pain when migrating to new majors.
We can fix our internal types in the next major version but until then, providing support for both callable and the factory interfaces is doable. There are some caveats (i.e. due to the aliases / delegators registered in upstream projects - also initializers but I forgot these in the other components already) but IMHO worth doing so.

gsteel · 2023-03-29T10:12:49Z

I'm not sure I understand the point of preserving support for callables @boesing - As this lib is only useful to a Mezzio app, it's most likely that Stream|Response factories are already wired up in the container via Diactoros, making migration a zero effort chore.

The callable response decorator was already deprecated, and I'll have to introduce a callable stream decorator to satisfy StreamFactoryInterface which in turn will create a hard dependency on Diactoros' StreamFactory so that createStreamFromFile etc can be implemented.

I thought that this patch was more about removing support for callables than adding PSR-17 because at least ResponseFactoryInterface has been supported for quite a while now???

boesing · 2023-03-29T10:17:02Z

I thought that this patch was more about removing support for callables than adding PSR-17 because at least ResponseFactoryInterface has been supported for quite a while now???

It is not supported in this component for quite a while and thus I dont understand why you think it is already supported.
The whole point of introducing PSR-17 is that we provide the possibility from within all the components before we require upstream to provide PSR-17.
Just because you have laminas/laminas-diactoros required does not mean that you automatically have its config provider registered (which would be required to have the factory interfaces available via $container - or upstream registered manually all the factory interfaces which is most likely not the case as it is not required to do so in mezzio v3).
Furthermore, diactoros is not required in mezzio, you can use nyholm/psr7 which ~~(AFAIR) does not provide any PSR-17 factories~~ provides PSR-17 factories but would require configuration changes to actually register all the factory interfaces to the nyholm/psr7 PSR-17 factory).

So overall, adding support for both makes sense as a first step while dropping support in next major.
Starting with mezzio v4, I'd expect all components do support PSR-17 as there won't be any callable registered anymore.

But maybe I am missing something, can you probably elaborate why you think that there is already PSR-17 support in this component?

So all I want to say is, I spent quite a lot of time in providing proper forward compatibility for PSR-17 so that we can switch to full PSR-17 in the next major. All I want is that we provide compatibility layer in mezzio-router v3 so that we can do this change (so the change in this PR) in mezzio-router v4.

Replaces all callable parameters and properties with `StreamFactoryInterface` and `ResponseFactoryInterface` where appropriate and removes the callable compatability layer. Closes mezzio#29 Signed-off-by: George Steel <george@net-glue.co.uk>

This reverts commit c127396. Signed-off-by: George Steel <george@net-glue.co.uk>

…ctoryInterface` where appropriate Signed-off-by: George Steel <george@net-glue.co.uk>

gsteel · 2023-03-29T11:11:01Z

@boesing - I reverted the patch and added compatibility for StreamFactoryInterface - I was actually targeting 4 here, so I think I understand now that whilst the response factory was already supported here, the stream factory was still outstanding for the 3.x series. I will revert a couple more things here so that this can go into 3.x

Signed-off-by: George Steel <george@net-glue.co.uk>

boesing

Works for me, the only thing which might happen is that there are custom factories provided by upstream factories for the StreamInterface which could be slightly modified to i.e. decorate the StreamInterface by default which won't be the case once the StreamFactoryInterface creates the StreamInterface.

If you need more input on the problem I try to outline, just let me know.
I guess you can adapt the tests regarding the ResponseFactoryInterface, that should outline the issue quite good.

src/Middleware/ImplicitHeadMiddleware.php

src/Middleware/ImplicitHeadMiddlewareFactory.php

boesing · 2023-03-29T16:22:12Z

the stream factory was still outstanding for the 3.x series

That was probably my fault. I did not properly detected the callable in #23 :-/

…Middleware The decorator enables a StreamFactoryInterface type internally whilst preserving BC with callable stream factories. Signed-off-by: George Steel <george@net-glue.co.uk>

… the deprecated callable Signed-off-by: George Steel <george@net-glue.co.uk>

src/Middleware/ImplicitHeadMiddleware.php

src/Middleware/ImplicitOptionsMiddleware.php

src/Middleware/MethodNotAllowedMiddleware.php

ghostwriter · 2023-04-22T23:58:06Z

review is based on the assumption that this PR will target v4.

gsteel · 2023-04-23T07:16:14Z

@ghostwriter

review is based on the assumption that this PR will target v4.

This patch is targeting 3.x - In v4, callable will be dropped along with the decorators

@boesing - when you get a chance, can you review again please?

boesing · 2023-04-23T20:37:08Z

Can you probably adapt the unit tests from the other PRs we've already created?
I have created some weird edge-case tests to verify we do not oversee something.

I havent had a full look on this as I currently dont want to spend time on non-SM or psalm stuff.
https://github.com/mezzio/mezzio-authorization/pull/11/files#diff-2a887fd425446ffddd77e23296b0972d2d60b1692d7151e1ade245217fc7d5d6 has some special checks regarding decorators and stuff, not quite sure if its worth to adapt these changes here or if we are good to go.
Maybe some1 else can have a look on this or you take some more time until I've finished with SM v4. My next target would've been mezzio v4 AFAIR.

gsteel · 2023-04-23T22:24:38Z

Pretty confident everything is covered - the StreamFactory only has 1 responsibility here and that is to generate an empty stream for implicit HEAD requests.

Perhaps @weierophinney or @Ocramius could review to get this one over the line - we'll need it to get v4 out the door right?

weierophinney

Functionality looks sane, and tests cover the changes. 🚢

Ocramius

🚢 LGTM, thanks @gsteel!

Ocramius · 2023-04-24T14:11:01Z

src/Stream/CallableStreamFactoryDecorator.php

+        throw new RuntimeException('This method will not be implemented');
+    }
+
+    /** @inheritDoc */


We should probably document @returns never?

Ocramius · 2023-04-24T14:11:03Z

src/Stream/CallableStreamFactoryDecorator.php

+        return ($this->streamFactory)();
+    }
+
+    /** @inheritDoc */


We should probably document @returns never?

gsteel added BC Break Enhancement New feature or request labels Mar 29, 2023

gsteel added this to the 4.0.0 milestone Mar 29, 2023

gsteel requested review from Ocramius, boesing and weierophinney March 29, 2023 09:19

gsteel added 3 commits March 29, 2023 12:07

Add PSR-17 Support

d93d797

Replaces all callable parameters and properties with `StreamFactoryInterface` and `ResponseFactoryInterface` where appropriate and removes the callable compatability layer. Closes mezzio#29 Signed-off-by: George Steel <george@net-glue.co.uk>

Revert "Add PSR-17 Support"

46bbd4a

This reverts commit c127396. Signed-off-by: George Steel <george@net-glue.co.uk>

Add support for StreamFactoryInterface and type-hint on `ResponseFa…

b7a1522

…ctoryInterface` where appropriate Signed-off-by: George Steel <george@net-glue.co.uk>

gsteel force-pushed the psr-17 branch from eb92c98 to b7a1522 Compare March 29, 2023 11:07

Revert change in removal target version

0b17fbb

Signed-off-by: George Steel <george@net-glue.co.uk>

gsteel modified the milestones: 4.0.0, 3.16.0 Mar 29, 2023

gsteel changed the title ~~Add PSR-17 Support~~ Add PSR-17 StreamFactoryInterface Support in ImplicitHeadMiddleware Mar 29, 2023

gsteel removed the BC Break label Mar 29, 2023

gsteel assigned boesing Mar 29, 2023

boesing requested changes Mar 29, 2023

View reviewed changes

src/Middleware/ImplicitHeadMiddleware.php Outdated Show resolved Hide resolved

src/Middleware/ImplicitHeadMiddleware.php Outdated Show resolved Hide resolved

src/Middleware/ImplicitHeadMiddlewareFactory.php Outdated Show resolved Hide resolved

gsteel added 2 commits March 31, 2023 10:10

Add a decorator for callable stream factory arguments to ImplicitHead…

08ed660

…Middleware The decorator enables a StreamFactoryInterface type internally whilst preserving BC with callable stream factories. Signed-off-by: George Steel <george@net-glue.co.uk>

Amend factory to preserve existing 3.x series behaviour by preferring…

72d74a0

… the deprecated callable Signed-off-by: George Steel <george@net-glue.co.uk>

gsteel requested a review from boesing March 31, 2023 09:33

ghostwriter reviewed Apr 22, 2023

View reviewed changes

src/Middleware/ImplicitHeadMiddleware.php Show resolved Hide resolved

src/Middleware/ImplicitOptionsMiddleware.php Show resolved Hide resolved

src/Middleware/MethodNotAllowedMiddleware.php Show resolved Hide resolved

weierophinney approved these changes Apr 24, 2023

View reviewed changes

Ocramius approved these changes Apr 24, 2023

View reviewed changes

Ocramius assigned Ocramius and unassigned boesing Apr 24, 2023

Ocramius merged commit 49d9ada into mezzio:3.16.x Apr 24, 2023

gsteel deleted the psr-17 branch April 24, 2023 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PSR-17 `StreamFactoryInterface` Support in `ImplicitHeadMiddleware` #48

Add PSR-17 `StreamFactoryInterface` Support in `ImplicitHeadMiddleware` #48

gsteel commented Mar 29, 2023

froschdesign commented Mar 29, 2023

gsteel commented Mar 29, 2023

boesing commented Mar 29, 2023

gsteel commented Mar 29, 2023

boesing commented Mar 29, 2023 •

edited

Loading

gsteel commented Mar 29, 2023

boesing left a comment

boesing commented Mar 29, 2023

ghostwriter commented Apr 22, 2023

gsteel commented Apr 23, 2023

boesing commented Apr 23, 2023 •

edited

Loading

gsteel commented Apr 23, 2023

weierophinney left a comment

Ocramius left a comment

Ocramius Apr 24, 2023

Ocramius Apr 24, 2023

Add PSR-17 StreamFactoryInterface Support in ImplicitHeadMiddleware #48

Add PSR-17 StreamFactoryInterface Support in ImplicitHeadMiddleware #48

Conversation

gsteel commented Mar 29, 2023

Description

froschdesign commented Mar 29, 2023

gsteel commented Mar 29, 2023

boesing commented Mar 29, 2023

gsteel commented Mar 29, 2023

boesing commented Mar 29, 2023 • edited Loading

gsteel commented Mar 29, 2023

boesing left a comment

Choose a reason for hiding this comment

boesing commented Mar 29, 2023

ghostwriter commented Apr 22, 2023

gsteel commented Apr 23, 2023

boesing commented Apr 23, 2023 • edited Loading

gsteel commented Apr 23, 2023

weierophinney left a comment

Choose a reason for hiding this comment

Ocramius left a comment

Choose a reason for hiding this comment

Ocramius Apr 24, 2023

Choose a reason for hiding this comment

Ocramius Apr 24, 2023

Choose a reason for hiding this comment

Add PSR-17 `StreamFactoryInterface` Support in `ImplicitHeadMiddleware` #48

Add PSR-17 `StreamFactoryInterface` Support in `ImplicitHeadMiddleware` #48

boesing commented Mar 29, 2023 •

edited

Loading

boesing commented Apr 23, 2023 •

edited

Loading