Hi Lukas, I have already mentioned this in IRC conversation we had yesterday. I have two main problems with this approach:
- Consistency violation - Consistency violation Let's look at browser user registration example: 1. User gets an html form - GET /users/new 2. User fills the form and submits - POST /users 3. If submit successful - goto 5 4. If submit failed - display form with validation errors in response. 5. Redirect user to new user Location with response: Location: /users/1 Now let's convert that use case to json/xml rpc: 1. User reads docs about the format of new user resource 2. Users issues create user request - POST /users 3. If resource created - goto 5 4. If resource creation fails - return 5** response with optional body 5. Return 201 response with Location: /users/1 - to show location of newly created response Now the browser case seems consistent with your proposal, so let's compare your rpc example: 1. User reads docs about the format of new user resource 2. Users issues create user request - POST /users 3. Return response with new user resource or with validation error As you can see - the rpc case is completely different from browser case. Again, I am not saying that my suggested way of doing things is the *only* correct way. You could of course implement the same steps you have in rpc example in browser: 1. User gets an html form - GET /users/new 2. User fills the form and submits - POST /users 3. Return user account page of failed form with error messages One could argue at this point that we save ourselves a round-trip to the server if treat browser differently I could argue that given proper http cache implementation on the server - you actually save yourself caching trouble as the /users/1 url would get cached in your reverse proxy mechanism and browser I think that introducing inconsistencies as such would only cause more misunderstandings and frustration for the users of the framework. Please note, that I am not against *forward* functionality - I just think it should work in a consistent was, no matter what your target client is - less magic = more goodness Also I agree, that having a convenience method to redirect to action without having to generate the url manually ourselves is a worthy addition Just my $0.02 On Wed, Dec 1, 2010 at 8:57 AM, Lukas Kahwe Smith <[email protected]>wrote: > Aloha, > > === Summary === > This has been implemented as part of the view layer pull request already ( > https://github.com/fabpot/symfony/pull/200), but even without the view > layer I would advocate for the addition of a second method to do redirects, > so that in the end there is one method which redirects to any URL and one > that does "redirects" to the same application. > > The goal is to require less code, less required dependencies to inject for > the common use case. Also the ability to "optimize" the redirect to a > forward in certain cases with little overhead. > > === Proposed Change === > Currently when doing a redirect to the application itself something like > the following is needed. When extending from the base Controller class > (which is not a best practice) the code would look like this > $this->redirect($this->generateUrl('doctrine_user_user_list')); > > Without extending from the base Controller the code would look like this > and this is the base line used for the rest of this proposal: > > $this->response->setRedirect($this->router->generateUrl('doctrine_user_user_list')); > > With this proposal there would instead be two methods. For now I will refer > to them as setRedirectX (for the current setRedirect, which accepts as url + > code) and setRedirectY (for a method accepting a route + param + code). The > final method names are open for discussion, but are a bike shedding issue: > > $this->response->setRedirectX($this->router->generateUrl('doctrine_user_user_list')); > $this->response->setRedirectY('doctrine_user_user_list'); > > === Details === > Let's imagine a registration workflow implemented by come controller: > 1) user clicks on a register button: registration form is displayed > 2) user submits the form: user is stored and a page explaining further > steps is displayed > > Now when using a browser in step 2 we actually want to split up things in 2 > steps, because we want to prevent annoying warnings and double submissions > if the user refreshes. We also want to provide a bookmark-able url: > 2a) user submits the form: user is stored and a url to redirect to is > returned > 2b) browser redirects to the returned url: a page explaining further steps > is displayed > > Now when using AJAX to display the form and the explanations in an overlay > we do not necessarily need this 2a) and 2b), we could do 2) as originally > intended. We would thereby safe another round trip. Of course we might still > choose to do 2a) and 2b) because we prefer to be explicit. > > Now trying to implement the above inside a Controller atm requires logic to > detect the actual output format to decide if to use a redirect() or a > forward(). We could alternatively also simply extend the default Response > class to: > - check if the Request _format isn't HTML > - check if the url passed to setRedirect() has the same domain as the > current request > - check if the router can match the path of the passed url > > If any of the checks fail we redirect happens as usual and if all checks > pass we forward to the controller with the parsed parameters from the route > that was matched. > > However there would be no way for a developer to express if he wants to > have the redirect automatically switched to a forward when the target format > is not HTML. So a developer could only either stay with the logic as is, aka > always return a target url when a redirect is done, or always convert a > redirect to a forward in the non HTML case making this entire approach > essentially not feasible. > > By splitting the method in two, we can make the semantics explicit. > Furthermore we do not have to first generate and then parse the URL, which > should improve performance. Finally since the controller would not be > responsible anymore for generating the url in the case when we do want the > redirect to be convert-able into a forward, the code in that case would be > shorter and wouldn't require a dependency on the router anymore. Since most > redirects in applications actually point back to the application, this would > simplify the code in the most common use case. > > So the following: > // always return the redirect URL > > $this->response->setRedirect($this->router->generateUrl('doctrine_user_user_list')); > $this->response->setRedirect('http://foo.com'); > > Could then be written as follows: > // always return the redirect URL > > $this->response->setRedirectX($this->router->generateUrl('doctrine_user_user_list')); > $this->response->setRedirectX('http://foo.com'); > // optionally forward to the route when the request _format isnt HTML > $this->response->setRedirectY('doctrine_user_user_list'); > > Choosing if to convert a redirect to a forward or not would then be easily > implementable inside the Response (or View layer for that matter). The > default behavior could also be up to debate or could be made configurable. > > The way its implemented inside the view layer pull request is as follows: > $this->view->setRedirect() with format = 'html' -> redirect > $this->view->setRedirect() with format != 'html' -> forward > $this->view->setExternalRedirect() with format = 'html' -> redirect > $this->view->setExternalRedirect() with format != 'html' -> return url > > Of course there is also still the possibility to do an explicit forward() > just like before, however for that case there is no different between > _format = 'html' and _format != 'html' and therefore it does not concern > this RFC. > > regards, > Lukas Kahwe Smith > [email protected] > > > > -- > If you want to report a vulnerability issue on symfony, please send it to > security at symfony-project.com > > You received this message because you are subscribed to the Google > Groups "symfony developers" group. > To post to this group, send email to [email protected] > To unsubscribe from this group, send email to > [email protected]<symfony-devs%[email protected]> > For more options, visit this group at > http://groups.google.com/group/symfony-devs?hl=en > -- *Bulat Shakirzyanov* | Software Alchemist *a: *about.me/avalanche123 *e:* [email protected] -- If you want to report a vulnerability issue on symfony, please send it to security at symfony-project.com You received this message because you are subscribed to the Google Groups "symfony developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/symfony-devs?hl=en
