diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

merged branch fabpot/proxy-route-fix (PR #6791)

Browse Source 
This PR was merged into the master branch.

Commits
-------

cdf1d72 [FrameworkBundle] fixed requirement of the _controller palceholder for the proxy route (closes #6783)

Discussion
----------

[FrameworkBundle] fixed requirement of the _controller palceholder for the proxy route (closes #6783)

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #6783
| License       | MIT
| Doc PR        | n/a

---------------------------------------------------------------------------

by vicb at 2013-01-18T10:23:06Z

What about a UT ?

---------------------------------------------------------------------------

by vicb at 2013-01-18T11:28:41Z

and the syntax is wrong also !

---------------------------------------------------------------------------

by gimler at 2013-01-21T19:59:57Z

same problem the sonata admin bundle use
```
{% render 'sonata.admin.controller.admin:getShortObjectDescriptionAction' %}
```

rewrite to
```
{% render controller('sonata.admin.controller.admin:getShortObjectDescriptionAction') %}
```
throws
```
An exception has been thrown during the rendering of a template ("Parameter "_controller" for route "_proxy" must match "[^/\.]++" ("sonata.admin.controller.admin:getShortObjectDescriptionAction" given) to generate a corresponding URL.") in "SonataAdminBundle:CRUD:edit.html.twig".
```
with the requirement fix it throws
```
An exception has been thrown during the rendering of a template ("Unable to parse the controller name "sonata".") in "SonataAdminBundle:CRUD:edit.html.twig".
```

---------------------------------------------------------------------------

by fabpot at 2013-01-22T06:40:14Z

ok, I've updated the patch. There is now a static segment (`/for`) between the controller and the format, which should fix the problem.

While thinking about this, there is another option, which might be even better: removing the need for the proxy route altogether and check for a defined path like `/_proxy`. It would remove the dependency on a Url Generator in the rendering strategy, and would not make the router proxy listener any more complex.

---------------------------------------------------------------------------

by gimler at 2013-01-22T07:20:43Z

+1 for me the patch works i will open a PR for sonata doctrine orm bundle
```
{% render controller('sonata.admin.controller.admin:getShortObjectDescriptionAction', {},  {
    'code':     sonata_admin.field_description.associationadmin.code,
    'objectId': sonata_admin.field_description.associationadmin.id(sonata_admin.value),
    'uniqid':   sonata_admin.field_description.associationadmin.uniqid
})
```

---------------------------------------------------------------------------

by gimler at 2013-01-22T07:22:21Z

When the proxy route is nessesary we should add a note into the upgrade guide.

+1 for less complexesy

---------------------------------------------------------------------------

by fabpot at 2013-01-22T08:02:12Z

There is one issue with removing the proxy route: when generating a proxy URL, we need a Request instance, which is not always the case. I'm going to submit another PR to "fix" that first.

---------------------------------------------------------------------------

by vicb at 2013-01-22T08:17:51Z

> It would remove the dependency

Paul leaves this body :)

---------------------------------------------------------------------------

by Tobion at 2013-01-22T08:53:52Z

I don't think removing the proxy route is good. That's the purpose of the routing system to handle generating and matching. Now if you do it manually it will probably show a bad approach to people to handle such stuff.
Also people cannot see what routes are defined explicitly and use tools like `router:debug`.

---------------------------------------------------------------------------

by fabpot at 2013-01-22T09:28:55Z

@Tobion: see #6829

---------------------------------------------------------------------------

by fabpot at 2013-01-22T09:57:57Z

I've again changed the route pattern to avoid any possible problems (even if a controller contains a `/`).

---------------------------------------------------------------------------

by Tobion at 2013-01-22T10:16:03Z

Can a controller contain `/`? It's neither a valid service nor a valid class name or?

---------------------------------------------------------------------------

by mvrhov at 2013-01-22T10:40:26Z

AFAIK yes, at least I used Namespace/SubController more then once...
This commit is contained in:
Fabien Potencier 2013-01-22 12:03:36 +01:00
commit 5d896f60f4
1 changed files with 3 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/Symfony/Bundle/FrameworkBundle/Resources/config/routing/proxy.xml
View File

@ -4,5 +4,7 @@
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://symfony.com/schema/routing http://symfony.com/schema/routing/routing-1.0.xsd">
<route id="_proxy" pattern="/_proxy/{_controller}.{_format}" />
<route id="_proxy" pattern="/_proxy/controller/{_controller}/format/{_format}">
<requirement key="_controller">.+</requirement>
</route>
</routes>