Added View layer

[Seldaek]

This is obviously up for discussion, once/if we reach an agreement I will document the methods & write tests.

Example usage is available in the FrameworkBundle\View\DefaultView class.

Lukas will send a mail to the ml soon to explain a bit what we did and have a discussion with everyone there.

[beberlei]

First overview => nice! But can you make the DefaultView have an interface?

[lsmith77]

This addresses the remaining issue from http://pooteeweet.org/blog/1850

[lsmith77]

@beberlei: Yeah, I think actually that many internals classes in Symfony2 should get interfaces (request, response, etc.)

[Seldaek]

Of course I can do that (same as testing & docs, once we agree on the implementation). Also if you have ideas of some methods that should be renamed just say so.

[lsmith77]

btw .. from the examples its not immediately obvious, but the redirect() is now split in two methods, one for internal and one for external redirects. furthermore there is handling for redirect*() for non HTML requests.

[fabpot]

I don't see how the split of the redirect() method is useful in this context? Am I missing something? If not, can we remove this change, so that the pull request is really just about the View?

[lsmith77]

the separation is important since in order to cleanly handle redirects() in non HTML cases we implemented an automatic "forwarding" to the target route for internal routes. in the external case we default to just returning the url for now, though this is probably a broken use case. but due to the separation its easier to also add custom logic to handle the two cases in a sensible manner for the given use case of the developer.

of course one could also use the route matching to try and find if there is an internal route for the given url (which i did in my MultiplexBundle), but that seems like a lot of overhead.

another approach we discussed was using one method but supporting both 2 (url+code) and 3 (route+parameters+code) parameters in setRedirect().

we however decided that a clean separation was easier to work with.

[lsmith77]

oh and btw .. redirect is totally a view "thing" .. IIRC even you agreed on that :)
" redirects are part of the View"
http://trac.symfony-project.org/wiki/IRCLogs20101111

[weaverryan]

