Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[HttpKernel][HttpCache] SSI-include support #6108

New issue

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

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

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

[HttpKernel][HttpCache] SSI-include support #6108

wants to merge 2 commits into from

Conversation

kingcrunch
Copy link
Contributor

Exactly like the already existing ESI-support, but for SSI. Idea is to allow NGinx (or other servers, that support SSI, but not ESI) cache parts of the site like it does varnish with ESI, but in enviroments, where a dedicated varnish is overhead.

@lsmith77
Copy link
Contributor

i have talked to @Crell about this too .. he said that there were people in the Drupal community interested in this too.

@kingcrunch
Copy link
Contributor Author

Interesting, couldn't find anything about "symfony2 SSI" anywhere. Now, that I know, that I'm not the only one, I'll started to play around with it a little bit more serious and I have "a kind of working" solution. Guess I can provide a first PR soon

@Crell
Copy link
Contributor

Crell commented Nov 24, 2012

The discussion in Drupal has been that FrameworkBundle:HttpKernel::render() should not hard code various options but instead be pluggable, so that various options (other than raw subrequest) can be swapped in as appropriate, such as nginx's SSI. We haven't gone as far as determining exactly how. The two possibilities are a kernel.render event that lets various listeners decide if they want to handle rendering the request, or simply an array of injected objects.

The other question that comes to mind is whether or not it's logical to support multiple render mechanisms at the same time. Would I site be using ESI and SSI at the same time? Likely not. ESI and hInclude at the same time? Maybe, I don't know. That would impact how that gets implemented.

@kingcrunch
Copy link
Contributor Author

The other question that comes to mind is whether or not it's logical to support multiple render mechanisms at the same time. Would I site be using ESI and SSI at the same time? Likely not. ESI and hInclude at the same time? Maybe, I don't know. That would impact how that gets implemented.

Thought about this too and come to the conclusion, that you can define the renderer yourself explictly like

{% render "AcmeBundle:Acme:myaction" with {}, {standalone:'ssi'} %}

but on the other side you can pass true, what can be redefined as "first appropiate renderer". Now it's hardcoded as "esi", because the moment it was defined only esi was available.

    {% render "AcmeBundle:Acme:myaction" with {}, {standalone:true} %}

The discussion in Drupal has been that FrameworkBundle:HttpKernel::render() should not hard code various options but instead be pluggable, so that various options (other than raw subrequest) can be swapped in as appropriate, such as nginx's SSI. We haven't gone as far as determining exactly how. The two possibilities are a kernel.render event that lets various listeners decide if they want to handle rendering the request, or simply an array of injected objects.

Yes, while implementing I found, that I'm not really happy with my current copy&pasty-into-HttpKernel solution. I see, that when using esi the HttpKernel fetches the Esi-class out of the DIC

$this->container->has('esi') && $this->container->get('esi')

One solution can be to change this to a dynamic identifier. I guess it requires to rename the service names to avoid conflicts (Don't know, if "renderer" is the right name here)

$this->container->has('renderer.' . $config['standalone']) && $this->container->get('renderer.' . $config['standalone'])

Another question, that comes into my mind: What was the reason, that one must explicitly enable esi-support in configuration? Isn't it sufficient, when the proxy/webserver set the corresponding header Surrogate-Capability?

framework
    esi:
        enabled: true

Must say, that I don't know, if it's OK to change config-options now ... :X ?

@kingcrunch
Copy link
Contributor Author

OK, must say, that I don't know, what should I do now (PR or not, and such), because the implementation is obvious incomplete. I just pushed it to KingCrunch@0f5daa4738 if you want to have a look at it.

What I can say:

  • HttpCache-handling is completely missing as this seems to be more complicated. The class is directly bound to a Esi-instance.
  • It's mostly copy&paste.
  • I played with tags and it seems to be quite easy with HttpKernel, but I guess this means, that the renderer-instances were always injected independently wether they are required, or not. I don't know, if this is wanted.

So, up for discussion

@kingcrunch
Copy link
Contributor Author

Hadn't enough time anymore, so it's only possible to inject SSI-tags (instead of ESI) with {standalone: 'ssi'}.

@fabpot
Copy link
Member

fabpot commented Dec 20, 2012

I'm currently working on refactoring the HttpKernel class from FrameworkBundle so that we can use this logic in products not using the full-stack framework like Drupal and Silex. I'm almost done and I will submit a PR soon.

@fabpot fabpot mentioned this pull request Dec 21, 2012
fabpot added a commit that referenced this pull request Jan 11, 2013
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.
@kingcrunch
Copy link
Contributor Author

Ive waited for the kernel-refactoring to be merged. I'll close this an open a new PR once I'm done with my own refactoring

@kingcrunch kingcrunch closed this Jan 11, 2013
@fabpot
Copy link
Member

fabpot commented Apr 22, 2013

@kingcrunch Any news on this feature based on the new HttpKernel fragment framework?

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

Successfully merging this pull request may close these issues.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.