Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jan 22, 2013

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Jan 18, 2013

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

@vicb
Copy link
Contributor

vicb commented Jan 18, 2013

What about a UT ?

@@ -5,4 +5,5 @@
xsi:schemaLocation="http://symfony.com/schema/routing http://symfony.com/schema/routing/routing-1.0.xsd">

<route id="_proxy" pattern="/_proxy/{_controller}.{_format}" />
<requirement key="_controller">.+?</requirement>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this fixes #6783 because AFAIK it would match _controller = foo; _format = bar.service:blah.format

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So either format matches eagerly then controller could not include a dot. Or controller matches eagerly, then format could not contain a dot.

@vicb
Copy link
Contributor

vicb commented Jan 18, 2013

and the syntax is wrong also !

@gimler
Copy link
Contributor

gimler commented Jan 21, 2013

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". 

@fabpot
Copy link
Member Author

fabpot commented Jan 22, 2013

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.

@gimler
Copy link
Contributor

gimler commented Jan 22, 2013

+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
})

@gimler
Copy link
Contributor

gimler commented Jan 22, 2013

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

+1 for less complexesy

@fabpot
Copy link
Member Author

fabpot commented Jan 22, 2013

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.

@vicb
Copy link
Contributor

vicb commented Jan 22, 2013

It would remove the dependency

Paul leaves this body :)

@@ -4,5 +4,5 @@
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}/for.{_format}" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this would be nicer: /_proxy/{_controller}/{_format}

@Tobion
Copy link
Member

Tobion commented Jan 22, 2013

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.

@fabpot fabpot mentioned this pull request Jan 22, 2013
1 task
@fabpot
Copy link
Member Author

fabpot commented Jan 22, 2013

@Tobion: see #6829