The splitting of the redirect both seems unnatural (i.e. why should my application care if I'm redirecting internally or externally?) and quite sensible (i.e. because the redirect is truly internal, and thus looks more like a forward).

I'm barely negative on the idea of the internal vs external redirect, just because it seems like the type of thing that we've been keeping out of Symfony so far. Though not as performant, couldn't the full redirect url be passed back into Router::match() if you wanted to forward instead of redirect?

Btw, I do like the idea of having the redirect in this new view in general - this seems like it'll be much more flexible.

[lsmith77]

Like I said its possible without this separation:
https://github.com/liip/MultiplexBundle/blob/master/Controller/MultiplexController.php#L81

However this also means that with the current implemented default its no longer possible to prevent the magic follow we currently have, aka if you don't want the magic follow one would use setExternalRedirect(). Not sure how frequent that use case will be though, but I could see this be relevant for cases where you just want to fire the JSON request and you dont care about a "proper" reply ("proper" meaning something equivalent to the HTML case).

If this is what is stopping things we can move it out. But honestly I don't see the harm at all and didn't expect this to be controversial. In 99% of the cases we redirect internally and from the API its immediately obvious that if you want to redirect else where that you will have to look for another method. So this API will will make the API easier to use, since you don't have to generateUrl(), which is particularly useful if you don't extend from the Controller base class. So to me this separation is full of win with zero draw backs.

Lets admit it, this redirect() stuff in the HTML view is an ugly hack to a browser problem

[Seldaek]

Basically, for redirects, the current implementation means faster JSON/XML responses (since we don't have to generate an url, then parse it back, then do the internal forward), and less code to write for HTML responses, since you just say which route you want to redirect to, no need to actually call the router->generate() method yourself.

The only issue is if you want to redirect to an arbitrary URI, be it on the current site (typically if you use HTTP_REFERER) or just another site, then yes you have to use ->setExternalRedirect(). Is this really a problem? Maybe it's only a problem of naming the method right? Is ->setUriRedirect() more explicit?

[fabpot]

I'm still not sold on the redirect thing, but everything else looks like a good initial implementation.

I think there is one missing thing though: the possibility to restrict the supported formats. The handle() method signature should probably something like:

handle($parameters, $template = null, $formats = null)

I have changed the order of the first two parameters because the template argument is optional (if you don't want to support the HTML format, you don't need a template). The formats argument can be an array of formats (like array('xml', 'json')) that restricts the supported formats. If null, there is no restriction.

Last but not the least, I think we also need to optionally take into account the HTTP Accept header (useful for pure web services).

[lsmith77]

The proposed API changes seem sensible and at least the order of the two parameters we also discussed and just left as is to make "migration" easier (though that shouldnt yet really be a concern at this stage).

With the format restriction we would then throw a 404 Exception if the format doesnt match?

As for Accept header, I was expecting this to be handled in the layer that currently sets _format in the routing.

Finally for the redirect method split, just to make it clear. You prefer having to write:
// somehow inject and set $this->view and $this->router
return $this->view->setRedirect($this->router->generateUrl('doctrine_user_user_list'));
return $this->view->setRedirect('http://foobar');

Overwriting:
// somehow inject and set $this->view, replacing the router dependencies requires modifying the view layer
return $this->view->setRedirect('doctrine_user_user_list');
return $this->view->setExternalRedirect('http://foobar');

Its obviously more compact for the internal redirect which is what is done much more often. The later means not having to inject a router dependencies into the controller (of course if you want to change the router you can do so with a custom view class). It also means less work to do in the non HTML case. It also semantically clearer as an internal redirect is actually a forward in all cases, except for in HTML, where you want it to be a real redirect because of how browsers work (its really a hack). The semantic differences is illustrated by the difference in how we handle the two cases in the non HTML case (aka forward vs. returning the target URL).

[Seldaek]

@fabpot: I agree with the handle parameter swap, but I don't think you need to restrict types at this level. The route already has the option to do that with a requirements: { _format: (xml|json) } - but well, sure we can support it too in the View layer if you like. I just don't think it's necessary.

Accept headers.. Again I think that should be done at the Router or Request level, and not in the view class. atm the view is just reading the format from the request, so it should be set properly already at that point.

As for the redirect issue, I won't repeat what lukas said, but something that may solve the confusion is if we found another name for internal redirects. Forward is not really correct because it is not always an internal forward, redirect is not really right either because it is only a redirect for HTML (the hack of which lukas speaks btw is just the fact that we mostly redirect to avoid issues with page refreshes after form submits).

If we had some other name for this thing, maybe it would help separate the concept between internal/optimized redirects/forwards and external ones that are really meant to redirect the user anywhere, which is quite a rare case I'd say.

[fabpot]

About the format restriction. The client can restrict the format it wants via the Accept header (several formats) or via the special _format route attribute (only one format). Each resource can then decide which resource to support via the _format requirement (only useful when you use _format in the route pattern), or via something else. And my proposal address this case.

[fabpot]

On a side note, the router is only responsible for the conversion of the path info into request attributes. The fact that we have a _route requirement is just a nice side effect.

[fabpot]

@lukas: my preference is for the first proposal:

$this->view->setRedirect($this->router->generateUrl('doctrine_user_user_list'));
return $this->view->setRedirect('http://foobar');

It's up to you to construct the URL the way you want; you can use the Router or anything else. That should have no impact on the view class.

[lsmith77]

ok, guess we will then have to remove it from this pull request, but to me its not even something that makes remotely sense to do. its simply an inferior approach anyway i try to look at it. i will surely bring it up later on in another pull request hoping that down the line it will become clear what a horrible mistake it would be to leave this separation out. :)

[lsmith77]

so since without the differentiation of internal and external redirects the entire format handling falls apart, we will therefore drop setExternalRedirect(), but add logic to determine if its an internal or an external redirect in setRedirect() aka compare domain and then use the router match routine to find if there is a matching routine and if so assume its an internal redirect, which will lead to a forward in all but the HTML case.

[Seldaek]

I'll submit a new version of this code tomorrow incorporating some of the changes discussed here, then we can maybe rediscuss.

[lsmith77]

Since there is some discussion on IRC, here an overview of the behavior for the two redirect methods in the initial (aka current) proposal (again the method naming is debatable):
setRedirect() with _format = 'html' -> redirect
setRedirect() with _format = 'html' -> forward
setExternalRedirect() with _format = 'html' -> redirect
setExternalRedirect() with _format != 'html' -> return url

Without this separation there would be no way to express what should happen in the format != 'html' case, furthermore if one wants to do a forward it would require comparing the domain in the url with the domain in the current request and asking the router to try and match the path to determine the route (and therefore controller) and parameters to forward to.

I will write an RFC to the list that summarizes the discussion tomorrow to make this a topic at the IRC meeting.

[jmikola]

I was linked to this conversation via some lively #symfony-dev discussion. After looking over the code and comments above, I think there's a miscommunication over what "redirect" means.

Fabien, if I understand correctly, you don't see any reason why one would redirect() to anything other than an absolute URL via Location: in the HTTP 3xx response header. The lowest, and perhaps only, level of redirection.

Assuming we have a View class that is delegated redirect responsibility, an efficient solution to what Lukas/Seldaek are trying to do might be injecting their own View implementation via DIC with an overloaded the View::redirect() method:

• Attempt to resolve the redirect URL to a route and controller
• If resolved, they can proxy the controller invocation and response its response
• Otherwise, do an HTTP redirection (parent::redirect())

This does add significant overhead, considering immediately before redirect() we likely generated an absolute URL from a routing rule. In IRC, ornicar mentioned that most redirects are likely to be internal, so there's some convenience in having a redirect() interface that removes the need for the developer to explicitly call generateUrl(). I wouldn't think of suggesting a return to loose-signature Symfony1 methods, so this should be named something else, such as redirectRoute().

That would be the method for Lukas/Seldaek to overload and add their own logic. By default, Symfony2's standard View class would just proxy a call to $this->redirect($this->generateUrl(...)) (and Router would have to be injected into the View).

The benefits of the extra method would be flexibility for Lukas/Seldaek's admittedly *special" use case, and convenience for common developers (esp. those coming from Symfony1). Downside is an extra method on the brand new View class and having to inject a Router into the View.

[beberlei]

btw, what is the fundamental (Browser) problem here that leads to this internal vs external redirect requirement? I see the whole discussion but nobody wrote about the technical requirement that forces us to do something about it. :-)

[Seldaek]

I explained it shortly in #200 (comment) - Lukas went a bit overboard calling it a browser issue maybe, but the thing is, in most cases you do redirects after form submits to avoid that the user refreshes the page and re-submits the form. The other use cases like redirecting to the canonical for example are not browser-hacks but rather SEO hacks, but again only apply to HTML requests, you don't really want to do a real redirect in json, a forward is more than enough.

[weaverryan]

This seems like a very philosophical issue, which usually (ideally) means that a group in some land (e.g. Java, Rails, etc) has beat it to death and come up with a best-practice. Does anything like that exist? Surely we can't be alone in struggling with this. I do think this view layer solves a very legitimate problem with respect to redirecting (though it's an issue we lived with in sf1) and formats, and I like it.

In general, please just continue to be weary of the beginner and keep this layer present, but "out of their way". This pull request seems to address that nicely (redirect issue aside).

Also, not to complicate things, but the internal redirect behavior is unique to HTML formatted non-XMLHttpRequest (an HTML formatted ajax request would also forward internally).

[fabpot]

Here is how I understand the current code: a nice abstraction for read-only web services; a way to automatically convert an entity/array/... to XML/JSON/HTML/... And I don't see the need for redirection for that.

The next step will probably be the other side of web services: creation and update; you submit an XML/JSON/HTML form and serialize it to the native format of your application (probably a Doctrine entity). For this second step, I can see a need for redirection; but not in this first step.

Am I missing something? Am I way off?

[Seldaek]

We've beaten this to death on irc a few times I think. So I'll quickly refactor some things, update the pull request, then hopefully it's mergeable and we'll rediscuss the controversial stuff later on if it makes sense once we have some application code to show as an example, discussing this further on a theoretical level is just not possible.

[Seldaek]

Updated the patch. If everyone is happy with the API and functionality and naming of it all now, I'll start writing tests and docs.

Changes:

1. The redirects are now done via: setRedirectRoute() and setRedirectUri() - they are just different ways of setting a redirect for convenience (no need to generate the url yourself), but they are functionally equivalent.
2. All formats are now handled in the same way, they all use the handleGeneric method which attests to that. Custom behaviors can be implemented by subclassing and overriding the method.
3. Added a global parameters argument to the constructor. These variables by default are now merged with $parameters when you render a template in transformHtml(). This solves the longstanding question people had about how to retrieve configuration values or common data into their layout for example. This data can not be injected at the controller level because you don't always control the controllers (3rd party bundle) and because it's just painful anyway. The way it's done now you can inject basic configuration values from the application config file which should be sufficient for most use cases. For more complex data injection there are other ways to do it, including template helpers.

P.S.: Thanks for avalanche123 & jmikola & johanness & whoever else was around to help figuring out naming and reading the http specs on redirects etc etc. I hope we can all be happy now.

[avalanche123]

+1

[schmittjoh]

+1

[everzet]

+1

[stloyd]

+1, plus suggestion for CS fix in DefaultView.php: is_null($node) to $node === null, $v to $nodeParentKey, $n to $nodeKey ;-)

[lsmith77]

here is a little example using the latest code to inject both static as well as dynamic parameters into the templates when rendering html
http://pastie.org/1352992

[lsmith77]

An article explaining that javascript cannot handle a 302 redirect and instead the browser will automatically redirect in case the redirect is to another domain:
http://www.comanswer.com/question/how-to-catch-an-expected-and-intended-302-ajax-response

This highlights that for non HTML responses its probably not smart to return a 302 in many cases.

[Seldaek]

Please don't start this again :) It was agreed (though, now that I think of it, you weren't there I think) that people redirecting XHRs outside of their domain sandbox were asking for it, and deserve the error. If the use case really mandates to do things that break the standard, you can still override the handleJson & handleXml calls to handle this in a custom way.

[lsmith77]

i just put it here so it isnt forgotten. but its a real issue. for example facebook requires you redirect to their login page if the access token is out of date, which is of course a redirect to another domain, which means that the use case has nothing to do with being stupid.

[lsmith77]

@jordi: just do we dont forget .. we should also add a supports($format) method

[Seldaek]

@lukas: done

@stloyd: fixed

also rebased on latest upstream

[lsmith77]

great .. could you also make $request optional in the handle method?

[lsmith77]

Since things have stalled with this pull, we created a Bundle:
https://github.com/liip/ViewBundle

The idea is to make it easier for people to try out the view layer and to make it easier to provide feedback. We are still hoping that this view class will eventually make it into core.

[fabpot]

As discussed with Seldaek, the first step is to extract the serialization code to a new Serializer component.

[lsmith77]

Just FYI: I have started a new branch of Liip\ViewBundle using the Serializer at https://github.com/liip/ViewBundle/tree/serializer

[fabpot]

Closing this PR as the new RestBundle replaces this PR.