Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Content renderer simplification #6829

Merged
merged 5 commits into from

5 participants

@fabpot
Owner
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:

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

@fabpot
Owner

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.

@fabpot fabpot referenced this pull request from a commit
@fabpot fabpot merged branch fabpot/proxy-route-fix (PR #6791)
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...
5d896f6
@fabpot
Owner

Any comments? ping @Tobion @vicb

@vicb vicb commented on the diff
...Kernel/RenderingStrategy/DefaultRenderingStrategy.php
((22 lines not shown))
$subRequest = Request::create($uri, 'get', array(), $cookies, array(), $server);
- if (null !== $request && $session = $request->getSession()) {
+ if ($session = $request->getSession()) {
@vicb
vicb added a note

Is the condition useful here ?

@stof Collaborator
stof added a note

yes, because you can disable the session entirely in FrameworkBundle

@Tobion Collaborator
Tobion added a note

Also the phpdoc says it can return null.

@vicb
vicb added a note

I still don't see the pb with set(null) ?

@vicb
vicb added a note

ok, now I see :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vicb vicb commented on the diff
...nel/RenderingStrategy/ProxyAwareRenderingStrategy.php
((10 lines not shown))
+ */
+
+namespace Symfony\Component\HttpKernel\RenderingStrategy;
+
+use Symfony\Component\HttpKernel\Controller\ControllerReference;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\EventListener\RouterProxyListener;
+
+/**
+ * Adds the possibility to generate a proxy URI for a given Controller.
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+abstract class ProxyAwareRenderingStrategy implements RenderingStrategyInterface
+{
+ private $proxyPath = '/_proxy';
@vicb
vicb added a note

do you really want to make this private w/o getter ?

@vicb
vicb added a note

Also what about defining a const and use it here ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Tobion
Collaborator

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.

...FrameworkBundle/DependencyInjection/Configuration.php
@@ -114,6 +115,21 @@ private function addEsiSection(ArrayNodeDefinition $rootNode)
;
}
+ private function addProxySection(ArrayNodeDefinition $rootNode)
+ {
+ $rootNode
+ ->children()
+ ->arrayNode('proxy')
+ ->info('proxy configuration')
@vicb
vicb added a note

->addDefaultsIfNotSet() ?

@fabpot Owner
fabpot added a note

Like ESI (and many other things in Symfony), everything that is optional must be explicitly enabled (also because it has some performance overhead).

@vicb
vicb added a note

I agree but then it should be

<?php
->addDefaultsIfNotSet()
->canBeEnabled()
@fabpot Owner
fabpot added a note

This is wrong. The addDefaultsIfNotSet() will always enable the configuration even if it is not present. That's not what we want as explained in my previous comment.

@stof Collaborator
stof added a note

@fabpot you are confusing canBeEnabled and canBeDisabled. In canBeEnabled, the enabled flag defaults to false

@fabpot Owner
fabpot added a note

I've just talked with @vicb about this:

  • the current configuration works just fine
  • the one proposed by Victor does not work (as you need to explicitly set enabled: true to enable it).
@vicb
vicb added a note

Well it works perfectly when I test, I'll submit a PR.

@vicb
vicb added a note

@fabpot please add to your merger guidelines to never ever again merge a PR w/o [updated] UTs. I have to update many things for such a benign change !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...workBundle/DependencyInjection/FrameworkExtension.php
@@ -185,6 +189,20 @@ private function registerEsiConfiguration(array $config, XmlFileLoader $loader)
}
/**
+ * Loads the proxy configuration.
+ *
+ * @param array $config A proxy configuration array
+ * @param XmlFileLoader $loader An XmlFileLoader instance
+ */
+ private function registerProxyConfiguration(array $config, ContainerBuilder $container, XmlFileLoader $loader)
+ {
+ if (!empty($config['enabled'])) {
@vicb
vicb added a note

... and remove !empty()

@fabpot Owner
fabpot added a note

... and see above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...FrameworkBundle/DependencyInjection/Configuration.php
@@ -73,6 +73,7 @@ public function getConfigTreeBuilder()
$this->addFormSection($rootNode);
$this->addEsiSection($rootNode);
+ $this->addProxySection($rootNode);
@vicb
vicb added a note

I don't like "proxy" in the config / extension, could you come with something more specific (we already have "trusted_proxies")

@fabpot Owner
fabpot added a note

I need help here ;)

What about "rendering_proxy", "router_proxy"?

@Tobion Collaborator
Tobion added a note

A router_proxy that does not use the routing component ;)

@vicb
vicb added a note

we already have a "router" key so :-1: for "router_proxy".

How about ->addSubRequestSection() and a new sub_request node with a prefix child?

@fabpot Owner
fabpot added a note

I've renamed proxy to router_proxy. Even if we already have a router entry and even if it does not use the Router component, this is still the best name that describes what it does: it routes a request before the default router.

I'm open to change it again to a better name, but I cannot think of one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabpot
Owner

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

@Tobion
Collaborator

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.

@fabpot
Owner

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.

@Tobion
Collaborator

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

@kriswallsmith

How about InlineRenderingStrategy?

@kriswallsmith

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

@Tobion
Collaborator

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

@fabpot
Owner

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.

...nel/RenderingStrategy/ProxyAwareRenderingStrategy.php
((40 lines not shown))
+ * Generates a proxy URI for a given controller.
+ *
+ * @param ControllerReference $reference A ControllerReference instance
+ * @param Request $request A Request instance
+ *
+ * @return string A proxy URI
+ */
+ protected function generateProxyUri(ControllerReference $reference, Request $request)
+ {
+ if (!isset($reference->attributes['_format'])) {
+ $reference->attributes['_format'] = $request->getRequestFormat();
+ }
+
+ $reference->attributes['_controller'] = $reference->controller;
+
+ $reference->query['path'] = http_build_query($reference->attributes, '', '&');

Change this key to _path?

@fabpot Owner
fabpot added a note

Good catch, done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kriswallsmith

:+1: for removing the router dependency.

@Tobion perhaps request is the verb there?

@Tobion
Collaborator

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.

@kriswallsmith

@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…

@fabpot
Owner

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

@fabpot
Owner

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.

...FrameworkBundle/DependencyInjection/Configuration.php
@@ -114,6 +115,21 @@ private function addEsiSection(ArrayNodeDefinition $rootNode)
;
}
+ private function addRouterProxySection(ArrayNodeDefinition $rootNode)
+ {
+ $rootNode
+ ->children()
+ ->arrayNode('router_proxy')
+ ->info('router proxy configuration')
@vicb
vicb added a note

May be the info should be more specific and indicate that the router is used for the http renderers

@fabpot Owner
fabpot added a note

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vicb vicb commented on the diff
...workBundle/DependencyInjection/FrameworkExtension.php
@@ -185,6 +189,20 @@ private function registerEsiConfiguration(array $config, XmlFileLoader $loader)
}
/**
+ * Loads the router proxy configuration.
+ *
+ * @param array $config A proxy configuration array
+ * @param XmlFileLoader $loader An XmlFileLoader instance
+ */
+ private function registerRouterProxyConfiguration(array $config, ContainerBuilder $container, XmlFileLoader $loader)
+ {
+ if (!empty($config['enabled'])) {
+ $loader->load('proxy.xml');
+ $container->setParameter('http_content_renderer.proxy_path', $config['path']);
@vicb
vicb added a note

router_proxy_path ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ameworkBundle/Resources/config/schema/symfony-1.0.xsd
@@ -12,6 +12,7 @@
<xsd:element name="form" type="form" minOccurs="0" maxOccurs="1" />
<xsd:element name="csrf-protection" type="csrf_protection" minOccurs="0" maxOccurs="1" />
<xsd:element name="esi" type="esi" minOccurs="0" maxOccurs="1" />
+ <xsd:element name="proxy" type="proxy" minOccurs="0" maxOccurs="1" />
@vicb
vicb added a note

router_proxy

@fabpot Owner
fabpot added a note

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ameworkBundle/Resources/config/schema/symfony-1.0.xsd
@@ -44,6 +45,11 @@
<xsd:attribute name="enabled" type="xsd:boolean" />
</xsd:complexType>
+ <xsd:complexType name="proxy">
@vicb
vicb added a note

...and see above

@fabpot Owner
fabpot added a note

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vicb

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 ?

@fabpot
Owner

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

@vicb

@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 (`#...') ?
@vicb

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)
@fabpot
Owner

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.

@vicb

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 ?

@fabpot
Owner

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

@fabpot fabpot referenced this pull request in symfony/symfony-docs
Merged

replaced the proxy route with a listener #2179

fabpot added some commits
@fabpot fabpot [HttpKernel] made the Request required when using rendering strategies
The previous code allowed to pass null as a Request but that does not
really make sense as rendering a sub-request can only happen from a
master request. This was done to ease testing but that was a mistake.
b9f0e17
@fabpot fabpot removed the need for a proxy route for rendering strategies ad82893
@fabpot fabpot made the proxy path configurable 3193a90
@fabpot fabpot [HttpKernel] renamed path to _path to avoid collision e5135f6
@fabpot fabpot renamed proxy to router_proxy 23f5145
@fabpot fabpot referenced this pull request from a commit
@fabpot fabpot merged branch fabpot/content-renderer-request (PR #6829)
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 :+1:

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

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.
877603a
@fabpot fabpot merged commit 23f5145 into symfony:master
@vicb

@fabpot merged ? what about:

  • the config, see my comment above,
  • a changelog,
  • urldecoding & signer in the listener, should I assume this is ok ?
@fabpot
Owner

Indeed, I forgot to update the CHANGELOG (fixed now).
For the config, I've created an issue as this is a larger issue that we have elsewhere in the framework; the implementation here is consistent with other parts of the framework (symfony/symfony#6849) and anyway it has nothing to do with this PR.
The way the listener works has not been changed by the changes introduced in this PR.

@vicb

I am talking about the config issue that is related to this PR.

@fabpot
Owner

Sorry, I must have missed it. Which config issue?

@vicb

I haven't linked it because it should be in the outdated diff, you should be able to find it there.

@fabpot
Owner

I have reopened all outdated comments and I don't see anything that were not taken into account.

@vicb vicb referenced this pull request from a commit in vicb/symfony
@vicb vicb [FrameworkBundle] Fix the DI config
see #6829
728dd93
@fabpot fabpot referenced this pull request from a commit in symfony/symfony-docs
@fabpot fabpot merged branch fabpot/content-renderer-simplification (PR #2179)
This PR was merged into the master branch.

Commits
-------

7ab8be5 replaced the proxy route with a listener

Discussion
----------

replaced the proxy route with a listener

see symfony/symfony#6829
4d23c70
@mmucklo mmucklo referenced this pull request from a commit
@fabpot fabpot merged branch fabpot/proxy-route-fix (PR #6791)
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...
f430edb
@mmucklo mmucklo referenced this pull request from a commit
@fabpot fabpot merged branch fabpot/content-renderer-request (PR #6829)
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 :+1:

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

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.
51ebad5
@cordoval cordoval referenced this pull request from a commit in cordoval/symfony-bootstrap
@vicb vicb Update the security config with the latest Sf2.2 changes
The internal route has been removed, see symfony/symfony#6829 for details.
760d624
@cordoval cordoval referenced this pull request from a commit in cordoval/symfony-bootstrap
@fabpot fabpot merged branch vicb/patch-1 (PR #482)
This PR was merged into the master branch.

Commits
-------

760d624 Update the security config with the latest Sf2.2 changes

Discussion
----------

Update the security config with the latest Sf2.2 changes

The internal route has been removed, see symfony/symfony#6829 for details.
6c817e8
@ondrejmirtes ondrejmirtes referenced this pull request from a commit in ondrejmirtes/symfony
@fabpot fabpot merged branch vicb/fmwk/config (PR #6852)
This PR was squashed before being merged into the master branch (closes #6852).

Commits
-------

fde7585 [DIC] Better handling of enableable configurations

Discussion
----------

[DIC] Better handling of enableable configurations

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no, this feature has not been released yet
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

My definition of bug fix might be discussable. The thing which I think is not discussable is that this PR fixes the semantic - and I think it is important for a "semantic configuration": before this PR, some nodes had `->canBeDisabled` for nodes that were actually disabled by default. Those nodes now have `->canBeEnabled` which sounds right.

**Edit: Jan 28, 2013** - history:

See [the related comments](symfony#6829 (comment)).

I think Symfony **must** get the configuration right as we can expect of lot of devs to use this as a template when writting their own configuration.

@schmittjoh could you please give me your feedback on [this change](https://github.com/symfony/symfony/pull/6852/files#L4R224) considering [the rationale](https://github.com/symfony/symfony/pull/6852/files#L3R7).

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

by stof at 2013-01-23T16:10:33Z

@vicb your links are broken as they are pointing to the PR creation page

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

by stof at 2013-01-23T16:10:55Z

and to create a TODO list, it has to be a list first

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

by vicb at 2013-01-23T16:31:10Z

@stof thanks for reporting the broken links, they are fixed /cc @schmittjoh

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

by vicb at 2013-01-23T16:31:50Z

@Tobion please submit a PR to my repo, I don't have much time to work on this. Thanks !

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

by vicb at 2013-01-25T15:14:47Z

@fabpot @schmittjoh I'd like your feedback on the latest commit, rationale is in the method phpDoc. It better matches what we do now and seem the most sensible thing to do.

edit: with this you can no more disable the node explicitly, I have to find a better solution

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

by schmittjoh at 2013-01-25T15:20:13Z

Looks good.

On Fri, Jan 25, 2013 at 4:15 PM, Victor Berchet <notifications@github.com>wrote:

> @fabpot <https://github.com/fabpot> @schmittjoh<https://github.com/schmittjoh>I'd like your feedback on the latest commit, rationale is in the method
> phpDoc. It better matches what we do now and seem the most sensible thing
> to do.
>
> —
> Reply to this email directly or view it on GitHub<symfony#6852 (comment)>.
>
>

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

by vicb at 2013-01-28T14:37:57Z

@fabpot I know I keep insisting on this one and I am sorry for that but I think this should be considered as a bug fix (see the PR header for details) and should be merged in 2.2. I think the Symfony core should be exemplary as it is used by many developers as a template when creating their own bundle. *This PR is no more a WIP and can be merged right now*.

In addition to fixing the enableable nodes, this PR contain new UTs and some fixes to the code / tests.

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

by fabpot at 2013-01-28T16:43:42Z

@vicb As explained in a comment, this is not a BC break as this feature does not exist in 2.1. So, I can make the change to the CHANGELOG if you want after merging, or I can let you make the change.

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

by vicb at 2013-01-28T16:46:33Z

I am going to change it right now !

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

by vicb at 2013-01-28T16:46:56Z

(and thanks for having checked this)

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

by vicb at 2013-01-28T16:54:37Z

@fabpot I have updated the changelog and the PR header.

I am not sure if the commits should be squashed or not. On one side the multiple commits can help understand the changes but on the other side that's a lot of small commits which could pollute history. I let you choose what to do.
c1037b1
@goabonga goabonga referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@goabonga goabonga referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@fabpot fabpot referenced this pull request from a commit in symfony/FrameworkBundle
@fabpot fabpot merged branch vicb/fmwk/config (PR #6852)
This PR was squashed before being merged into the master branch (closes #6852).

Commits
-------

fde7585 [DIC] Better handling of enableable configurations

Discussion
----------

[DIC] Better handling of enableable configurations

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no, this feature has not been released yet
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

My definition of bug fix might be discussable. The thing which I think is not discussable is that this PR fixes the semantic - and I think it is important for a "semantic configuration": before this PR, some nodes had `->canBeDisabled` for nodes that were actually disabled by default. Those nodes now have `->canBeEnabled` which sounds right.

**Edit: Jan 28, 2013** - history:

See [the related comments](symfony/symfony#6829 (comment)).

I think Symfony **must** get the configuration right as we can expect of lot of devs to use this as a template when writting their own configuration.

@schmittjoh could you please give me your feedback on [this change](https://github.com/symfony/symfony/pull/6852/files#L4R224) considering [the rationale](https://github.com/symfony/symfony/pull/6852/files#L3R7).

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

by stof at 2013-01-23T16:10:33Z

@vicb your links are broken as they are pointing to the PR creation page

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

by stof at 2013-01-23T16:10:55Z

and to create a TODO list, it has to be a list first

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

by vicb at 2013-01-23T16:31:10Z

@stof thanks for reporting the broken links, they are fixed /cc @schmittjoh

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

by vicb at 2013-01-23T16:31:50Z

@Tobion please submit a PR to my repo, I don't have much time to work on this. Thanks !

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

by vicb at 2013-01-25T15:14:47Z

@fabpot @schmittjoh I'd like your feedback on the latest commit, rationale is in the method phpDoc. It better matches what we do now and seem the most sensible thing to do.

edit: with this you can no more disable the node explicitly, I have to find a better solution

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

by schmittjoh at 2013-01-25T15:20:13Z

Looks good.

On Fri, Jan 25, 2013 at 4:15 PM, Victor Berchet <notifications@github.com>wrote:

> @fabpot <https://github.com/fabpot> @schmittjoh<https://github.com/schmittjoh>I'd like your feedback on the latest commit, rationale is in the method
> phpDoc. It better matches what we do now and seem the most sensible thing
> to do.
>
> —
> Reply to this email directly or view it on GitHub<symfony/symfony#6852 (comment)>.
>
>

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

by vicb at 2013-01-28T14:37:57Z

@fabpot I know I keep insisting on this one and I am sorry for that but I think this should be considered as a bug fix (see the PR header for details) and should be merged in 2.2. I think the Symfony core should be exemplary as it is used by many developers as a template when creating their own bundle. *This PR is no more a WIP and can be merged right now*.

In addition to fixing the enableable nodes, this PR contain new UTs and some fixes to the code / tests.

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

by fabpot at 2013-01-28T16:43:42Z

@vicb As explained in a comment, this is not a BC break as this feature does not exist in 2.1. So, I can make the change to the CHANGELOG if you want after merging, or I can let you make the change.

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

by vicb at 2013-01-28T16:46:33Z

I am going to change it right now !

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

by vicb at 2013-01-28T16:46:56Z

(and thanks for having checked this)

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

by vicb at 2013-01-28T16:54:37Z

@fabpot I have updated the changelog and the PR header.

I am not sure if the commits should be squashed or not. On one side the multiple commits can help understand the changes but on the other side that's a lot of small commits which could pollute history. I let you choose what to do.
fdaec5f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 23, 2013
  1. @fabpot

    [HttpKernel] made the Request required when using rendering strategies

    fabpot authored
    The previous code allowed to pass null as a Request but that does not
    really make sense as rendering a sub-request can only happen from a
    master request. This was done to ease testing but that was a mistake.
  2. @fabpot
  3. @fabpot
  4. @fabpot
  5. @fabpot

    renamed proxy to router_proxy

    fabpot authored
This page is out of date. Refresh to see the latest.
Showing with 275 additions and 329 deletions.
  1. +13 −8 src/Symfony/Bridge/Twig/Tests/Extension/HttpKernelExtensionTest.php
  2. +16 −0 src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
  3. +18 −0 src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
  4. +3 −9 src/Symfony/Bundle/FrameworkBundle/Resources/config/content_generator.xml
  5. +1 −1  src/Symfony/Bundle/FrameworkBundle/Resources/config/esi.xml
  6. +18 −0 src/Symfony/Bundle/FrameworkBundle/Resources/config/proxy.xml
  7. +0 −10 src/Symfony/Bundle/FrameworkBundle/Resources/config/routing/proxy.xml
  8. +6 −0 src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd
  9. +13 −5 src/Symfony/Component/HttpKernel/EventListener/RouterProxyListener.php
  10. +7 −3 src/Symfony/Component/HttpKernel/HttpContentRenderer.php
  11. +9 −14 src/Symfony/Component/HttpKernel/RenderingStrategy/DefaultRenderingStrategy.php
  12. +3 −3 src/Symfony/Component/HttpKernel/RenderingStrategy/EsiRenderingStrategy.php
  13. +0 −84 src/Symfony/Component/HttpKernel/RenderingStrategy/GeneratorAwareRenderingStrategy.php
  14. +2 −2 src/Symfony/Component/HttpKernel/RenderingStrategy/HIncludeRenderingStrategy.php
  15. +59 −0 src/Symfony/Component/HttpKernel/RenderingStrategy/ProxyAwareRenderingStrategy.php
  16. +1 −1  src/Symfony/Component/HttpKernel/RenderingStrategy/RenderingStrategyInterface.php
  17. +12 −30 src/Symfony/Component/HttpKernel/Tests/EventListener/RouterProxyListenerTest.php
  18. +13 −1 src/Symfony/Component/HttpKernel/Tests/HttpContentRendererTest.php
  19. +0 −29 src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/AbstractRenderingStrategyTest.php
  20. +6 −7 src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/DefaultRenderingStrategyTest.php
  21. +3 −8 src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/EsiRenderingStrategyTest.php
  22. +0 −101 src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/GeneratorAwareRenderingStrategyTest.php
  23. +9 −13 src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/HIncludeRenderingStrategyTest.php
  24. +63 −0 src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/ProxyAwareRenderingStrategyTest.php
View
21 src/Symfony/Bridge/Twig/Tests/Extension/HttpKernelExtensionTest.php
@@ -13,6 +13,7 @@
use Symfony\Bridge\Twig\Extension\HttpKernelExtension;
use Symfony\Bridge\Twig\Tests\TestCase;
+use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\HttpContentRenderer;
@@ -29,13 +30,6 @@ protected function setUp()
}
}
- public function testRenderWithoutMasterRequest()
- {
- $kernel = $this->getHttpContentRenderer($this->returnValue(new Response('foo')));
-
- $this->assertEquals('foo', $this->renderTemplate($kernel));
- }
-
/**
* @expectedException \Twig_Error_Runtime
*/
@@ -56,7 +50,18 @@ protected function getHttpContentRenderer($return)
$strategy->expects($this->once())->method('getName')->will($this->returnValue('default'));
$strategy->expects($this->once())->method('render')->will($return);
- return new HttpContentRenderer(array($strategy));
+ // simulate a master request
+ $event = $this->getMockBuilder('Symfony\Component\HttpKernel\Event\GetResponseEvent')->disableOriginalConstructor()->getMock();
+ $event
+ ->expects($this->once())
+ ->method('getRequest')
+ ->will($this->returnValue(Request::create('/')))
+ ;
+
+ $renderer = new HttpContentRenderer(array($strategy));
+ $renderer->onKernelRequest($event);
+
+ return $renderer;
}
protected function renderTemplate(HttpContentRenderer $renderer, $template = '{{ render("foo") }}')
View
16 src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
@@ -73,6 +73,7 @@ public function getConfigTreeBuilder()
$this->addFormSection($rootNode);
$this->addEsiSection($rootNode);
+ $this->addRouterProxySection($rootNode);
$this->addProfilerSection($rootNode);
$this->addRouterSection($rootNode);
$this->addSessionSection($rootNode);
@@ -114,6 +115,21 @@ private function addEsiSection(ArrayNodeDefinition $rootNode)
;
}
+ private function addRouterProxySection(ArrayNodeDefinition $rootNode)
+ {
+ $rootNode
+ ->children()
+ ->arrayNode('router_proxy')
+ ->info('proxy configuration for the HTTP content renderer')
+ ->canBeDisabled()
+ ->children()
+ ->scalarNode('path')->defaultValue('/_proxy')->end()
+ ->end()
+ ->end()
+ ->end()
+ ;
+ }
+
private function addProfilerSection(ArrayNodeDefinition $rootNode)
{
$rootNode
View
18 src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
@@ -94,6 +94,10 @@ public function load(array $configs, ContainerBuilder $container)
$this->registerEsiConfiguration($config['esi'], $loader);
}
+ if (isset($config['router_proxy'])) {
+ $this->registerRouterProxyConfiguration($config['router_proxy'], $container, $loader);
+ }
+
if (isset($config['profiler'])) {
$this->registerProfilerConfiguration($config['profiler'], $container, $loader);
}
@@ -185,6 +189,20 @@ private function registerEsiConfiguration(array $config, XmlFileLoader $loader)
}
/**
+ * Loads the router proxy configuration.
+ *
+ * @param array $config A proxy configuration array
+ * @param XmlFileLoader $loader An XmlFileLoader instance
+ */
+ private function registerRouterProxyConfiguration(array $config, ContainerBuilder $container, XmlFileLoader $loader)
+ {
+ if (!empty($config['enabled'])) {
+ $loader->load('proxy.xml');
+ $container->setParameter('http_content_renderer.proxy_path', $config['path']);
@vicb
vicb added a note

router_proxy_path ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
+ }
+
+ /**
* Loads the profiler configuration.
*
* @param array $config A profiler configuration array
View
12 src/Symfony/Bundle/FrameworkBundle/Resources/config/content_generator.xml
@@ -9,7 +9,7 @@
<parameter key="http_content_renderer.strategy.default.class">Symfony\Component\HttpKernel\RenderingStrategy\DefaultRenderingStrategy</parameter>
<parameter key="http_content_renderer.strategy.hinclude.class">Symfony\Component\HttpKernel\RenderingStrategy\HIncludeRenderingStrategy</parameter>
<parameter key="http_content_renderer.strategy.hinclude.global_template"></parameter>
- <parameter key="http_content_renderer.listener.router_proxy.class">Symfony\Component\HttpKernel\EventListener\RouterProxyListener</parameter>
+ <parameter key="http_content_renderer.proxy_path">/_proxy</parameter>
</parameters>
<services>
@@ -22,7 +22,7 @@
<service id="http_content_renderer.strategy.default" class="%http_content_renderer.strategy.default.class%">
<tag name="kernel.content_renderer_strategy" />
<argument type="service" id="http_kernel" />
- <call method="setUrlGenerator"><argument type="service" id="router" /></call>
+ <call method="setProxyPath"><argument>%http_content_renderer.proxy_path%</argument></call>
</service>
<service id="http_content_renderer.strategy.hinclude" class="%http_content_renderer.strategy.hinclude.class%">
@@ -30,13 +30,7 @@
<argument type="service" id="templating" on-invalid="null" />
<argument type="service" id="uri_signer" />
<argument>%http_content_renderer.strategy.hinclude.global_template%</argument>
- <call method="setUrlGenerator"><argument type="service" id="router" /></call>
- </service>
-
-<!-- FIXME: make the listener registration optional via a configuration setting? -->
- <service id="http_content_renderer.listener.router_proxy" class="%http_content_renderer.listener.router_proxy.class%">
- <tag name="kernel.event_subscriber" />
- <argument type="service" id="uri_signer" />
+ <call method="setProxyPath"><argument>%http_content_renderer.proxy_path%</argument></call>
</service>
</services>
View
2  src/Symfony/Bundle/FrameworkBundle/Resources/config/esi.xml
@@ -22,7 +22,7 @@
<tag name="kernel.content_renderer_strategy" />
<argument type="service" id="esi" />
<argument type="service" id="http_content_renderer.strategy.default" />
- <call method="setUrlGenerator"><argument type="service" id="router" /></call>
+ <call method="setProxyPath"><argument>%http_content_renderer.proxy_path%</argument></call>
</service>
</services>
</container>
View
18 src/Symfony/Bundle/FrameworkBundle/Resources/config/proxy.xml
@@ -0,0 +1,18 @@
+<?xml version="1.0" ?>
+
+<container xmlns="http://symfony.com/schema/dic/services"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
+
+ <parameters>
+ <parameter key="http_content_renderer.listener.router_proxy.class">Symfony\Component\HttpKernel\EventListener\RouterProxyListener</parameter>
+ </parameters>
+
+ <services>
+ <service id="http_content_renderer.listener.router_proxy" class="%http_content_renderer.listener.router_proxy.class%">
+ <tag name="kernel.event_subscriber" />
+ <argument type="service" id="uri_signer" />
+ <argument>%http_content_renderer.proxy_path%</argument>
+ </service>
+ </services>
+</container>
View
10 src/Symfony/Bundle/FrameworkBundle/Resources/config/routing/proxy.xml
@@ -1,10 +0,0 @@
-<?xml version="1.0" encoding="UTF-8" ?>
-
-<routes xmlns="http://symfony.com/schema/routing"
- 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/{_controller}/format/{_format}">
- <requirement key="_controller">.+</requirement>
- </route>
-</routes>
View
6 src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd
@@ -12,6 +12,7 @@
<xsd:element name="form" type="form" minOccurs="0" maxOccurs="1" />
<xsd:element name="csrf-protection" type="csrf_protection" minOccurs="0" maxOccurs="1" />
<xsd:element name="esi" type="esi" minOccurs="0" maxOccurs="1" />
+ <xsd:element name="router-proxy" type="router-proxy" minOccurs="0" maxOccurs="1" />
<xsd:element name="profiler" type="profiler" minOccurs="0" maxOccurs="1" />
<xsd:element name="router" type="router" minOccurs="0" maxOccurs="1" />
<xsd:element name="session" type="session" minOccurs="0" maxOccurs="1" />
@@ -44,6 +45,11 @@
<xsd:attribute name="enabled" type="xsd:boolean" />
</xsd:complexType>
+ <xsd:complexType name="router-proxy">
+ <xsd:attribute name="enabled" type="xsd:boolean" />
+ <xsd:attribute name="path" type="xsd:string" />
+ </xsd:complexType>
+
<xsd:complexType name="profiler">
<xsd:all>
<xsd:element name="matcher" type="profiler_matcher" minOccurs="0" maxOccurs="1" />
View
18 src/Symfony/Component/HttpKernel/EventListener/RouterProxyListener.php
@@ -30,10 +30,18 @@
class RouterProxyListener implements EventSubscriberInterface
{
private $signer;
+ private $proxyPath;
- public function __construct(UriSigner $signer)
+ /**
+ * Constructor.
+ *
+ * @param UriSigner $signer A UriSigner instance
+ * @param string $proxyPath The path that triggers this listener
+ */
+ public function __construct(UriSigner $signer, $proxyPath = '/_proxy')
{
$this->signer = $signer;
+ $this->proxyPath = $proxyPath;
}
/**
@@ -47,16 +55,16 @@ public function onKernelRequest(GetResponseEvent $event)
{
$request = $event->getRequest();
- if ('_proxy' !== $request->attributes->get('_route')) {
+ if ($this->proxyPath !== rawurldecode($request->getPathInfo())) {
return;
}
$this->validateRequest($request);
- parse_str($request->query->get('path', ''), $attributes);
+ parse_str($request->query->get('_path', ''), $attributes);
$request->attributes->add($attributes);
$request->attributes->set('_route_params', array_replace($request->attributes->get('_route_params', array()), $attributes));
- $request->query->remove('path');
+ $request->query->remove('_path');
}
protected function validateRequest(Request $request)
@@ -92,7 +100,7 @@ protected function getLocalIpAddresses()
public static function getSubscribedEvents()
{
return array(
- KernelEvents::REQUEST => array(array('onKernelRequest', 16)),
+ KernelEvents::REQUEST => array(array('onKernelRequest', 48)),
);
}
}
View
10 src/Symfony/Component/HttpKernel/HttpContentRenderer.php
@@ -23,7 +23,13 @@
/**
* Renders a URI using different strategies.
*
+ * This class handles sub-requests. The response content from the sub-request
+ * is then embedded into a master request. The handling of the sub-request
+ * is managed by rendering strategies.
+ *
* @author Fabien Potencier <fabien@symfony.com>
+ *
+ * @see RenderingStrategyInterface
*/
class HttpContentRenderer implements EventSubscriberInterface
{
@@ -103,9 +109,7 @@ public function render($uri, $strategy = 'default', array $options = array())
throw new \InvalidArgumentException(sprintf('The "%s" rendering strategy does not exist.', $strategy));
}
- $request = $this->requests ? $this->requests[0] : null;
-
- return $this->deliver($this->strategies[$strategy]->render($uri, $request, $options));
+ return $this->deliver($this->strategies[$strategy]->render($uri, $this->requests[0], $options));
}
/**
View
23 src/Symfony/Component/HttpKernel/RenderingStrategy/DefaultRenderingStrategy.php
@@ -21,7 +21,7 @@
*
* @author Fabien Potencier <fabien@symfony.com>
*/
-class DefaultRenderingStrategy extends GeneratorAwareRenderingStrategy
+class DefaultRenderingStrategy extends ProxyAwareRenderingStrategy
{
private $kernel;
@@ -42,7 +42,7 @@ public function __construct(HttpKernelInterface $kernel)
*
* * alt: an alternative URI to render in case of an error
*/
- public function render($uri, Request $request = null, array $options = array())
+ public function render($uri, Request $request, array $options = array())
{
if ($uri instanceof ControllerReference) {
$uri = $this->generateProxyUri($uri, $request);
@@ -74,21 +74,16 @@ public function render($uri, Request $request = null, array $options = array())
}
}
- protected function createSubRequest($uri, Request $request = null)
+ protected function createSubRequest($uri, Request $request)
{
- if (null !== $request) {
- $cookies = $request->cookies->all();
- $server = $request->server->all();
-
- // the sub-request is internal
- $server['REMOTE_ADDR'] = '127.0.0.1';
- } else {
- $cookies = array();
- $server = array();
- }
+ $cookies = $request->cookies->all();
+ $server = $request->server->all();
+
+ // the sub-request is internal
+ $server['REMOTE_ADDR'] = '127.0.0.1';
$subRequest = Request::create($uri, 'get', array(), $cookies, array(), $server);
- if (null !== $request && $session = $request->getSession()) {
+ if ($session = $request->getSession()) {
@vicb
vicb added a note

Is the condition useful here ?

@stof Collaborator
stof added a note

yes, because you can disable the session entirely in FrameworkBundle

@Tobion Collaborator
Tobion added a note

Also the phpdoc says it can return null.

@vicb
vicb added a note

I still don't see the pb with set(null) ?

@vicb
vicb added a note

ok, now I see :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
$subRequest->setSession($session);
}
View
6 src/Symfony/Component/HttpKernel/RenderingStrategy/EsiRenderingStrategy.php
@@ -21,7 +21,7 @@
*
* @author Fabien Potencier <fabien@symfony.com>
*/
-class EsiRenderingStrategy extends GeneratorAwareRenderingStrategy
+class EsiRenderingStrategy extends ProxyAwareRenderingStrategy
{
private $esi;
private $defaultStrategy;
@@ -55,9 +55,9 @@ public function __construct(Esi $esi, RenderingStrategyInterface $defaultStrateg
*
* @see Symfony\Component\HttpKernel\HttpCache\ESI
*/
- public function render($uri, Request $request = null, array $options = array())
+ public function render($uri, Request $request, array $options = array())
{
- if (null === $request || !$this->esi->hasSurrogateEsiCapability($request)) {
+ if (!$this->esi->hasSurrogateEsiCapability($request)) {
return $this->defaultStrategy->render($uri, $request, $options);
}
View
84 src/Symfony/Component/HttpKernel/RenderingStrategy/GeneratorAwareRenderingStrategy.php
@@ -1,84 +0,0 @@
-<?php
-
-/*
- * This file is part of the Symfony package.
- *
- * (c) Fabien Potencier <fabien@symfony.com>
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Symfony\Component\HttpKernel\RenderingStrategy;
-
-use Symfony\Component\HttpKernel\Controller\ControllerReference;
-use Symfony\Component\HttpFoundation\Request;
-use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
-use Symfony\Component\Routing\Exception\RouteNotFoundException;
-
-/**
- * Adds the possibility to generate a proxy URI for a given Controller.
- *
- * @author Fabien Potencier <fabien@symfony.com>
- */
-abstract class GeneratorAwareRenderingStrategy implements RenderingStrategyInterface
-{
- protected $generator;
-
- /**
- * Sets a URL generator to use for proxy URIs generation.
- *
- * @param UrlGeneratorInterface $generator An UrlGeneratorInterface instance
- */
- public function setUrlGenerator(UrlGeneratorInterface $generator)
- {
- $this->generator = $generator;
- }
-
- /**
- * Generates a proxy URI for a given controller.
- *
- * This method only works when using the Symfony Routing component and
- * if a "_proxy" route is defined with a {_controller} and {_format}
- * placeholders.
- *
- * @param ControllerReference $reference A ControllerReference instance
- * @param Request $request A Request instance
- *
- * @return string A proxy URI
- *
- * @throws \LogicException when the _proxy route is not available
- * @throws \LogicException when there is no registered route generator
- */
- protected function generateProxyUri(ControllerReference $reference, Request $request = null)
- {
- if (null === $this->generator) {
- throw new \LogicException('Unable to generate a proxy URL as there is no registered route generator.');
- }
-
- if (isset($reference->attributes['_format'])) {
- $format = $reference->attributes['_format'];
- unset($reference->attributes['_format']);
- } elseif (null !== $request) {
- $format = $request->getRequestFormat();
- } else {
- $format = 'html';
- }
-
- try {
- $uri = $this->generator->generate('_proxy', array('_controller' => $reference->controller, '_format' => $format), UrlGeneratorInterface::ABSOLUTE_URL);
- } catch (RouteNotFoundException $e) {
- throw new \LogicException('Unable to generate a proxy URL as the "_proxy" route is not registered.', 0, $e);
- }
-
- if ($path = http_build_query($reference->attributes, '', '&')) {
- $reference->query['path'] = $path;
- }
-
- if ($qs = http_build_query($reference->query, '', '&')) {
- $uri .= '?'.$qs;
- }
-
- return $uri;
- }
-}
View
4 src/Symfony/Component/HttpKernel/RenderingStrategy/HIncludeRenderingStrategy.php
@@ -22,7 +22,7 @@
*
* @author Fabien Potencier <fabien@symfony.com>
*/
-class HIncludeRenderingStrategy extends GeneratorAwareRenderingStrategy
+class HIncludeRenderingStrategy extends ProxyAwareRenderingStrategy
{
private $templating;
private $globalDefaultTemplate;
@@ -53,7 +53,7 @@ public function __construct($templating = null, UriSigner $signer = null, $globa
*
* * default: The default content (it can be a template name or the content)
*/
- public function render($uri, Request $request = null, array $options = array())
+ public function render($uri, Request $request, array $options = array())
{
if ($uri instanceof ControllerReference) {
if (null === $this->signer) {
View
59 src/Symfony/Component/HttpKernel/RenderingStrategy/ProxyAwareRenderingStrategy.php
@@ -0,0 +1,59 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel\RenderingStrategy;
+
+use Symfony\Component\HttpKernel\Controller\ControllerReference;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\EventListener\RouterProxyListener;
+
+/**
+ * Adds the possibility to generate a proxy URI for a given Controller.
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+abstract class ProxyAwareRenderingStrategy implements RenderingStrategyInterface
+{
+ private $proxyPath = '/_proxy';
@vicb
vicb added a note

do you really want to make this private w/o getter ?

@vicb
vicb added a note

Also what about defining a const and use it here ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ /**
+ * Sets the proxy path that triggers the proxy listener
+ *
+ * @param string $path The path
+ *
+ * @see RouterProxyListener
+ */
+ public function setProxyPath($path)
+ {
+ $this->proxyPath = $path;
+ }
+
+ /**
+ * Generates a proxy URI for a given controller.
+ *
+ * @param ControllerReference $reference A ControllerReference instance
+ * @param Request $request A Request instance
+ *
+ * @return string A proxy URI
+ */
+ protected function generateProxyUri(ControllerReference $reference, Request $request)
+ {
+ if (!isset($reference->attributes['_format'])) {
+ $reference->attributes['_format'] = $request->getRequestFormat();
+ }
+
+ $reference->attributes['_controller'] = $reference->controller;
+
+ $reference->query['_path'] = http_build_query($reference->attributes, '', '&');
+
+ return $request->getUriForPath($this->proxyPath.'?'.http_build_query($reference->query, '', '&'));
+ }
+}
View
2  src/Symfony/Component/HttpKernel/RenderingStrategy/RenderingStrategyInterface.php
@@ -32,7 +32,7 @@
*
* @return Response A Response instance
*/
- public function render($uri, Request $request = null, array $options = array());
+ public function render($uri, Request $request, array $options = array());
/**
* Gets the name of the strategy.
View
42 src/Symfony/Component/HttpKernel/Tests/EventListener/RouterProxyListenerTest.php
@@ -28,17 +28,17 @@ protected function setUp()
public function testOnlyTriggeredOnProxyRoute()
{
- $request = Request::create('http://example.com/foo?path=foo%3D=bar');
+ $request = Request::create('http://example.com/foo?_path=foo%3Dbar%26_controller%3Dfoo');
$listener = new RouterProxyListener(new UriSigner('foo'));
- $event = $this->createGetResponseEvent($request, 'foobar');
+ $event = $this->createGetResponseEvent($request);
$expected = $request->attributes->all();
$listener->onKernelRequest($event);
$this->assertEquals($expected, $request->attributes->all());
- $this->assertTrue($request->query->has('path'));
+ $this->assertTrue($request->query->has('_path'));
}
/**
@@ -46,7 +46,7 @@ public function testOnlyTriggeredOnProxyRoute()
*/
public function testAccessDeniedWithNonSafeMethods()
{
- $request = Request::create('http://example.com/foo', 'POST');
+ $request = Request::create('http://example.com/_proxy', 'POST');
$listener = new RouterProxyListener(new UriSigner('foo'));
$event = $this->createGetResponseEvent($request);
@@ -59,7 +59,7 @@ public function testAccessDeniedWithNonSafeMethods()
*/
public function testAccessDeniedWithNonLocalIps()
{
- $request = Request::create('http://example.com/foo', 'GET', array(), array(), array(), array('REMOTE_ADDR' => '10.0.0.1'));
+ $request = Request::create('http://example.com/_proxy', 'GET', array(), array(), array(), array('REMOTE_ADDR' => '10.0.0.1'));
$listener = new RouterProxyListener(new UriSigner('foo'));
$event = $this->createGetResponseEvent($request);
@@ -72,7 +72,7 @@ public function testAccessDeniedWithNonLocalIps()
*/
public function testAccessDeniedWithWrongSignature()
{
- $request = Request::create('http://example.com/foo', 'GET', array(), array(), array(), array('REMOTE_ADDR' => '10.0.0.1'));
+ $request = Request::create('http://example.com/_proxy', 'GET', array(), array(), array(), array('REMOTE_ADDR' => '10.0.0.1'));
$listener = new RouterProxyListener(new UriSigner('foo'));
$event = $this->createGetResponseEvent($request);
@@ -80,40 +80,22 @@ public function testAccessDeniedWithWrongSignature()
$listener->onKernelRequest($event);
}
- public function testWithSignatureAndNoPath()
- {
- $signer = new UriSigner('foo');
- $request = Request::create($signer->sign('http://example.com/foo'), 'GET', array(), array(), array(), array('REMOTE_ADDR' => '10.0.0.1'));
-
- $listener = new RouterProxyListener($signer);
- $event = $this->createGetResponseEvent($request);
-
- $listener->onKernelRequest($event);
-
- $this->assertEquals(array('foo' => 'foo'), $request->attributes->get('_route_params'));
- $this->assertFalse($request->query->has('path'));
- }
-
- public function testWithSignatureAndPath()
+ public function testWithSignature()
{
$signer = new UriSigner('foo');
- $request = Request::create($signer->sign('http://example.com/foo?path=bar%3Dbar'), 'GET', array(), array(), array(), array('REMOTE_ADDR' => '10.0.0.1'));
+ $request = Request::create($signer->sign('http://example.com/_proxy?_path=foo%3Dbar%26_controller%3Dfoo'), 'GET', array(), array(), array(), array('REMOTE_ADDR' => '10.0.0.1'));
$listener = new RouterProxyListener($signer);
$event = $this->createGetResponseEvent($request);
$listener->onKernelRequest($event);
- $this->assertEquals(array('foo' => 'foo', 'bar' => 'bar'), $request->attributes->get('_route_params'));
- $this->assertFalse($request->query->has('path'));
+ $this->assertEquals(array('foo' => 'bar', '_controller' => 'foo'), $request->attributes->get('_route_params'));
+ $this->assertFalse($request->query->has('_path'));
}
- private function createGetResponseEvent(Request $request, $route = '_proxy')
+ private function createGetResponseEvent(Request $request)
{
- $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface');
- $request->attributes->set('_route', $route);
- $request->attributes->set('_route_params', array('foo' => 'foo'));
-
- return new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST);
+ return new GetResponseEvent($this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'), $request, HttpKernelInterface::MASTER_REQUEST);
}
}
View
14 src/Symfony/Component/HttpKernel/Tests/HttpContentRendererTest.php
@@ -12,6 +12,7 @@
namespace Symfony\Component\HttpKernel\Tests;
use Symfony\Component\HttpKernel\HttpContentRenderer;
+use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
class HttpContentRendererTest extends \PHPUnit_Framework_TestCase
@@ -34,6 +35,8 @@ public function testRenderWhenStrategyDoesNotExist()
public function testRender()
{
+ $request = Request::create('/');
+
$strategy = $this->getMock('Symfony\Component\HttpKernel\RenderingStrategy\RenderingStrategyInterface');
$strategy
->expects($this->any())
@@ -43,13 +46,22 @@ public function testRender()
$strategy
->expects($this->any())
->method('render')
- ->with('/', null, array('foo' => 'foo', 'ignore_errors' => true))
+ ->with('/', $request, array('foo' => 'foo', 'ignore_errors' => true))
->will($this->returnValue(new Response('foo')))
;
$renderer = new HttpContentRenderer();
$renderer->addStrategy($strategy);
+ // simulate a master request
+ $event = $this->getMockBuilder('Symfony\Component\HttpKernel\Event\GetResponseEvent')->disableOriginalConstructor()->getMock();
+ $event
+ ->expects($this->once())
+ ->method('getRequest')
+ ->will($this->returnValue(Request::create('/')))
+ ;
+ $renderer->onKernelRequest($event);
+
$this->assertEquals('foo', $renderer->render('/', 'foo', array('foo' => 'foo')));
}
View
29 src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/AbstractRenderingStrategyTest.php
@@ -1,29 +0,0 @@
-<?php
-
-/*
- * This file is part of the Symfony package.
- *
- * (c) Fabien Potencier <fabien@symfony.com>
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Symfony\Component\HttpKernel\Tests\RenderingStrategy;
-
-abstract class AbstractRenderingStrategyTest extends \PHPUnit_Framework_TestCase
-{
- protected function getUrlGenerator()
- {
- $generator = $this->getMock('Symfony\Component\Routing\Generator\UrlGeneratorInterface');
- $generator
- ->expects($this->any())
- ->method('generate')
- ->will($this->returnCallback(function ($name, $parameters, $referenceType) {
- return '/'.$parameters['_controller'].'.'.$parameters['_format'];
- }))
- ;
-
- return $generator;
- }
-}
View
13 src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/DefaultRenderingStrategyTest.php
@@ -18,7 +18,7 @@
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\EventDispatcher\EventDispatcher;
-class DefaultRenderingStrategyTest extends AbstractRenderingStrategyTest
+class DefaultRenderingStrategyTest extends \PHPUnit_Framework_TestCase
{
protected function setUp()
{
@@ -35,15 +35,14 @@ public function testRender()
{
$strategy = new DefaultRenderingStrategy($this->getKernel($this->returnValue(new Response('foo'))));
- $this->assertEquals('foo', $strategy->render('/')->getContent());
+ $this->assertEquals('foo', $strategy->render('/', Request::create('/'))->getContent());
}
public function testRenderWithControllerReference()
{
$strategy = new DefaultRenderingStrategy($this->getKernel($this->returnValue(new Response('foo'))));
- $strategy->setUrlGenerator($this->getUrlGenerator());
- $this->assertEquals('foo', $strategy->render(new ControllerReference('main_controller', array(), array()))->getContent());
+ $this->assertEquals('foo', $strategy->render(new ControllerReference('main_controller', array(), array()), Request::create('/'))->getContent());
}
/**
@@ -53,14 +52,14 @@ public function testRenderExceptionNoIgnoreErrors()
{
$strategy = new DefaultRenderingStrategy($this->getKernel($this->throwException(new \RuntimeException('foo'))));
- $this->assertEquals('foo', $strategy->render('/')->getContent());
+ $this->assertEquals('foo', $strategy->render('/', Request::create('/'))->getContent());
}
public function testRenderExceptionIgnoreErrors()
{
$strategy = new DefaultRenderingStrategy($this->getKernel($this->throwException(new \RuntimeException('foo'))));
- $this->assertEmpty($strategy->render('/', null, array('ignore_errors' => true))->getContent());
+ $this->assertEmpty($strategy->render('/', Request::create('/'), array('ignore_errors' => true))->getContent());
}
public function testRenderExceptionIgnoreErrorsWithAlt()
@@ -70,7 +69,7 @@ public function testRenderExceptionIgnoreErrorsWithAlt()
$this->returnValue(new Response('bar'))
)));
- $this->assertEquals('bar', $strategy->render('/', null, array('ignore_errors' => true, 'alt' => '/foo'))->getContent());
+ $this->assertEquals('bar', $strategy->render('/', Request::create('/'), array('ignore_errors' => true, 'alt' => '/foo'))->getContent());
}
private function getKernel($returnValue)
View
11 src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/EsiRenderingStrategyTest.php
@@ -16,23 +16,19 @@
use Symfony\Component\HttpKernel\HttpCache\Esi;
use Symfony\Component\HttpFoundation\Request;
-class EsiRenderingStrategyTest extends AbstractRenderingStrategyTest
+class EsiRenderingStrategyTest extends \PHPUnit_Framework_TestCase
{
protected function setUp()
{
if (!class_exists('Symfony\Component\HttpFoundation\Request')) {
$this->markTestSkipped('The "HttpFoundation" component is not available');
}
-
- if (!interface_exists('Symfony\Component\Routing\Generator\UrlGeneratorInterface')) {
- $this->markTestSkipped('The "Routing" component is not available');
- }
}
public function testRenderFallbackToDefaultStrategyIfNoRequest()
{
$strategy = new EsiRenderingStrategy(new Esi(), $this->getDefaultStrategy(true));
- $strategy->render('/');
+ $strategy->render('/', Request::create('/'));
}
public function testRenderFallbackToDefaultStrategyIfEsiNotSupported()
@@ -44,7 +40,6 @@ public function testRenderFallbackToDefaultStrategyIfEsiNotSupported()
public function testRender()
{
$strategy = new EsiRenderingStrategy(new Esi(), $this->getDefaultStrategy());
- $strategy->setUrlGenerator($this->getUrlGenerator());
$request = Request::create('/');
$request->headers->set('Surrogate-Capability', 'ESI/1.0');
@@ -52,7 +47,7 @@ public function testRender()
$this->assertEquals('<esi:include src="/" />', $strategy->render('/', $request)->getContent());
$this->assertEquals("<esi:comment text=\"This is a comment\" />\n<esi:include src=\"/\" />", $strategy->render('/', $request, array('comment' => 'This is a comment'))->getContent());
$this->assertEquals('<esi:include src="/" alt="foo" />', $strategy->render('/', $request, array('alt' => 'foo'))->getContent());
- $this->assertEquals('<esi:include src="/main_controller.html" alt="/alt_controller.html" />', $strategy->render(new ControllerReference('main_controller', array(), array()), $request, array('alt' => new ControllerReference('alt_controller', array(), array())))->getContent());
+ $this->assertEquals('<esi:include src="http://localhost/_proxy?_path=_format%3Dhtml%26_controller%3Dmain_controller" alt="http://localhost/_proxy?_path=_format%3Dhtml%26_controller%3Dalt_controller" />', $strategy->render(new ControllerReference('main_controller', array(), array()), $request, array('alt' => new ControllerReference('alt_controller', array(), array())))->getContent());
}
private function getDefaultStrategy($called = false)
View
101 ...fony/Component/HttpKernel/Tests/RenderingStrategy/GeneratorAwareRenderingStrategyTest.php
@@ -1,101 +0,0 @@
-<?php
-
-/*
- * This file is part of the Symfony package.
- *
- * (c) Fabien Potencier <fabien@symfony.com>
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Symfony\Component\HttpKernel\Tests\RenderingStrategy;
-
-use Symfony\Component\HttpFoundation\Request;
-use Symfony\Component\HttpKernel\Controller\ControllerReference;
-use Symfony\Component\HttpKernel\RenderingStrategy\GeneratorAwareRenderingStrategy;
-use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
-use Symfony\Component\Routing\Exception\RouteNotFoundException;
-
-class GeneratorAwareRenderingStrategyTest extends AbstractRenderingStrategyTest
-{
- protected function setUp()
- {
- if (!interface_exists('Symfony\Component\Routing\Generator\UrlGeneratorInterface')) {
- $this->markTestSkipped('The "Routing" component is not available');
- }
- }
-
- /**
- * @expectedException \LogicException
- */
- public function testGenerateProxyUriWithNoGenerator()
- {
- $strategy = new Strategy();
- $strategy->doGenerateProxyUri(new ControllerReference('controller', array(), array()));
- }
-
- /**
- * @expectedException \LogicException
- */
- public function testGenerateProxyUriWhenRouteNotFound()
- {
- $generator = $this->getMock('Symfony\Component\Routing\Generator\UrlGeneratorInterface');
- $generator
- ->expects($this->once())
- ->method('generate')
- ->will($this->throwException(new RouteNotFoundException()))
- ;
-
- $strategy = new Strategy();
- $strategy->setUrlGenerator($generator);
- $strategy->doGenerateProxyUri(new ControllerReference('controller', array(), array()));
- }
-
- /**
- * @dataProvider getGeneratorProxyUriData
- */
- public function testGenerateProxyUri($uri, $controller)
- {
- $this->assertEquals($uri, $this->getStrategy()->doGenerateProxyUri($controller));
- }
-
- public function getGeneratorProxyUriData()
- {
- return array(
- array('/controller.html', new ControllerReference('controller', array(), array())),
- array('/controller.xml', new ControllerReference('controller', array('_format' => 'xml'), array())),
- array('/controller.json?path=foo%3Dfoo', new ControllerReference('controller', array('foo' => 'foo', '_format' => 'json'), array())),
- array('/controller.html?bar=bar&path=foo%3Dfoo', new ControllerReference('controller', array('foo' => 'foo'), array('bar' => 'bar'))),
- array('/controller.html?foo=foo', new ControllerReference('controller', array(), array('foo' => 'foo'))),
- );
- }
-
- public function testGenerateProxyUriWithARequest()
- {
- $request = Request::create('/');
- $request->attributes->set('_format', 'json');
- $controller = new ControllerReference('controller', array(), array());
-
- $this->assertEquals('/controller.json', $this->getStrategy()->doGenerateProxyUri($controller, $request));
- }
-
- private function getStrategy()
- {
- $strategy = new Strategy();
- $strategy->setUrlGenerator($this->getUrlGenerator());
-
- return $strategy;
- }
-}
-
-class Strategy extends GeneratorAwareRenderingStrategy
-{
- public function render($uri, Request $request = null, array $options = array()) {}
- public function getName() {}
-
- public function doGenerateProxyUri(ControllerReference $reference, Request $request = null)
- {
- return parent::generateProxyUri($reference, $request);
- }
-}
View
22 src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/HIncludeRenderingStrategyTest.php
@@ -16,17 +16,13 @@
use Symfony\Component\HttpKernel\UriSigner;
use Symfony\Component\HttpFoundation\Request;
-class HIncludeRenderingStrategyTest extends AbstractRenderingStrategyTest
+class HIncludeRenderingStrategyTest extends \PHPUnit_Framework_TestCase
{
protected function setUp()
{
if (!class_exists('Symfony\Component\HttpFoundation\Request')) {
$this->markTestSkipped('The "HttpFoundation" component is not available');
}
-
- if (!interface_exists('Symfony\Component\Routing\Generator\UrlGeneratorInterface')) {
- $this->markTestSkipped('The "Routing" component is not available');
- }
}
/**
@@ -35,37 +31,37 @@ protected function setUp()
public function testRenderExceptionWhenControllerAndNoSigner()
{
$strategy = new HIncludeRenderingStrategy();
- $strategy->render(new ControllerReference('main_controller', array(), array()));
+ $strategy->render(new ControllerReference('main_controller', array(), array()), Request::create('/'));
}
public function testRenderWithControllerAndSigner()
{
$strategy = new HIncludeRenderingStrategy(null, new UriSigner('foo'));
- $strategy->setUrlGenerator($this->getUrlGenerator());
- $this->assertEquals('<hx:include src="/main_controller.html?_hash=6MuxpWUHcqIddMMmoN36uPsEjws%3D"></hx:include>', $strategy->render(new ControllerReference('main_controller', array(), array()))->getContent());
+
+ $this->assertEquals('<hx:include src="http://localhost/_proxy?_path=_format%3Dhtml%26_controller%3Dmain_controller&_hash=ctQ5X4vzZnFmmPiqIqnBkVr%2B%2B10%3D"></hx:include>', $strategy->render(new ControllerReference('main_controller', array(), array()), Request::create('/'))->getContent());
}
public function testRenderWithUri()
{
$strategy = new HIncludeRenderingStrategy();
- $this->assertEquals('<hx:include src="/foo"></hx:include>', $strategy->render('/foo')->getContent());
+ $this->assertEquals('<hx:include src="/foo"></hx:include>', $strategy->render('/foo', Request::create('/'))->getContent());
$strategy = new HIncludeRenderingStrategy(null, new UriSigner('foo'));
- $this->assertEquals('<hx:include src="/foo"></hx:include>', $strategy->render('/foo')->getContent());
+ $this->assertEquals('<hx:include src="/foo"></hx:include>', $strategy->render('/foo', Request::create('/'))->getContent());
}
public function testRenderWhithDefault()
{
// only default
$strategy = new HIncludeRenderingStrategy();
- $this->assertEquals('<hx:include src="/foo">default</hx:include>', $strategy->render('/foo', null, array('default' => 'default'))->getContent());
+ $this->assertEquals('<hx:include src="/foo">default</hx:include>', $strategy->render('/foo', Request::create('/'), array('default' => 'default'))->getContent());
// only global default
$strategy = new HIncludeRenderingStrategy(null, null, 'global_default');
- $this->assertEquals('<hx:include src="/foo">global_default</hx:include>', $strategy->render('/foo', null, array())->getContent());
+ $this->assertEquals('<hx:include src="/foo">global_default</hx:include>', $strategy->render('/foo', Request::create('/'), array())->getContent());
// global default and default
$strategy = new HIncludeRenderingStrategy(null, null, 'global_default');
- $this->assertEquals('<hx:include src="/foo">default</hx:include>', $strategy->render('/foo', null, array('default' => 'default'))->getContent());
+ $this->assertEquals('<hx:include src="/foo">default</hx:include>', $strategy->render('/foo', Request::create('/'), array('default' => 'default'))->getContent());
}
}
View
63 src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/ProxyAwareRenderingStrategyTest.php
@@ -0,0 +1,63 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel\Tests\RenderingStrategy;
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\Controller\ControllerReference;
+use Symfony\Component\HttpKernel\RenderingStrategy\ProxyAwareRenderingStrategy;
+
+class ProxyAwareRenderingStrategyTest extends \PHPUnit_Framework_TestCase
+{
+ /**
+ * @dataProvider getGenerateProxyUriData
+ */
+ public function testGenerateProxyUri($uri, $controller)
+ {
+ $this->assertEquals($uri, $this->getStrategy()->doGenerateProxyUri($controller, Request::create('/')));
+ }
+
+ public function getGenerateProxyUriData()
+ {
+ return array(
+ array('http://localhost/_proxy?_path=_format%3Dhtml%26_controller%3Dcontroller', new ControllerReference('controller', array(), array())),
+ array('http://localhost/_proxy?_path=_format%3Dxml%26_controller%3Dcontroller', new ControllerReference('controller', array('_format' => 'xml'), array())),
+ array('http://localhost/_proxy?_path=foo%3Dfoo%26_format%3Djson%26_controller%3Dcontroller', new ControllerReference('controller', array('foo' => 'foo', '_format' => 'json'), array())),
+ array('http://localhost/_proxy?bar=bar&_path=foo%3Dfoo%26_format%3Dhtml%26_controller%3Dcontroller', new ControllerReference('controller', array('foo' => 'foo'), array('bar' => 'bar'))),
+ array('http://localhost/_proxy?foo=foo&_path=_format%3Dhtml%26_controller%3Dcontroller', new ControllerReference('controller', array(), array('foo' => 'foo'))),
+ );
+ }
+
+ public function testGenerateProxyUriWithARequest()
+ {
+ $request = Request::create('/');
+ $request->attributes->set('_format', 'json');
+ $controller = new ControllerReference('controller', array(), array());
+
+ $this->assertEquals('http://localhost/_proxy?_path=_format%3Djson%26_controller%3Dcontroller', $this->getStrategy()->doGenerateProxyUri($controller, $request));
+ }
+
+ private function getStrategy()
+ {
+ return new Strategy();
+ }
+}
+
+class Strategy extends ProxyAwareRenderingStrategy
+{
+ public function render($uri, Request $request, array $options = array()) {}
+ public function getName() {}
+
+ public function doGenerateProxyUri(ControllerReference $reference, Request $request)
+ {
+ return parent::generateProxyUri($reference, $request);
+ }
+}
Something went wrong with that request. Please try again.