Make "play.mvc.Controller".setContext implementable

[cies]

We currently inherit from Controller. I want to remove some of it's capabilities in some cases (say in case of controllers that are only concerned with API requests). And I want to add some capabilities in other cases.

So I added the PlayContextController interface in between Controller and PlayController which defines the setContext() method, so ActionInvoker will be able to add the context to the controller I implement.

[cies]

Now with the tests passing...

[asolntsev]

@cies do we really need new interface PlayContextController? Why? Why can't we just add method setContext to PlayController?

[cies]

Good question. It's because of this bit of code:

https://github.com/codeborne/replay/pull/223/files#diff-1af1c0b2b21d6cc5bf1ca5c869b5b9d559deaa3c12b29b5675ea0ae2e278b87bL186-L190

I read it meaning "you can have a basic controller without the context being set by the FW, or you can have a rich controller with the context set". I did not want to change behaviour of the FW, so I wanted to keep the basic controller available for whomever may have written code depending on that.

We do not need the basic one (and with a self implementation setContext() method we can make a basic controller ourselves. So I wont mind simplifying it.

TL;DR. I tried to keep the API stable "for others".

[cies]

I can adjust (or feel free to adjust) it. All I want to achieve is more control over what context i'm giving certain controller superclasses as the built-in-RePlay validation, renderArgs and flash is on their way out for us, hence I like not to expose them. Also I want to be able to choose the visibility (public/protected/private) of the controller's request/response/session fields, as we simply pass the controller to the view.

[asolntsev]

as we simply pass the controller to the view

Really? You the whole controller instance to the view? Why?
It seems a very strange and dangerous to me. The whole point of controller layer is to prepare the data for view. @cies

P.S. Let me double-check. Do I correctly understand that you don't like play.mvc.Controller as a base class for controllers, and you want to create your own base class in your project? Which will implement method setContext(ActionContext actionContext) in its own way?

[cies]

Really? You the whole controller instance to the view? Why?

Yes for various reasons. We want to build links in the views, and for that we needs request/response since RePlay deprecated the thread local solution. We also generate links based on ref (KFunction<Result> values) to controller methods (we added some on top of RePlay so we do not use Java/Play1 string refs (like: "@showForm").

I've accepted that the controller and view are not as well decoupled as model is in the MVC split.

P.S. Let me double-check. Do I correctly understand that you don't like play.mvc.Controller as a base class for controllers, and you want to create your own base class in your project? Which will implement method setContext(ActionContext actionContext) in its own way?

Yes, as per this bit of code only instanceof Controller gets the context, and I cannot self-implement setContext():

https://github.com/codeborne/replay/pull/223/files#diff-1af1c0b2b21d6cc5bf1ca5c869b5b9d559deaa3c12b29b5675ea0ae2e278b87bL186-L190

And I'd like to be able to implement it myself.

[asolntsev]

@cies

1. From our experience, automatically generating links from controller methods was rather a bad practice.
   Initially it seemed to be a nice idea to avoid String routes, but later we realized that it caused unexpected changes in public API when developers renamed controller methods, but some party didn't expect the change in URL.

So we ended up in fixed routes as plain text. It doesn't really create a big work for us, but it's stable and predictable.

Anyway, I don't understand why do you need to set some context to controller to be able to modify dynamic links? The links should not depend on any context.

2. the controller and view are not as well decoupled as model is in the MVC split.
   Wait, what do you mean?
   Surely, controller and view must be decoupled. I mean, controller should prepare all the needed data for view, and pass this data to the view. Controller passes the data to the view, not itself.

[cies]

@asolntsev

From our experience, automatically generating links from controller methods was rather a bad practice.
Initially it seemed to be a nice idea to avoid String routes, but later we realized that it caused unexpected changes in public API when developers renamed controller methods, but some party didn't expect the change in URL.

We forbid * in the routing. All our routes are specified written out in full. I went through the pain of this some years ago, so we can have easier API path stability.

I don't understand why do you need to set some context to controller to be able to modify dynamic links?

Those are two separate concerns. I want to be able to set the context myself (AND implement Controller myself) for these reasons:

• I want to deprecate/remove some context: validation (we have our own), flash (rarely used, works weird in Play1/RePlay -- hope to replace it with something else) and renderArgs (we almost removed GroovyTemplates, so then this can be removed as well).
• I want to control the accessibility (private/protected/public) myself in Controller.
• We have no use for most of the methods on Controller, I currently cannot "remove" them.

Surely, controller and view must be decoupled. I mean, controller should prepare all the needed data for view, and pass this data to the view. Controller passes the data to the view, not itself.

I know about ideals, but there's the reality of this working great. Controller action method refs are the way we build links (not based on the horrendously hard to refactor Play1ism strings "some.package.ControllerName.actionName"). So this is the biggest way controllers are known to views. Then links are built by controllers as they know about the request/response needed to build them since RePlay. Then

Myself, I do not worry to much about V and C coupling (I do care about M decoupling); since they are more closely related: they are totally useless without each other anyway. If #256 lands in a release we are not so much reliant on request/response in our views in order to build links with non-deprecated methods. If was because we needed request+response from the controller anyway that we started passing the whole controller along, as we put methods to build link on controller super-classes since the controller has the request+response object.