search

psr7-sessions / storageless

:mailbox_with_mail: storage-less PSR-7 session support
MIT License
648 stars 38 forks source link

Soften the zend-stratigility dependency #53

Closed jschreuder closed 8 years ago

jschreuder commented 8 years ago

Currently zendframework/zend-stratigility is only necessary for the Middleware, which I'm not using personally. This could be softened to a suggested package and only in require-dev probably?

Props on the lib otherwise! Just checking it out and loving how it works.

Ocramius commented 8 years ago

Not removing the dependency unless another middleware specification/interface is provided (for which I'd add a different dependency).

Sorry, I do absolutely not do duck typing.

jschreuder commented 8 years ago

I wasn't suggesting duck typing, I was suggesting that you can use the lib perfectly without pulling in Stratigility. Which I'm doing right now. As such I am now pulling in something I don't use and is thus (in my humble opinion) a soft dependency. It is suggested when used together with it, but not required for the central lib classes to function.

So you could keep SessionMiddleware based on Stratigility while allowing others not to depend on it. And it really wouldn't matter as either (1) someone is already using Stratigility, or (2) someone won't use Stratigility and won't run into the dependency.

(though for those of us not using Stratigility it would also be nice if the Middleware was only a utility Trait with Stratigility specific implementation)

Ocramius commented 8 years ago

If I depend on the interface and it isn't included, the entire thing would just crash

On 12 Sep 2016 15:12, "Jelmer Schreuder" notifications@github.com wrote:

I wasn't suggesting duck typing, I was suggesting that you can use the lib perfectly without pulling in Stratigility. Which I'm doing right now. As such I am now pulling in something I don't use and is thus (in my humble opinion) a soft dependency. It is suggested when used together with it, but not required for the central lib classes to function.

So you could keep SessionMiddleware based on Stratigility while allowing others not to depend on it. And it really wouldn't matter as either (1) someone is already using Stratigility, or (2) someone won't use Stratigility and won't run into the dependency.

(though for those of us not using Stratigility it would also be nice if the Middleware was only a utility Trait with Stratigility specific implementation)

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/Ocramius/PSR7Session/issues/53#issuecomment-246342558, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakLzYcK9ibDJGa8mZELQYmRZiV-Hfks5qpU_QgaJpZM4J6jQq .

jschreuder commented 8 years ago

Not if you don't use it? Or am I wrong and is it used in other classes than the SessionMiddleware class? I don't think I see it anywhere else but I may have missed something...

jschreuder commented 8 years ago

Nevermind, I've pretty much copy pasted the whole thing by now. I may code up a pull-request which makes this suggestion a bit more feasible. Refactoring most of the Middleware into a trait that works with PSR7/SetCookie. And then one implementation that links it to Stratigility but keeps other options open by using the trait.

Ocramius commented 8 years ago

This is security-sensitive code: do not copy-paste it unless you understand all the details of the package, and are prepared to rewrite the copy-pasted code every time we do a release.

Also, yes, the interface is used as simple type safety and BC boundary enforcement.

If I didn't include stratigility, I would have had to provide my own middleware interface, and then this would defeat the BC boundary, since the purpose of requiring an external interface is to lock onto a specific signature defined by @mwop, who is the most knowledgeable person when it comes to PSR-7.

Should stratigility change in major version bump, then we'd have to adapt via a BC break, and that is intentional again.

On 12 Sep 2016 15:21, "Jelmer Schreuder" notifications@github.com wrote:

Nevermind, I've pretty much copy pasted the whole thing by now. I may code up a pull-request which makes this suggestion a bit more feasible. Refactoring most of the Middleware into a trait that works with PSR7/SetCookie. And then one implementation that links it to Stratigility but keeps other options open by using the trait.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/Ocramius/PSR7Session/issues/53#issuecomment-246345018, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakNfLp-KDtgZtVDjNjdbHC_HTvoVBks5qpVHxgaJpZM4J6jQq .

Isinlor commented 8 years ago

@Ocramius Is there any specific reason why Stratigility does not provide package with just the interface?

@jschreuder Maybe you should go to Stratigility folks and ask them to make separate Middelware interface package that Stratigility and PSR7Session can depend on.

jschreuder commented 8 years ago

@Isinlor I'm currently working on something which is pretty much copy-pasted from PHP-FIG's working document for PSR-15 which I prefer over the Stratigility interface. So I'm already preparing to move to an outside interface with multi-vendor support.

Isinlor commented 8 years ago

@jschreuder If I remember well you should be able to create adaptor for Stratigility interface to PSR-15.

Nevertheless I don't see a reason for PSR7Session and all other like-minded middelwares to depend on concrete implementation of Stratigility and not on the interface only. I will create an issue on Stratigility repo as they should be able to do this without BC.

Ocramius commented 8 years ago

@Ocramius Is there any specific reason why Stratigility does not provide package with just the interface?

not sure, but that's a good suggestion indeed! Thanks for opening an issue there, if you do :)

Ocramius commented 8 years ago

Meanwhile, closed/dead-end here: come back to this suggestion once a more widespread middleware specification is ready.

I will NOT allow duck-typing on my code.

Isinlor commented 8 years ago

I added it here: https://github.com/zendframework/zend-stratigility/issues/64 ;)

jschreuder commented 8 years ago

I will NOT allow duck-typing on my code.

Maybe I'm not being clear in what I mean, but I never suggested duck-typing. I just suggested that Stratigility not being available to those who don't use it isn't bad. You don't have to always load all dependencies, some may be optional but fail when not loaded. That's what I meant and that is very much and in every way NOT duck typing.

My suggestion was and remains: Extract everything from SessionMiddleware into a utility/service class that knows how to handle a ServerRequestInterface instance and how to post-process a ResponseInterface instance.

Then you create a SessionMiddleware implementation that has the Stratigility interface (optional for those who need it) and expects the utility object/trait as a dependency. Once you have that others also have the means to write other types of Middlewares without pulling in Stratigility.

Ocramius commented 8 years ago

That's what I meant and that is very much and in every way NOT duck typing.

That still makes it for a non-interfaced final class, which is just trouble.

Extract everything from SessionMiddleware into a utility/service class that knows how to handle a ServerRequestInterface instance and how to post-process a ResponseInterface instance. Then you create a SessionMiddleware implementation that has the Stratigility interface (optional for those who need it) and expects the utility object/trait as a dependency. Once you have that others also have the means to write other types of Middlewares without pulling in Stratigility.

That's the path we went towards when designing the package last year, but in the end:

Requiring the interface and then wrapping the middleware in your own implementation is brutally simple, let's not go down the overengineering way (introducing a lot of mistakes) just to remove an external interface, ending up adding a few internal ones.