Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel][FrameworkBundle] SSI support#10702
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what about here ? shouldn't be Esi renamed too ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Couldn't decide myself (and later I forgot). Problem is, to what should I rename it? You can overwrite it in your applicationsAppCache-class with
protectedfunctioncreateSurrogate(){returnnewSsi();}
That's not perfect, but it works and is backward compatible. The other solution is to provide two separateFrameworkBundle:HttpCache\HttpCache-classes...
stof commentedApr 14, 2014
Please implement the featur ein a BC way, otherwise it means it cannot be merged until we move to 3.0 |
kingcrunch commentedApr 14, 2014
Fixed. Did I miss something? Also do you have an idea about how to solve this? The So in my opinion it is fine for now, but maybe somebody has an idea? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks wrong, first param shouln't be optional if second is required...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it is already the existing signature.= null is not really about making the argument optional, but about making it nullable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Exactly (I've tried it out...)
murnieza commentedApr 14, 2014
Really looking forward to see this functionality in Symfony 👍 |
kingcrunch commentedApr 14, 2014
So, updated once more. Sorry for the inconvenience with the BC I've mentioned in the source, that the components are deprecated since 2.5, but actually I don't really know. It's too late for 2.5, isn't? Are there any other concerns? |
stof commentedApr 15, 2014
@kingcrunch yes it is. 2.5 is already feature-frozen. It will come in 2.6 now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually, this is the "virtual" attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks wrong to me.
fabpot commentedJun 6, 2014
@kingcrunch Looks good to me. Can you fix the minor issues I've mentioned in the comments? Thanks. |
fabpot commentedJun 18, 2014
👍 |
fabpot commentedJun 18, 2014
@kingcrunch Thanks for that. Can you fix the fabbot errors (mainly about using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is not BC. If someone overwritescreateEsi, it is not called anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch, you're right.
kingcrunch commentedJun 20, 2014
Rebase and pushed. Actually some days ago already, but I don't know, if it's fine to "ping" after every commit :X |
fabpot commentedJun 20, 2014
@kingcrunch Pinging is fine, but we now need 2 +1 from the core team before being able to merge this PR.http://symfony.com/doc/current/contributing/code/core_team.html#code-development-rules |
fabpot commentedJun 26, 2014
kingcrunch commentedJul 23, 2014
cough ping@stof@romainneutron@nicolas-grekas |
fabpot commentedJul 25, 2014
Thank you@kingcrunch. |
This PR was merged into the 2.6-dev branch.Discussion----------[HttpKernel][FrameworkBundle] SSI support| Q | A| ------------- | ---| Bug fix? | No| New feature? | Yes| BC breaks? | No| Deprecations? | No| Tests pass? | Yes| Fixed tickets |#9419 (,#10684)| License | MITIt does not support comments, or alternative URIs, or "continue" in case of errors. Maybe I can workaround that, but I've decided to left it out for this PR. Especially as far as I can see a "alternative URIs"-hack would _always_ lead to two requests, even if it's not needed.Commits-------06cea08 SSI support
pborreli commentedJul 25, 2014
Thanks for this nice work@kingcrunch and all reviewers. This is a great new feature ✨ . |
It does not support comments, or alternative URIs, or "continue" in case of errors. Maybe I can workaround that, but I've decided to left it out for this PR. Especially as far as I can see a "alternative URIs"-hack wouldalways lead to two requests, even if it's not needed.