dflydev / dflydev-fig-cookies

Cookies for PSR-7 HTTP Message Interface.
MIT License
224 stars 29 forks source link

Making this more easily useable #6

Closed designermonkey closed 8 years ago

designermonkey commented 9 years ago

I really like this implementation, but I feel when using something like a Pimple container, it is a little complex to use.

What would make this really user friendly would be if FigRequestCookies and FigResponseCookies had an all method each that did the same as Cookies::fromRequest and Cookies::fromResponse

What do you think?

simensen commented 9 years ago

How would having an all method that did the same thing as fromRequest and fromResponse make it easier to use from a container? I think I'm probably misunderstanding something about how you'd like to see it used.

Right now the easiest way to use this would be to not use them from a container at all as the facade classes are meant to just be used directly (static methods) as the classes don't make sense outside of the context of having a request or response to begin w/.

Perhaps if you could tell me more about your use case / how you would like to use it / how it currently isn't able to do what you'd like I'd be able to better figure out a solution that would make your life easier.

Thanks for the feedback!

designermonkey commented 9 years ago

I'm using Slim PHP 3 and trying to come up with a cookies implementation that will have to be stored inside the container. Using static classes like this in a DI app is bad practice (not preaching to you by any means).

As it currently is, I would have request-cookies and response-cookies in the container which return instances of the classes I mention. Then they can be used in object context, not static which fits the design principal at work with Slim.

The only thing missing is the ability to get a package of all the current cookies from the headers in that object context, without making a wrapper class.

simensen commented 9 years ago

I see! So the two things you mentioned were completely separate. 1) You like it but it is awkward to use w/ a container and 2) You want an all method so you can get all of Cookies or SetCookies. Is that correct?

I'd not be opposed to adding some sort of all method to get all of the Cookies and SetCookies from a request or response. If you want to add it / prototype it and send a PR I'd happily take a look.

I'd probably go w/ getAll as that maps better with what is inside Cookies and SetCookies since both of those classes already have a getAll method.

designermonkey commented 9 years ago

I hadn't realised that the whole thing was static methods when I wrote the original issue, sorry.

simensen commented 9 years ago

Is ok. :) If you'd still like to add a getAll method for the outermost facades that would be awesome. I think it would be very useful. :) I don't think I'll take it away from being static methods, though.

And to be fair, I think the primitives (Cookie and SetCookie) themselves are not entirely static but they are entirely immutable so the mutators return new copies of the objects rather than modifying the object itself (much like PSR-7). That still won't help you much, though. :)

designermonkey commented 9 years ago

entirely immutable

which is why I like this :D

When I get a spare minute or few, I'll put something together. Thanks.

simensen commented 8 years ago

Thanks again for the issue. I'm closing this for now as I think that it was resolved (or as resolved as it was going to get) already?