ENHANCEMENT: Improving Cookie class to allow for extendability

[fatlewis]

Previous to this the Cookie class has been very inflexible (cookies are
all set using the static Cookie::set() and so the functionality is not
extendable). Cookie class has been adjusted so extension is now a
possibility for those wishing to alter its functionality. (Improves
compliance to the Law of Demeter)

[dhensby]

Perfect for us EU types! Up vote!

[wilr]

Perhaps it would be better to use the 3.0 DI framework for creating the cookie class? http://api.silverstripe.org/3.0/framework/injector/Injector.html

[dhensby]

To be honest, it's based on the way session is implemented, it makes little sense for it to implement injector from what I can see.

[halkyon]

Looks okay to me. Although I would've thought it should act more like Requirements class. So in that case you'd have something like a Cookie_SessionBackend class which does the current session cookie work, leaving Cookie class as the accessor class to the currently used backend. This is the same pattern the Requirements class uses.

Anyone have any other thoughts?

The Injector idea @WillRossi suggested was probably for the way you can set the Cookie backend class as an alternative to the static variable?

[dhensby]

@halkyon yep, agreed. Lots of ways to implement this, however the route of being as similar to Session was intended to keep code consistency.

[fatlewis]

Agree with everything Dan's said. There are other ways that this can be done, but it is modelled on the way Session is already implemented to conform as best as possible with the current state of the framework.

One of the issues at present is that core classes rely on the Cookie static methods, so cookies are set before Silverstripe even begins to execute a developer's custom code. It's pretty hard to override this and prevent any cookies from being set unless you go about it in a hacky manner (and these changes would solve that problem).

[ajshort]

I'm not really a fan of this approach - I personally would prefer to have a separate class and use the new injector stuff rather than having to namespace all the methods with inst_. The session class was implemented before the new Injector API was available, so I think it would be better to use this. One other possibility would be to attach the cookie class to the request and response objects, since they are really part of the request and response. Thoughts?

[dhensby]

@ajshort unless my understanding of the injector class is completely wrong, I don't see how this would work. Can you give some kind of idea of how using the injector methodology.

Using the injector methodology would also require a lot of changes of where cookies are being set. This fix is incredibly unobtrusive, no other changes other than to the Cookie class are required and all old code can be ported with no fuss. I think that is it's strongest quality and why it should be accepted. At least until someone else can come up with a 'better' way to do it.

[willmorgan]

I think we should remember what exactly we are wrapping. Essentially, we are wrapping setcookie. That's all.

The solution Matt and Dan have produced is lightweight enough, and consistent with Session's architecture. I don't think implementing full blown injector methodologies here is proportional to this case.

This solves an important problem if you want to be legal in the EU, and is completely unobtrusive. In any case, the API should not change regardless of how one intends to implement the guts of the changes. So as Dan said, unless someone else can code a better, accepted, solution, I see no objective reason not to accept it.

But if we're going to stall, I would question why I should bother writing patches at all... :rolleyes:

[wilr]

@willmorgan this is just the standard approach, patches undergo review hence why we use pull requests :). It's not a criticism, it just allows each dev to chip in.

I'll merge if but beware of the wrath of chillu. I would expect this to get refactored along with session in the near future.

[dhensby]

@WillRossi Thanks for pulling it in. I'll handle @chillu !

Here @BetterBrief we're more than happy to see it get re-factored, but we think this is a great solution for the immidiate that is unobtrusive and in line with the current API. No developers need to change their Cookie implementation to make this work, and that's great. So only people that WANT to modify the Cookie behaviour need to change anything.

Any other implementation would mean that users would have to modify their code so that it would work as it did before.

PS: well done @fatlewis for your first SS core contribution!!! WHOOP!

[wilr]

@dhensby this broke the tests - http://teamcity.silverstripe.org/viewLog.html?buildId=8791&buildTypeId=bt27&tab=buildChangesDiv

Can you review and submit a fix.

[ajshort]

The instance methods still use static variables and method calls, it would be good to fix this as well.

[tractorcow]

This broke my framework lib due to duplicate function naming. I changed line 89 from:

    protected function report_errors() {

to:

    protected function inst_report_errors() {

to fix.

[wilr]

Have also pushed c91e855 to fix the builds.

[fatlewis]

Well spotted @tractorcow, thanks. Looks like it's been fixed by @WillRossi now, thanks to you as well!