…r the proxy route (closes symfony#6783)

A controller name can be a service and service names can contain dots.
@fabpot
Copy link
Member Author

fabpot commented Jan 22, 2013

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

@Tobion
Copy link
Member

Tobion commented Jan 22, 2013

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

@mvrhov
Copy link

mvrhov commented Jan 22, 2013

AFAIK yes, at least I used Namespace/SubController more then once...

fabpot added a commit that referenced this pull request Jan 22, 2013
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...
@fabpot fabpot merged commit cdf1d72 into symfony:master Jan 22, 2013
@stof
Copy link
Member

stof commented Jan 22, 2013

@mvrhov the namespace separator in PHP is \, not /

@vicb
Copy link
Contributor

vicb commented Jan 22, 2013

@stof $controller = str_replace('/', '\\', $controller);

fabpot added a commit that referenced this pull request Jan 23, 2013
This PR was merged into the master branch.

Commits
-------

23f5145 renamed proxy to router_proxy
e5135f6 [HttpKernel] renamed path to _path to avoid collision
3193a90 made the proxy path configurable
ad82893 removed the need for a proxy route for rendering strategies
b9f0e17 [HttpKernel] made the Request required when using rendering strategies

Discussion
----------

Content renderer simplification

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

Todo:

  - [x] submit a PR for documentation update

The first commit makes the Request required when dealing with rendering strategies (see the commit why this was a bad idea to make it optional in the first place).

The second commit removes the need for a proxy route and replaces it with the same system we have in the security component.

The third commit makes the proxy path configurable (default to `/_proxy`).

This PR has been triggered by a discussion on #6791.

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

by fabpot at 2013-01-22T09:49:37Z

My opinion:

  * The first commit should be merged.

  * For the second and third one, I don't have a strong opinion. One of the benefits that the content renderer and its strategies do not rely on the Routing component anymore.

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

by fabpot at 2013-01-22T15:22:47Z

Any comments? ping @Tobion @vicb

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

by Tobion at 2013-01-22T16:07:15Z

Shouldn't the class name be like `SubRequestRenderingStrategyInterface` because currently it does not say anything about what is rendered. `RenderingStrategyInterface` makes it look like it's for rendering a normal master request, i.e. templating.

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

by fabpot at 2013-01-22T16:19:26Z

@Tobion: This was actually the first name I had but I found it too long. It is indeed rendering a normal request, but only in the context of a master request.

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

by Tobion at 2013-01-22T16:23:25Z

I found the correct term for what this is about: http://en.wikipedia.org/wiki/Transclusion
So it should probably be like `Symfony/Component/HttpKernel/Transclusion/TransclusionInterface`
or `TransclusionStrategyInterface`.
So the term `rendering` is misleading as it does not really render the subrequest, but only prints a reference to the subrequest. The rendering is done by ESI processor or hinclude etc.

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

by fabpot at 2013-01-22T16:37:00Z

The `RenderingStrategyInterface` does render a request and returns a Response. One of the strategy consist of returning an ESI tag, but this is still a Response.

I don't like introducing the `Transclusion` word as (at least for me) it does not evoke anything.

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

by Tobion at 2013-01-22T16:46:10Z

Also `DefaultRenderingStrategy` is not saying anything. What is default? It should express that it directly includes the resource in the other (term `integrate` comes to my mind).

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

by kriswallsmith at 2013-01-22T17:23:21Z

How about `InlineRenderingStrategy`?

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

by vicb at 2013-01-22T17:25:17Z

@Tobion @kriswallsmith 👍

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

by kriswallsmith at 2013-01-22T17:26:17Z

Also, `SubRequestStrategyInterface` may be more apparent (`InlineSubRequestStrategy`, `EsiSubRequestStrategy`…)

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

by Tobion at 2013-01-22T18:10:19Z

`SubRequestStrategyInterface` is missing the verb somehow. A strategy for what? @kriswallsmith as an English native speaker, is transclusion also not convenient for you?

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

by fabpot at 2013-01-22T18:11:41Z

Thanks for all your suggestions, I appreciate them, but what about the whole approach? Do you agree that it is better than the current one? I'd like to avoid the bikeshedding if the approach is not better.

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

by kriswallsmith at 2013-01-22T18:22:47Z

:+1: for removing the router dependency.

@Tobion perhaps request is the verb there?

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

by Tobion at 2013-01-22T19:51:20Z

I'm also fine with making it independent from the routing component because routing is about making routes configurable with placeholders etc. for nice URLs. But this is not needed for a proxy feature.

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

by kriswallsmith at 2013-01-22T20:13:48Z

@fabpot Do you anticipate ever wanting a sub request to be handled differently based on HTTP method? Just thinking of possible reasons to continue using the routing component here…

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

by fabpot at 2013-01-22T20:40:06Z

No, sub-requests only make sense for GET requests. In fact, we even enforce that in HttpContentRenderer.

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

by fabpot at 2013-01-23T06:51:54Z

I'm not going to discuss names further in the context of this PR as it has already been discussed in the initial PR that introduced these classes and because this PR does not change anything to the meaning of these classes. If you think the names can be better, feel free to open a ticket and discuss names there.

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

by vicb at 2013-01-23T07:48:36Z

If I understand correctly, both hsi and esi will generate path starting with "/_proxy", isn't it a problem wrt access_control ? should it be possible to configure a per strategy path ?

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

by fabpot at 2013-01-23T07:56:11Z

@vicb: Yes, all strategies use the `/_proxy` path when the developer uses a controller reference. But the router proxy takes care of securing the route, so there is no need to do it yourself.

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

by vicb at 2013-01-23T08:07:36Z

@fabpot that's smart, I've missed it.

Some questions though (they should be answered by UT I think - and might already have been, I have not checked)
- Isn't there a pb with urlencoding in the listener ?
- Would the listener work with fragments (`#...') ?

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

by vicb at 2013-01-23T08:31:37Z

Some more points:

- Should we validate that the router_proxy is enabled when esi are enabled (early failure ?)
- Should we be able to enable each strategy individually (ie no need to expose the signer when hsi are not used)

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

by fabpot at 2013-01-23T09:58:45Z

Enabling the router proxy when using ESI si not required. The router proxy is "only" required when you use the `controller` Twig function (or the equivalent in PHP -> `ControllerReference`). But we can probably throw an exception in the `ControllerReference` constructor if the proxy is not enabled.

Enabling each strategy individually is indeed a good idea (and that's more a general question as we could do the same for the translator loaders, or the service container loaders). Let's create another issue on this global topic.

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

by vicb at 2013-01-23T10:10:29Z

> But we can probably throw an exception in the ControllerReference constructor if the proxy is not enabled.

It should probably be in a wrapper class then ?

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

by fabpot at 2013-01-23T12:45:36Z

The listener does not need to work with fragments as URLs as they are never part of the generated URL.
@soerenmartius
Copy link

hey.. i am on symfony 2.2 beta1. When I try to render a controller from a template i got the exception:

An exception has been thrown during the rendering of a template ("No route found for "GET /_proxy"") in AnchorbrandsMessageBundle:Message:Show/show.html.twig at line 19.

It worked before the proxy.xml was removed

@stof
Copy link
Member

stof commented Jan 23, 2013

@caramba13337 in 2.2-beta1, the controller syntax was not working at all. The PR has been merged after that for beta2

@soerenmartius
Copy link

@stof i ve set symfony requirement to 2.2* in my composer.json so what is the correct one to get the fixed vendors?

@csanquer
Copy link
Contributor

I have the same exception with propel bundle and symfony 2.2 beta 2 :

An exception has been thrown during the rendering of a template ("No route found for "GET /_proxy"") in PropelBundle:Collector:propel.html.twig at line 110

@soerenmartius
Copy link

Charles: put router_proxy: { path: /_proxy } to the framework section
in your config.yml. correct syntax is described here:
http://symfony.com/doc/current/book/templating.html#embedding-controllers

2013/1/24 Charles SANQUER notifications@github.com

I have the same exception with propel bundle and symfony 2.2 beta 2 :

An exception has been thrown during the rendering of a template ("No route found for "GET /_proxy"") in PropelBundle:Collector:propel.html.twig at line 110


Reply to this email directly or view it on GitHubhttps://github.com//pull/6791#issuecomment-12648618.

@csanquer
Copy link
Contributor

Thx @caramba1337.

Nice to see the master branch documentation has been updated.

@chrisnoden
Copy link

I'm new to Symfony. I have 2.2.1 and when using the DB profiler (with Propel) I get the error: An exception has been thrown during the rendering of a template ("No route found for "GET Panel:configuration"") in PropelBundle:Collector:propel.html.twig at line 107.
500 Internal Server Error - Twig_Error_Runtime
2 linked Exceptions: NotFoundHttpException » ResourceNotFoundException »

So I added to my config.yml, framework section:
router_proxy: { path: /_proxy }

Now I get:
InvalidConfigurationException: Unrecognized options "router_proxy" under "framework"

@stof
Copy link
Member

stof commented Apr 11, 2013

@chrisnoden it is fragments, not router_proxy

@chrisnoden
Copy link

Old -> fragments: ~
New -> fragments: { path: /_proxy }

Either way, it doesn't fix the propel db profiler. I still get the Twig_Error_Runtime and the previously mentioned message.

@stof
Copy link
Member

stof commented Apr 13, 2013

@chrisnoden are you using the latest PropelBundle ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants