[WIP] Kernel refactor #6459

fabpot · Dec 21, 2012

Currently, the handling of sub-requests (including ESI and hinclude) is mostly done in FrameworkBundle. It makes these important features harder to implement for people using only HttpKernel (like Drupal and Silex for instance).

This PR moves the code to HttpKernel instead. The code has also been refactored to allow easier integration of other rendering strategies (refs #6108).

The internal route has been re-introduced but it can only be used for trusted IPs (so for the internal rendering which is managed by Symfony itself, or by a trusted reverse proxy like Varnish for ESI handling). For the hinclude strategy, when using a controller, the URL is automatically signed (see #6463).

The usage of a listener instead of a controller to handle internal sub-requests speeds up things quite a lot as it saves one sub-request handling. In Symfony 2.0 and 2.1, the handling of a sub-request actually creates two sub-requests.

Rendering a sub-request from a controller can be done with the following code:

{# default strategy #}
{{ render(path("partial")) }}
{{ render(controller("SomeBundle:Controller:partial")) }}

{# ESI strategy #}
{{ render(path("partial"), { strategy: 'esi' }) }}
{{ render(controller("SomeBundle:Controller:partial"), { strategy: 'esi' }) }}

{# hinclude strategy #}
{{ render(path("default1"), { strategy: 'hinclude' }) }}

The second commit allows to simplify the calls a little bit thanks to some nice syntactic sugar:

{# default strategy #}
{{ render(path("partial")) }}
{{ render(controller("SomeBundle:Controller:partial")) }}

{# ESI strategy #}
{{ render_esi(path("partial")) }}
{{ render_esi(controller("SomeBundle:Controller:partial")) }}

{# hinclude strategy #}
{{ render_hinclude(path("default1")) }}

Tobion · Dec 21, 2012

src/Symfony/Component/HttpKernel/HttpContentRenderer.php

+     * Renders a Controller and returns the Response content.
+     *
+     *  * ignore_errors: true to return an empty string in case of an error
+     *  * standalone: whether to generate an esi:include tag or not when ESI is supported


"whether" suggests that a boolean is passed as option value. but it's not. should instead list the valid values.

Crell · Dec 27, 2012

src/Symfony/Component/HttpKernel/HttpContentRenderer.php

+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class HttpContentRenderer implements EventSubscriberInterface


I don't know if this is common in Symfony, but having a class that does something useful be its own event subscriber feels weird to me. We've been largely avoiding that in Drupal so far.

We do this in other places as well (the firewall for instance), and using a separate listener would complicate things here

fabpot · Jan 3, 2013

I've just pushed a new version of the code that actually works in my browser (but I've not yet written any unit tests). I've updated the PR description accordingly.

All comments welcome!

Koc · Jan 3, 2013

what about render(controller="SomeBundle:Controller:partial", strategy="esi")?

Tobion · Jan 3, 2013

src/Symfony/Bundle/FrameworkBundle/Resources/config/routing/proxy.xml

+    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="/{_controller}.{_format}" />


Shouldn't this have some unique static prefix like /_proxy? Otherwise it will probably conflict with custom route patterns and one or the other route won't match as expected.

No need for a prefix as you need to import it in your routing file and use a prefix (like it is done today with the internal routes).

IMO it's better to set the prefix explicitly in the pattern instead of relying on the user to set a prefix on import. Because if the users forgets the prefix the route will most likely collide with custom ones as the pattern is too general. So the user does not need to care about the prefix, and if he still sets a custom prefix, it will work anyway.

fair enough, done

stof · Jan 4, 2013

shouldn't we have interfaces for the UriSigner and the HttpContentRenderer ?

jalliot · Jan 4, 2013

src/Symfony/Bundle/FrameworkBundle/HttpKernel.php

-    public function hasEsiSupport()
-    {
-        return $this->esiSupport;
+        $this->container->get('content_renderer')->render($uri, $options);


Shoud be http_content_renderer.

stof · Jan 6, 2013

src/Symfony/Bundle/FrameworkBundle/HttpKernel.php


 /**
 * This HttpKernel is used to manage scope changes of the DI container.
 *
 * @author Fabien Potencier <fabien@symfony.com>
 * @author Johannes M. Schmitt <schmittjoh@gmail.com>
+ *
+ * @deprecated This class is deprecated in 2.2 and will be removed in 2.3


you should add a trigger_error call in the constructor

No, as this class is still used in the DIC, which means that everyone will have the deprecated message without being able to fix it. And we cannot switch to the new class as it does not have the forward and render methods. Anyway, I don't think that too many people need to care about this.

sdboyer · Jan 7, 2013

@Crell asked me to weigh in, since i'm one of the Drupal folks who's likely to work most with this.

i think i've grokked about 60% of the big picture here, and i'm generally happy with what i see. the assumption that the HInclude strategy makes about working with templates probably isn't one that we'll be able to use (and so, would need to write our own), but that's not a big deal since the whole goal here is to make strategies pluggable.

so, yeah. +1.

winzou · Jan 9, 2013

Just for my information: will this PR be merged for 2.2 version? Thanks.

stof · Jan 9, 2013

@winzou according to the blog post announcing the beta 1 release, yes. It is explicitly listed as being one of the reason to make it a beta instead of the first RC.

winzou · Jan 9, 2013

OK thanks, I've totally skipped this blog post.

…component

…ing strategies Here is the code you need to write when using the regular render function for an ESI strategy: {{ render(path('path'), { strategy: 'esi' }) }} And the same with the new render_* function: {{ render_esi(path('path')) }}

…erer::render()

…nce now and not part of any interface anyway)

…emplating engine is available (the latter being mostly useful when testing)

… class

fabpot · Jan 10, 2013

I've just added a bunch of unit tests and fix some bugs I found while writing the tests.

This PR was merged into the master branch. Commits ------- 76fefe3 updated CHANGELOG and UPGRADE files f7da1f0 added some unit tests (and fixed some bugs) f17f586 moved the container aware HTTP kernel to the HttpKernel component 2eea768 moved the deprecation logic calls outside the new HttpContentRenderer class bd102c5 made the content renderer work even when ESI is disabled or when no templating engine is available (the latter being mostly useful when testing) a8ea4e4 [FrameworkBundle] deprecated HttpKernel::forward() (it is only used once now and not part of any interface anyway) 1240690 [HttpKernel] made the strategy a regular parameter in HttpContentRenderer::render() adc067e [FrameworkBundle] made some services private 1f1392d [HttpKernel] simplified and enhanced code managing the hinclude strategy 403bb06 [HttpKernel] added missing phpdoc and tweaked existing ones 892f00f [HttpKernel] added a URL signer mechanism for hincludes a0c49c3 [TwigBridge] added a render_* function to ease usage of custom rendering strategies 9aaceb1 moved the logic from HttpKernel in FrameworkBundle to the HttpKernel component Discussion ---------- [WIP] Kernel refactor Currently, the handling of sub-requests (including ESI and hinclude) is mostly done in FrameworkBundle. It makes these important features harder to implement for people using only HttpKernel (like Drupal and Silex for instance). This PR moves the code to HttpKernel instead. The code has also been refactored to allow easier integration of other rendering strategies (refs #6108). The internal route has been re-introduced but it can only be used for trusted IPs (so for the internal rendering which is managed by Symfony itself, or by a trusted reverse proxy like Varnish for ESI handling). For the hinclude strategy, when using a controller, the URL is automatically signed (see #6463). The usage of a listener instead of a controller to handle internal sub-requests speeds up things quite a lot as it saves one sub-request handling. In Symfony 2.0 and 2.1, the handling of a sub-request actually creates two sub-requests. Rendering a sub-request from a controller can be done with the following code: ```jinja {# default strategy #} {{ render(path("partial")) }} {{ render(controller("SomeBundle:Controller:partial")) }} {# ESI strategy #} {{ render(path("partial"), { strategy: 'esi' }) }} {{ render(controller("SomeBundle:Controller:partial"), { strategy: 'esi' }) }} {# hinclude strategy #} {{ render(path("default1"), { strategy: 'hinclude' }) }} ``` The second commit allows to simplify the calls a little bit thanks to some nice syntactic sugar: ```jinja {# default strategy #} {{ render(path("partial")) }} {{ render(controller("SomeBundle:Controller:partial")) }} {# ESI strategy #} {{ render_esi(path("partial")) }} {{ render_esi(controller("SomeBundle:Controller:partial")) }} {# hinclude strategy #} {{ render_hinclude(path("default1")) }} ``` --------------------------------------------------------------------------- by fabpot at 2013-01-03T17:58:49Z I've just pushed a new version of the code that actually works in my browser (but I've not yet written any unit tests). I've updated the PR description accordingly. All comments welcome! --------------------------------------------------------------------------- by Koc at 2013-01-03T20:11:43Z what about `render(controller="SomeBundle:Controller:partial", strategy="esi")`? --------------------------------------------------------------------------- by stof at 2013-01-04T09:01:01Z shouldn't we have interfaces for the UriSigner and the HttpContentRenderer ? --------------------------------------------------------------------------- by lsmith77 at 2013-01-04T19:28:09Z btw .. as mentioned in #6213 i think it would make sense to refactor the HttpCache to use a cache layer to allow more flexibility in where to cache the data (including clustering) and better invalidation. as such if you are refactoring HttpKernel .. it might also make sense to explore splitting off HttpCache. --------------------------------------------------------------------------- by fabpot at 2013-01-04T19:30:07Z @lsmith77 This is a totally different topic. This PR is just about moving things from FrameworkBundle to HttpKernel to make them more reusable outside of the full-stack framework. --------------------------------------------------------------------------- by fabpot at 2013-01-05T09:39:52Z I think this PR is almost ready now. I still need to update the docs and add some unit tests. Any other comments on the whole approach? The class names? The `controller` function thingy? The URI signer mechanism? The proxy protection for the internal controller? The proxy to handle internal routes? --------------------------------------------------------------------------- by sstok at 2013-01-05T10:08:25Z Looks good to me :+1: --------------------------------------------------------------------------- by sdboyer at 2013-01-07T18:17:08Z @Crell asked me to weigh in, since i'm one of the Drupal folks who's likely to work most with this. i think i've grokked about 60% of the big picture here, and i'm generally happy with what i see. the assumption that the HInclude strategy makes about working with templates probably isn't one that we'll be able to use (and so, would need to write our own), but that's not a big deal since the whole goal here is to make strategies pluggable. so, yeah. +1. --------------------------------------------------------------------------- by winzou at 2013-01-09T20:21:44Z Just for my information: will this PR be merged for 2.2 version? Thanks. --------------------------------------------------------------------------- by stof at 2013-01-09T20:41:04Z @winzou according to the blog post announcing the beta 1 release, yes. It is explicitly listed as being one of the reason to make it a beta instead of the first RC. --------------------------------------------------------------------------- by winzou at 2013-01-09T20:49:36Z OK thanks, I've totally skipped this blog post. --------------------------------------------------------------------------- by fabpot at 2013-01-10T15:26:15Z I've just added a bunch of unit tests and fix some bugs I found while writing the tests.

ghost · Jan 11, 2013

@fabpot - is there anything that needs to be documented now in the HttpKernel Component docs?

fabpot · Jan 11, 2013

@Drak: I wanted to update the docs but that does not work. I need to make big changes, so that will come later on... probably with the help of @weaverryan

Tobion reviewed Dec 21, 2012
View reviewed changes

asm89 mentioned this pull request Dec 22, 2012

[WIP][RFC] Signed internal uris #6463

Closed

Crell reviewed Dec 27, 2012
View reviewed changes

Tobion reviewed Jan 3, 2013
View reviewed changes

jalliot reviewed Jan 4, 2013
View reviewed changes

stof reviewed Jan 6, 2013
View reviewed changes

fabpot added 11 commits January 10, 2013 09:21

moved the logic from HttpKernel in FrameworkBundle to the HttpKernel …

9aaceb1

…component

[HttpKernel] added a URL signer mechanism for hincludes

892f00f

[HttpKernel] added missing phpdoc and tweaked existing ones

403bb06

[HttpKernel] simplified and enhanced code managing the hinclude strategy

1f1392d

[FrameworkBundle] made some services private

adc067e

[HttpKernel] made the strategy a regular parameter in HttpContentRend…

1240690

…erer::render()

[FrameworkBundle] deprecated HttpKernel::forward() (it is only used o…

a8ea4e4

…nce now and not part of any interface anyway)

made the content renderer work even when ESI is disabled or when no t…

bd102c5

…emplating engine is available (the latter being mostly useful when testing)

moved the deprecation logic calls outside the new HttpContentRenderer…

2eea768

… class

moved the container aware HTTP kernel to the HttpKernel component

f17f586

fabpot added 2 commits January 10, 2013 16:26

added some unit tests (and fixed some bugs)

f7da1f0

updated CHANGELOG and UPGRADE files

76fefe3

fabpot mentioned this pull request Jan 10, 2013

UPGRADE FROM 2.1 to 2.2 TwigBridge #6692

Closed

fabpot merged commit 76fefe3 into symfony:master Jan 11, 2013

fabpot mentioned this pull request Jan 12, 2013

Updated the docs for the new way to render partials/ESI/HInclude symfony/symfony-docs#2121

Merged

sstok mentioned this pull request Jan 14, 2013

[HttpKernel] Circular reference detected for service "templating" #6730

Closed

andrerom mentioned this pull request Apr 29, 2018

[HttpKernel] Set first trusted proxy as REMOTE_ADDR in InlineFragmentRenderer. #26973

Merged

alexander-schranz mentioned this pull request May 22, 2023

[HttpKernel] Move UriSigner to Http Foundation #50384

Closed

Search code, repositories, users, issues, pull requests...

Uh oh!

[WIP] Kernel refactor #6459

[WIP] Kernel refactor #6459

Uh oh!

Conversation

fabpot commented Dec 21, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jan 3, 2013

Uh oh!

Koc commented Jan 3, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Jan 4, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdboyer commented Jan 7, 2013

Uh oh!

winzou commented Jan 9, 2013

Uh oh!

stof commented Jan 9, 2013

Uh oh!

winzou commented Jan 9, 2013

Uh oh!

fabpot commented Jan 10, 2013

Uh oh!

ghost commented Jan 11, 2013

Uh oh!

fabpot commented Jan 11, 2013

Uh oh!

Uh oh!