8000 [HttpKernel][HttpCache][RFC] SSI kernel proxy by kingcrunch · Pull Request #6766 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpKernel][HttpCache][RFC] SSI kernel proxy #6766

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

[HttpKernel][HttpCache][RFC] SSI kernel proxy #6766

wants to merge 6 commits into from

Conversation

kingcrunch
Copy link
Contributor

Bug fix: no
Feature addition: yes
Backwards compatibility break: possible
Deprecations: possible
Fixes the following tickets: ~
Todo: see below
License of the code: MIT
Documentation PR: ~

Yes this is my second pull request about SSI-implementation (third, if you count the closed pre-HttpKernel-refactoring PR), but I came to the conclusion, that it could be more appropiate to separate HttpCache and the substitution of the include tags from each other, but before I put to much effort in an unwanted idea, I want to hear some opinions.

The motivation/idea

  • Separation of concerns: HttpCache can concentrate on its purpose. Therefore I would separate the esi-substitution too. This should make both easier to maintain
  • The substitution is useable without the cache, for example as a fallback, or for development, where/when caching is not wanted.

As a sideeffect HttpCache (when used) will cache the original content including the ssi-tag and the parsing of the tag will occur on every request. But we can get rid of eval() 😉

// with cache
$kernel = new SsiKernelProxy(new AppCache($kernel);
// without cache (just subsitute)
$kernel = new SsiKernelProxy($kernel);

The first commit contains the SSI rendering strategy, which is exactly the same as in my other PR.

The second commit is the single class SsiKernelProxy. When ESI should be separated too I would rewrite it into a IncludeKernelProxy-class, which consumes one or more IncludeSubstitutionStrategy-objects, that handles the differences.

// example 
$kernel = new SsiKernelProxy(new AppCache($kernel,
    array(new SsiSubstitutionStrategy,new EsiSubstitutionStrategy)
);

TODO

  • For now the proxy doesn't respect any Surrogate-*-header, but I probably should

  • When from a server a Surrogate-Capability with the corresponding value (I guess SSI/1.0?) is set, this proxy should passthrou the request without handling, so it act as a fallback

  • I would like to handle maxage and s-maxage separately:

    $maxage = min($maxages);
$sMaxage  = $shared ? min($sMaxAge) : 0;

    In the current esi-implementation maxage is always set to 0, but when I get it right it is more probably, that a on this way constructed page is private, than that it is public.

The current implementation already works in my demo-application.

lsmith77
lsmith77 reviewed Jan 17, 2013
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
* @param array $config An SSI configuration array
* @param XmlFileLoader $loader An XmlFileLoader instance
*/
private function registerSsiConfiguration (array $config, XmlFileLoader $loader) {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor issue .. { needs to be on the next line

@lsmith77
Copy link
Contributor

i think this separation makes sense. HttpCache imho should become its own component especially because I believe there is still a lot of potential work people might want to put in it to improve cache invalidation etc.

that being said .. i didnt look really review the code itself in much detail aside from the CS issues i noted.

@kingcrunch
Copy link
Contributor Author

I found a little bit time 😄

The SsiKernelProxy works. I didn't touch the Esi-substitution yet, because I got the message, that the first beta comes out tomorrow and I must admit, that I am not very familar on how the process goes on now, so I just wanted to get SSI updated for now.

However, the Esi-substitution will look very similar to this one, except that

  • Both classes inherits from the same base "include-proxy", or
  • The "include-proxy" will be more generic and consumes one or more include-strategies.

It's late here, so currently don't know, which is the better solution right now. I'll think about it later (also depending on whether this PR still can go into 2.2, or not 😉).

The Esi-substitution additionally includes, that I plan to strip all ESI-related things from HttpCache, which will make this PR a more or less heavy BC-break.

@kingcrunch
Copy link
Contributor Author

Removed ESI from HttpCache now. Probably many things from HttpCache/Esi*-classes are unused now too, but I didn't looked at it for now. Maybe we could remove it completely: The EsiIncludeStrategy doesn't need it at all and only the EsiRenderingStrategy uses two of Esis methods.
The (as named by me) IncludeProxy is gone into an own namespace HttpKernel/IncludeProxy

$kernel = new IncludeProxy (new HttpCache($kernel), array(new EsiIncludeStrategy, new SsiIncludeStrategy));
// or just
$kernel = new IncludeProxy ($kernel, array(new EsiIncludeStrategy, new SsiIncludeStrategy));

The SsiRenderingStrategy is still working and works like the other renderers.

Any suggestions?

stof
stof reviewed Jan 23, 2013
View reviewed changes
src/Symfony/Component/HttpKernel/IncludeProxy/IncludeProxyInterface.php
/**
* @author Sebastian Krebs <krebs.seb@gmail.com>
*/
interface IncludeProxyInterface extends HttpKernelInterface,TerminableInterface
Copy link
Member

Choose a reason for hiding this comment

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

this interface is useless IMO

@stof
Copy link
Member
stof commented Jan 23, 2013

This is flawed. It does not handle the headers of the final response properly. For instance, the max age of the final response should be the smaller age among all responses used to build it.

@kingcrunch
Copy link
Contributor Author

@stof Maybe I don't understand you, but I thought it does ❓

$maxAge = min($response->headers->getCacheControlDirective('max-age'), $subResponse->headers->getCacheControlDirective('max-age'));
$sMaxAge = min($response->headers->getCacheControlDirective('s-maxage'), $subResponse->headers->getCacheControlDirective('s-maxage'));

It takes the lowest value of all responses (including null as possible value). It is somehow buried in the response-handling-closure. I need to refactor it anyway (it looks a little bit scary to me), so any suggestions regarding this? Ive seen, that there were some changes in HttpCache, so my first idea now is to look how it is solved there. However, I didn't want to disable the browser-caching completely like it was solved in the old HttpCache-ESI-substitution.

Oh, and when anybody has suggestions how to handle the validation headers, I would like to hear them too 😄 Did I forgot other headers?

@stof
Copy link
Member
stof commented Jan 23, 2013

Well, the other caching headers probably need to be handled too (Last-Modifie-Since & co)

@kingcrunch kingcrunch closed this Feb 11, 2013
@hason hason mentioned this pull request Oct 31, 2013
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kingcrunch @lsmith77 @stof
0