Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[WIP] Kernel refactor #6459

Merged
merged 13 commits into from
@fabpot
Owner

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")) }}
src/Symfony/Component/HttpKernel/HttpContentRenderer.php
((28 lines not shown))
+ public function __construct(array $strategies = array(), $debug = false)
+ {
+ $this->strategies = $strategies;
+ $this->debug = $debug;
+ }
+
+ public function onKernelRequest(GetResponseEvent $event)
+ {
+ $this->request = $event->getRequest();
+ }
+
+ /**
+ * 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
@Tobion Collaborator
Tobion added a note

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/HttpKernel/HttpContentRenderer.php
((21 lines not shown))
+ */
+class HttpContentRenderer implements EventSubscriberInterface
+{
+ private $debug;
+ private $strategies;
+ private $request;
+
+ public function __construct(array $strategies = array(), $debug = false)
+ {
+ $this->strategies = $strategies;
+ $this->debug = $debug;
+ }
+
+ public function onKernelRequest(GetResponseEvent $event)
+ {
+ $this->request = $event->getRequest();
@stof Collaborator
stof added a note

This will not work correctly in some cases:

master request -> masterRequest
1st sub request _> set to subRequest1
continue the master request -> still using subRequest1

@fabpot Owner
fabpot added a note

I've made a change but it is still not perfect when the default strategy is involved and an exception is thrown. But I don't know how to fix this elegantly (and the way it's done in HttpContentRenderer is far from perfect too).

The only idea I have is to create a RequestAwareInterface and when a service implements that, we automatically call the setRequest when we enter the request scope and remove it/reset the previous one when leave the request scope. I've not had a look at how we can implement that, but does not look easy to do that without performance impact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Kernel/RenderingStrategy/DefaultRenderingStrategy.php
((26 lines not shown))
+ {
+ $this->kernel = $kernel;
+ }
+
+ public function render($uri, Request $request = null, array $options)
+ {
+ $subRequest = $this->createSubRequest($uri, $request);
+ $level = ob_get_level();
+ try {
+ return $this->kernel->handle($subRequest);
+ } catch (\Exception $e) {
+ if (isset($options['alt'])) {
+ $alt = $options['alt'];
+ unset($options['alt']);
+
+ return $this->kernel->render($alt, $request, $options);
@stof Collaborator
stof added a note

There is no render method on the kernel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Kernel/RenderingStrategy/DefaultRenderingStrategy.php
((20 lines not shown))
+ */
+class KernelRenderingStrategy implements RenderingStrategyInterface
+{
+ private $kernel;
+
+ public function __construct(HttpKernelInterface $kernel)
+ {
+ $this->kernel = $kernel;
+ }
+
+ public function render($uri, Request $request = null, array $options)
+ {
+ $subRequest = $this->createSubRequest($uri, $request);
+ $level = ob_get_level();
+ try {
+ return $this->kernel->handle($subRequest);
@stof Collaborator
stof added a note

shouldn't you use $this->handle() here (which is unused currently) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...HttpKernel/RenderingStrategy/EsiRenderingStrategy.php
((10 lines not shown))
+ */
+
+namespace Symfony\Component\HttpKernel\RenderingStrategy;
+
+use Symfony\Component\HttpFoundation\Request;
+
+/**
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class EsiRenderingStrategy implements RenderingStrategyInterface
+{
+ private $esi;
+ private $internal;
+
+ public function __construct(Esi $esi, InternalRenderingStrategy $internal)
@stof Collaborator
stof added a note

shouldn't you typehint the interface here ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ernel/RenderingStrategy/HIncludeRenderingStrategy.php
((10 lines not shown))
+ */
+
+namespace Symfony\Component\HttpKernel\RenderingStrategy;
+
+use Symfony\Component\HttpFoundation\Request;
+
+/**
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class HIncludeRenderingStrategy implements RenderingStrategyInterface
+{
+ private $templating;
+ private $globalDefaultTemplate;
+
+ public function __construct($templating, $globalDefaultTemplate = null)
@stof Collaborator
stof added a note

missing typehint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/HttpKernel/HttpContentRenderer.php
((44 lines not shown))
+ *
+ * @param string $uri A URI
+ * @param array $options An array of options
+ *
+ * @return string The Response content
+ */
+ public function render($uri, array $options = array())
+ {
+ if (!isset($options['ignore_errors'])) {
+ $options['ignore_errors'] = !$this->debug;
+ }
+
+ if (!isset($options['standalone'])) {
+ $options['standalone'] = 'default';
+ } elseif (true === $options['standalone']) {
+ // support for the true value is @deprecated in 2.2, will be removed in 2.3
@stof Collaborator
stof added a note

you should use trigger_error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/HttpKernel/HttpContentRenderer.php
((48 lines not shown))
+ * @return string The Response content
+ */
+ public function render($uri, array $options = array())
+ {
+ if (!isset($options['ignore_errors'])) {
+ $options['ignore_errors'] = !$this->debug;
+ }
+
+ if (!isset($options['standalone'])) {
+ $options['standalone'] = 'default';
+ } elseif (true === $options['standalone']) {
+ // support for the true value is @deprecated in 2.2, will be removed in 2.3
+ $options['standalone'] = 'esi';
+ }
+
+ return $this->strategies[$options['standalone']]->render($uri, $this->request, $options);
@stof Collaborator
stof added a note

you should check for invalid strategies here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Bundle/FrameworkBundle/HttpKernel.php
((11 lines not shown))
- * Available options:
- *
- * * ignore_errors: true to return an empty string in case of an error
- * * alt: an alternative URI to execute in case of an error
- * * standalone: whether to generate an esi:include tag or not when ESI is supported
- * * comment: a comment to add when returning an esi:include tag
- *
- * @param string $uri A URI
- * @param array $options An array of options
- *
- * @return string The Response content
- *
- * @throws \RuntimeException
- * @throws \Exception
- */
- public function render($uri, array $options = array())
@stof Collaborator
stof added a note

shouldn't you keep it as deprecated for 2.2 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Kernel/RenderingStrategy/DefaultRenderingStrategy.php
((6 lines not shown))
+ * (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\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\HttpKernelInterface;
+
+/**
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class KernelRenderingStrategy implements RenderingStrategyInterface
@sstok
sstok added a note

Class is not the same as filename, this will give problems with the autoloader.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/HttpKernel/HttpContentRenderer.php
((8 lines not shown))
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel;
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\Event\GetResponseEvent;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
+
+/**
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class HttpContentRenderer implements EventSubscriberInterface
@Crell
Crell added a note

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.

@stof Collaborator
stof added a note

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Kernel/RenderingStrategy/DefaultRenderingStrategy.php
((6 lines not shown))
+ * (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\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\HttpKernelInterface;
+
+/**
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class DefaultRenderingStrategy implements RenderingStrategyInterface
@Crell
Crell added a note

Perhaps instead of DefaultRenderingStrategy this should be called SubrequestRenderingStrategy? Since on many sites the "default" may actually be ESI/hInclude/nginx/whatever.

@sstok
sstok added a note

:+1:

@fabpot Owner
fabpot added a note

I had several possible names like: SubRequestRenderingStrategy, HttpKernelRenderingStrategy, FallbackRenderingStrategy, and DefaultRenderingStrategy. In the end, I chose DefaultRenderingStrategy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ernel/RenderingStrategy/HIncludeRenderingStrategy.php
((14 lines not shown))
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\Templating\EngineInterface;
+
+/**
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class HIncludeRenderingStrategy implements RenderingStrategyInterface
+{
+ private $templating;
+ private $globalDefaultTemplate;
+
+ public function __construct($templating, $globalDefaultTemplate = null)
+ {
+ if (!$templating instanceof EngineInterface && !$templating instanceof \Twig_Environment) {
+ throw new \InvalidArgumentException('$templating must be an instance of \Twig_Environment');
@stof Collaborator
stof added a note

wrong message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ernel/RenderingStrategy/HIncludeRenderingStrategy.php
((13 lines not shown))
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\Templating\EngineInterface;
+
+/**
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class HIncludeRenderingStrategy implements RenderingStrategyInterface
+{
+ private $templating;
+ private $globalDefaultTemplate;
+
+ public function __construct($templating, $globalDefaultTemplate = null)
+ {
+ if (!$templating instanceof EngineInterface && !$templating instanceof \Twig_Environment) {
@Crell
Crell added a note

What about hInclude implies Twig templating? That shouldn't be an issue for Drupal (I hope), but it seems odd to tightly couple hInclude to a particular rendering engine.

@stof Collaborator
stof added a note

It does not. It is either an EngineInterface or Twig directly (to allow using it with Twig without the Templating component and the bridge)

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

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
Koc commented

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

...le/FrameworkBundle/Resources/config/routing/proxy.xml
@@ -0,0 +1,8 @@
+<?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="/{_controller}.{_format}" />
@Tobion Collaborator
Tobion added a note

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.

@fabpot Owner
fabpot added a note

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

@Tobion Collaborator
Tobion added a note

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.

@fabpot Owner
fabpot added a note

fair enough, done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/HttpKernel/UrlSigner.php
((4 lines not shown))
+ * 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;
+
+/**
+ * UrlSigner.
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class UrlSigner
@Tobion Collaborator
Tobion added a note

should maybe be UriSigner. Uri is more correct and the HttpFoundation also uses Uri, e.g. $request->getUri().

@fabpot Owner
fabpot added a note

makes sense, done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/HttpKernel/UrlSigner.php
((24 lines not shown))
+ {
+ $this->secret = $secret;
+ }
+
+ public function sign($uri)
+ {
+ return $uri.(false === (strpos($uri, '?')) ? '?' : '&').'_key='.$this->computeKey($uri);
+ }
+
+ public function check($uri)
+ {
+ if (!preg_match('/(\?|&)_key=(.+?)(&|$)/', $uri, $matches, PREG_OFFSET_CAPTURE)) {
+ return false;
+ }
+
+ // the naked URI is the URI without the _key parameter (we need to keep the ? if there is some other parameters after)
@Tobion Collaborator
Tobion added a note

I would rather call it _hash instead of _key.

@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
@Tobion Tobion commented on the diff
...ernel/RenderingStrategy/HIncludeRenderingStrategy.php
((21 lines not shown))
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class HIncludeRenderingStrategy extends GeneratorAwareRenderingStrategy
+{
+ private $templating;
+ private $globalDefaultTemplate;
+
+ public function __construct($templating, UrlSigner $signer = null, $globalDefaultTemplate = null)
+ {
+ if (!$templating instanceof EngineInterface && !$templating instanceof \Twig_Environment) {
+ throw new \InvalidArgumentException('The hinclude rendering strategy needs an instance of \Twig_Environment or Symfony\Component\Templating\EngineInterface');
+ }
+
+ $this->templating = $templating;
+ $this->globalDefaultTemplate = $globalDefaultTemplate;
+ $this->signer = $signer;
@Tobion Collaborator
Tobion added a note

Missing property declaration.

@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
...rameworkBundle/Resources/config/content_generator.xml
((21 lines not shown))
+ </service>
+
+ <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>
+ </service>
+
+ <service id="http_content_renderer.strategy.esi" class="%http_content_renderer.strategy.esi.class%">
+ <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>
+ </service>
+
+ <service id="http_content_renderer.strategy.hinclude" class="%http_content_renderer.strategy.hinclude.class%">
@stof Collaborator
stof added a note

strategies could be marked as private, allowing to inline them in the container

@fabpot Owner
fabpot added a note

I thought about that, but because of a bug in the PHPDumper, that's not possible for hinclude. I'm working on fixing the bug so that we can use private services later on.

@fabpot Owner
fabpot added a note

see #6565 for the bug fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff
...Kernel/RenderingStrategy/DefaultRenderingStrategy.php
((32 lines not shown))
+ {
+ if ($uri instanceof ControllerReference) {
+ $uri = $this->generateProxyUri($uri, $request);
+ }
+
+ $subRequest = $this->createSubRequest($uri, $request);
+
+ $level = ob_get_level();
+ try {
+ return $this->handle($subRequest);
+ } catch (\Exception $e) {
+ if (isset($options['alt'])) {
+ $alt = $options['alt'];
+ unset($options['alt']);
+
+ return $this->render($alt, $request, $options);
@stof Collaborator
stof added a note

shouldn't we clean the output buffers in this case as well ?

@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
@stof stof commented on the diff
...Kernel/RenderingStrategy/DefaultRenderingStrategy.php
((58 lines not shown))
+ }
+ }
+
+ protected function handle(Request $request)
+ {
+ $response = $this->kernel->handle($request, HttpKernelInterface::SUB_REQUEST, false);
+
+ if (!$response->isSuccessful()) {
+ throw new \RuntimeException(sprintf('Error when rendering "%s" (Status code is %s).', $request->getUri(), $response->getStatusCode()));
+ }
+
+ if (!$response instanceof StreamedResponse) {
+ return $response->getContent();
+ }
+
+ $response->sendContent();
@stof Collaborator
stof added a note

This method is always returning null for StreamedResponse, which would mean that render also returns null

@fabpot Owner
fabpot added a note

I don't understand. sendContent does call the callback which does output the content.

@stof Collaborator
stof added a note

yeah, but it outputs it whereas the Strategy is supposed to return it

@fabpot Owner
fabpot added a note

which makes sense as when using streaming, you don't return anything, you output the content as it comes.

@stof Collaborator
stof added a note

but it also means that this case does not respec the interface

@fabpot Owner
fabpot added a note

I've fixed the phpdoc to take this case into account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ernel/RenderingStrategy/HIncludeRenderingStrategy.php
((45 lines not shown))
+ }
+
+ $uri = $this->signer->sign($this->generateProxyUri($uri, $request));
+ }
+
+ $defaultTemplate = isset($options['default']) ? $options['default'] : null;
+ $defaultContent = null;
+
+ if (null !== $defaultTemplate) {
+ if ($this->templateExists($defaultTemplate)) {
+ $defaultContent = $this->templating->render($defaultContent);
+ } else {
+ $defaultContent = $defaultTemplate;
+ }
+ } elseif ($this->globalDefaultTemplate) {
+ $defaultContent = $this->templating->render($this->globalDefaultTemplate);
@stof Collaborator
stof added a note

why not handling the global template the same way than the local one, allowing to pass the content directly ?

@fabpot Owner
fabpot added a note

I've just copied the existing code. I've refactored the code now. So, it should be fixed.

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

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

src/Symfony/Bundle/FrameworkBundle/HttpKernel.php
((101 lines not shown))
- * Renders an HInclude tag.
- *
- * @param string $uri A URI
- * @param string $defaultContent Default content
- *
- * @return string
- */
- public function renderHIncludeTag($uri, $defaultContent = null)
- {
- return sprintf('<hx:include src="%s">%s</hx:include>', $uri, $defaultContent);
- }
-
- public function hasEsiSupport()
- {
- return $this->esiSupport;
+ $this->container->get('content_renderer')->render($uri, $options);
@jalliot
jalliot added a note

Shoud be http_content_renderer.

@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
...RenderingStrategy/GeneratorAwareRenderingStrategy.php
((36 lines not shown))
+ * 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
+ */
+ 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.');
+ }
+
+ $arguments = array('_controller' => $reference->controller);
+ if (null !== $request && !isset($arguments['_format'])) {
@jalliot
jalliot added a note

The isset call will always be false here.

@fabpot Owner
fabpot added a note

No:

{{ render(controller('B:C:A', {_format: 'json'})) }}
@jalliot
jalliot added a note

@fabpot $arguments does not contain anything except _controller here.

@stloyd
stloyd added a note

@fabpot For me it looks like @jalliot is right... It looks like you should use:

if (null !== $request && !isset($reference->arguments['_format'])) {

Because line before you have overwritten $arguments variable to only contain:

array('_controller' => $reference->controller);
@fabpot Owner
fabpot added a note

fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ernel/RenderingStrategy/HIncludeRenderingStrategy.php
((40 lines not shown))
+ public function render($uri, Request $request = null, array $options)
+ {
+ if ($uri instanceof ControllerReference) {
+ if (null === $this->signer) {
+ throw new \LogicException('You must use a proper URI when using the Hinclude rendering strategy or set a URL signer.');
+ }
+
+ $uri = $this->signer->sign($this->generateProxyUri($uri, $request));
+ }
+
+ $defaultTemplate = isset($options['default']) ? $options['default'] : null;
+ $defaultContent = null;
+
+ if (null !== $defaultTemplate) {
+ if ($this->templateExists($defaultTemplate)) {
+ $defaultContent = $this->templating->render($defaultContent);
@jalliot
jalliot added a note

The second $defaultContent shoud read $defaultTemplate instead.

@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
...rnel/RenderingStrategy/RenderingStrategyInterface.php
((14 lines not shown))
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\Controller\ControllerReference;
+
+/**
+ * Interface implemented by all rendering strategies.
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ *
+ * @see Symfony\Component\HttpKernel\HttpContentRenderer
+ */
+interface RenderingStrategyInterface
+{
+ /**
+ * Renders a URI and returns the Response content.
+ *
+ * When the Response is a StreamedResponse, the content is stream immediately
@stof Collaborator
stof added a note

should be streamed

@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
...Symfony/Bridge/Twig/Extension/HttpKernelExtension.php
((88 lines not shown))
{
- $this->request = $event->getRequest();
+ $options['strategy']= $strategy;
@stof Collaborator
stof added a note

missing space before =

@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
@lsmith77
Collaborator

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.

@fabpot
Owner

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

@fabpot
Owner

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?

@sstok

Looks good to me :+1:

...rnel/RenderingStrategy/RenderingStrategyInterface.php
((21 lines not shown))
+ *
+ * @see Symfony\Component\HttpKernel\HttpContentRenderer
+ */
+interface RenderingStrategyInterface
+{
+ /**
+ * Renders a URI and returns the Response content.
+ *
+ * When the Response is a StreamedResponse, the content is streamed immediately
+ * instead of being returned.
+ *
+ * @param string|ControllerReference $uri A URI as a string or a ControllerReference instance
+ * @param Request $request A Request instance
+ * @param array $options An array of options
+ *
+ * @param string|null The Response content or null when the Response is streamed
@Tobion Collaborator
Tobion added a note

typo @return

@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
@Tobion Tobion commented on the diff
...le/FrameworkBundle/Resources/config/routing/proxy.xml
@@ -0,0 +1,8 @@
+<?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}.{_format}" />
@Tobion Collaborator
Tobion added a note

Is the _controller required to be of the form SomeBundle:Controller:partial? If so it should probably be specified as requirement for the placeholder.

@Koc
Koc added a note

NB: controllers could be services

@fabpot Owner
fabpot added a note

The controller name is really freeform. As mentioned by @Koc, it can be a service controller (but that's just an example as Class::method also works).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...nent/HttpKernel/EventListener/RouterProxyListener.php
((53 lines not shown))
+
+ $this->checkRequest($request);
+
+ parse_str($request->query->get('path', ''), $attributes);
+ $request->attributes->add($attributes);
+ $request->attributes->set('_route_params', array_replace($request->attributes->get('_route_params'), $attributes));
+ $request->query->remove('path');
+ }
+
+ protected function checkRequest(Request $request)
+ {
+ if (!$request->isMethod('get')) {
+ throw new AccessDeniedHttpException();
+ }
+
+ // does it come from a trusted IP?

What about a method isProxy or isFromProxy on Request object?

@fabpot Owner
fabpot added a note

I thought about that too but I'm not sure that this is needed often.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...nent/HttpKernel/EventListener/RouterProxyListener.php
((49 lines not shown))
+
+ if ('_proxy' !== $request->attributes->get('_route')) {
+ return;
+ }
+
+ $this->checkRequest($request);
+
+ parse_str($request->query->get('path', ''), $attributes);
+ $request->attributes->add($attributes);
+ $request->attributes->set('_route_params', array_replace($request->attributes->get('_route_params'), $attributes));
+ $request->query->remove('path');
+ }
+
+ protected function checkRequest(Request $request)
+ {
+ if (!$request->isMethod('get')) {

what about calling isMethodSafe to also handle HEAD requests

@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
...nent/HttpKernel/EventListener/RouterProxyListener.php
((54 lines not shown))
+ $this->checkRequest($request);
+
+ parse_str($request->query->get('path', ''), $attributes);
+ $request->attributes->add($attributes);
+ $request->attributes->set('_route_params', array_replace($request->attributes->get('_route_params'), $attributes));
+ $request->query->remove('path');
+ }
+
+ protected function checkRequest(Request $request)
+ {
+ if (!$request->isMethod('get')) {
+ throw new AccessDeniedHttpException();
+ }
+
+ // does it come from a trusted IP?
+ $trustedIps = array_merge(array('127.0.0.1', 'fe80::1', '::1'), $request->getTrustedProxies());

Isn't trusting local machine dangerous on shared-hosting?

@fabpot Owner
fabpot added a note

probably

@Crell
Crell added a note

I would recommend against any hard-coded "safe" IP. If we want a default, put it in the default config, not the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...mponent/HttpKernel/Controller/ControllerReference.php
((4 lines not shown))
+ * 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\Controller;
+
+/**
+ * ControllerReference.
+ *
+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+class ControllerReference
@Crell
Crell added a note

Without any documentation in the class, I haven't the slightest idea what this is for or why I'd want to use it, other than I get the feeling that Drupal will probably need to. This needs better inline documentation.

@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
...nent/HttpKernel/EventListener/RouterProxyListener.php
((10 lines not shown))
+ */
+
+namespace Symfony\Component\HttpKernel\EventListener;
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\IpUtils;
+use Symfony\Component\HttpKernel\Event\GetResponseEvent;
+use Symfony\Component\HttpKernel\KernelEvents;
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
+use Symfony\Component\HttpKernel\UriSigner;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+
+/**
+ * Proxies URIs when the current route name is "_proxy".
+ *
+ * If the request does not come from a trusted, it throws an
@Crell
Crell added a note

Does not come from a trusted what?

@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
...nent/HttpKernel/EventListener/RouterProxyListener.php
((41 lines not shown))
+ *
+ * @param GetResponseEvent $event A GetResponseEvent instance
+ *
+ * @throws AccessDeniedHttpException if the request does not come from a trusted IP.
+ */
+ public function onKernelRequest(GetResponseEvent $event)
+ {
+ $request = $event->getRequest();
+
+ if ('_proxy' !== $request->attributes->get('_route')) {
+ return;
+ }
+
+ $this->checkRequest($request);
+
+ parse_str($request->query->get('path', ''), $attributes);
@Crell
Crell added a note

I haven't tried it, but since the second param to parse_str() is an array by reference shouldn't this result in an E_NOTICE? $attributes should be pre-defined on the previous line.

@stof Collaborator
stof added a note

no it is not (it would be a notice if it were not argument passed by reference).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...nent/HttpKernel/EventListener/RouterProxyListener.php
((47 lines not shown))
+ {
+ $request = $event->getRequest();
+
+ if ('_proxy' !== $request->attributes->get('_route')) {
+ return;
+ }
+
+ $this->checkRequest($request);
+
+ parse_str($request->query->get('path', ''), $attributes);
+ $request->attributes->add($attributes);
+ $request->attributes->set('_route_params', array_replace($request->attributes->get('_route_params'), $attributes));
+ $request->query->remove('path');
+ }
+
+ protected function checkRequest(Request $request)
@Crell
Crell added a note

checkRequest() is not a very descriptive name for this method. Checks for what? I had to read through the code here and over in IpUtils to know what was going on. it's actually doing 2 different forms of validation. I'd suggest calling it validateIpAddress() and including a docblock that explains what it's doing. (There is no such thing as a method that does not need a docblock.)

@fabpot Owner
fabpot added a note

I've renamed the method to validateRequest as it also validates the HTTP method. The inline comments already explain what it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Crell Crell commented on the diff
src/Symfony/Component/HttpKernel/HttpContentRenderer.php
((50 lines not shown))
+ *
+ * @param RenderingStrategyInterface $strategy A RenderingStrategyInterface instance
+ */
+ public function addStrategy(RenderingStrategyInterface $strategy)
+ {
+ $this->strategies[$strategy->getName()] = $strategy;
+ }
+
+ /**
+ * Stores the Request object.
+ *
+ * @param GetResponseEvent $event A GetResponseEvent instance
+ */
+ public function onKernelRequest(GetResponseEvent $event)
+ {
+ array_unshift($this->requests, $event->getRequest());
@Crell
Crell added a note

This is perhaps a micro-optimization, but array_unshift() is a very slow function. array_push() and array_pop() are notably faster than shift/unshift. Or, since it appears that this is being used as a stack, http://us3.php.net/manual/en/class.splstack.php may be even nicer since it pushes more down into the engine where it belongs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Crell Crell commented on the diff
src/Symfony/Component/HttpKernel/HttpContentRenderer.php
((102 lines not shown))
+ if (!isset($this->strategies[$strategy])) {
+ throw new \InvalidArgumentException(sprintf('The "%s" rendering strategy does not exist.', $strategy));
+ }
+
+ return $this->strategies[$strategy]->render($uri, $this->requests ? $this->requests[0] : null, $options);
+ }
+
+ public static function getSubscribedEvents()
+ {
+ return array(
+ KernelEvents::REQUEST => 'onKernelRequest',
+ KernelEvents::RESPONSE => 'onKernelResponse',
+ );
+ }
+
+ // to be removed in 2.3
@Crell
Crell added a note

This comment should be included as part of a proper docblock. (Yes, I'm a pedant. It comes with being a Drupaler. :-) )

@fabpot Owner
fabpot added a note

no, it must not. I'm also a pedant. And this is a comment for the core team only. It must not be part of the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/HttpKernel/HttpContentRenderer.php
((70 lines not shown))
+ *
+ * @param FilterResponseEvent $event A FilterResponseEvent instance
+ */
+ public function onKernelResponse(FilterResponseEvent $event)
+ {
+ array_shift($this->requests);
+ }
+
+ /**
+ * Renders a URI and returns the Response content.
+ *
+ * When the Response is a StreamedResponse, the content is streamed immediately
+ * instead of being returned.
+ *
+ * * ignore_errors: true to return an empty string in case of an error
+ * * strategy: the strategy to use for rendering
@Crell
Crell added a note

I think these two lines belong under the $options @param, to indicate that they're the options for that array. As is, they're just floating here with no indication of what they are or where you'd use them.

@Crell
Crell added a note

Should strategy not be its own parameter? It seems important enough to warrant its own parameter, not being shuffled off to $options.

@fabpot Owner
fabpot added a note

The sub-title was missing. fixed now.

@fabpot Owner
fabpot added a note

The strategy is now its own parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/HttpKernel/HttpContentRenderer.php
((85 lines not shown))
+ * * strategy: the strategy to use for rendering
+ *
+ * @param string|ControllerReference $uri A URI as a string or a ControllerReference instance
+ * @param array $options An array of options
+ *
+ * @param string|null The Response content or null when the Response is streamed
+ */
+ public function render($uri, array $options = array())
+ {
+ if (!isset($options['ignore_errors'])) {
+ $options['ignore_errors'] = !$this->debug;
+ }
+
+ $options = $this->fixOptions($options);
+
+ $strategy = isset($options['strategy']) ? $options['strategy'] : 'default';
@Crell
Crell added a note

This forces all sites to have a default rendering strategy of subrequest, since the "default" renderer is subrequest. That doesn't seem like something that should be hard coded. Why shouldn't I be able to switch my rendering strategy wholesale?

@fabpot Owner
fabpot added a note

I think this is a very bad idea to be able to change the default strategy for many reasons:

  • First and foremost, using render() is a way to render a "partial" that needs some logic associated with it (think include vs render). Then, you might wat to use another strategy like ESI or hinclude.

  • Because we have many bundles in Symfony (like in Drupal with modules) and they should all be able to rely on something stable. I try to limit the number of moving things (less options, less settings, more predictability).

  • Each rendering strategy has its own set of options (like alt for ESI or default for hinclude), so the strategy should be chosen carefully by the developer and configured properly. If you can change the strategy globally, that could break things easily (and no, we won't add yet another setting to be able to set the default strategy global settings).

  • Lastly, I don't think it makes sense to use the ESI strategy for all the render calls (and the same goes for hinclude too). So, I fail to see the real-world use case.

And if you really want to change the default strategy or if you want to implement your own default strategy, just create a strategy with the default name and register it in place of the standard one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Crell Crell commented on the diff
...HttpKernel/RenderingStrategy/EsiRenderingStrategy.php
((22 lines not shown))
+ */
+class EsiRenderingStrategy extends GeneratorAwareRenderingStrategy
+{
+ private $esi;
+ private $defaultStrategy;
+
+ /**
+ * Constructor
+ *
+ * @param Esi $esi An Esi instance
+ * @param RenderingStrategyInterface $defaultStrategy The default strategy to use when ESI is not supported
+ */
+ public function __construct(Esi $esi, RenderingStrategyInterface $defaultStrategy)
+ {
+ $this->esi = $esi;
+ $this->defaultStrategy = $defaultStrategy;
@Crell
Crell added a note

Unless this can only ever be the subrequest strategy, it should probably be called "fallbackStrategy" instead to avoid confusion.

@fabpot Owner
fabpot added a note

The default strategy should be DefaultRenderingStrategy (for the reasons I've explained above). The type hint is the interface so that you can change the default strategy class if you need to adjust it). I've also added a comment about why this is the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff
src/Symfony/Bundle/FrameworkBundle/HttpKernel.php
((13 lines not shown))
/**
* 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
@stof Collaborator
stof added a note

you should add a trigger_error call in the constructor

@fabpot Owner
fabpot added a note

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff
src/Symfony/Bundle/FrameworkBundle/HttpKernel.php
@@ -67,6 +33,8 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
* @param array $query An array of request query parameters
*
* @return Response A Response instance
+ *
+ * @deprecated in 2.2, will be removed in 2.3
@stof Collaborator
stof added a note

ou should add trigger_error

@stof Collaborator
stof added a note

and what would be the replacement ?

@fabpot Owner
fabpot added a note

done. There is no replacements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ghost Show outdated diff Hide outdated diff Unknown commented on an outdated diff
.../DependencyInjection/ContainerAwareHttpKernelTest.php
@@ -9,16 +9,31 @@
* file that was distributed with this source code.
*/
-namespace Symfony\Bundle\FrameworkBundle\Tests;
+ namespace Symfony\Component\HttpKernel\Tests;
@ghost
ghost added a note

Looks like an extra space crept in at the beginning.

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

@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

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

@stof
Collaborator

@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

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

@fabpot
Owner

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

@fabpot fabpot referenced this pull request from a commit
@fabpot fabpot merged branch fabpot/kernel-refactor (PR #6459)
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.
b58e8ce
@fabpot fabpot merged commit 76fefe3 into symfony:master
@ghost

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

@fabpot
Owner

@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

@lsmith77
Collaborator

where is the renderer property supposed to be populated?

@mmucklo mmucklo referenced this pull request from a commit
@fabpot fabpot merged branch fabpot/kernel-refactor (PR #6459)
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.
307b16a
@hackzilla hackzilla referenced this pull request from a commit
@fabpot fabpot merged branch fabpot/kernel-refactor (PR #6459)
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.
39fb3c9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 10, 2013
  1. @fabpot
  2. @fabpot

    [TwigBridge] added a render_* function to ease usage of custom render…

    fabpot authored
    …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')) }}
  3. @fabpot
  4. @fabpot
  5. @fabpot
  6. @fabpot
  7. @fabpot
  8. @fabpot

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

    fabpot authored
    …nce now and not part of any interface anyway)
  9. @fabpot

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

    fabpot authored
    …emplating engine is available (the latter being mostly useful when testing)
  10. @fabpot
  11. @fabpot
  12. @fabpot
  13. @fabpot
Something went wrong with that request. Please try again.