symfony1
symfony1 copied to clipboard
FriendsOfSymfony1
Fix compatibility with HTTP specs about cacheable status codes and methods
Fixes: #199
- [x] Add pages with status codes 200, 203, 300, 301, 302, 404, 410 to cache.
- [x] Add to cache response from HEAD method request.
@j0k3r @GromNaN Can you look for this patch?
I provided all required tests for it.
Not something I am willing to merge. We are going to archive this repository #102.
Noted, then the goal will be to find a new repository based on this version and able to maintain using almost same quality as Symfony.
As I see on the discussion #102 there are many forks but we only need one.
The goal of this patch is to avoid adding another fork.
The goal of this project is to give the ability to migrate the most smooth to Symfony4+. The introduction of the service container was a good things and the first step. The next step is to use Symfony4+ DIC.
My point of view on pushing this repo further is to remove code instead of writing more. View caching, for example, should be completely unloaded from the framework to a specialized tool like Varnish.
Hello @e1himself
Indeed, but I proposed this patch because it fix a business issue.
@e1himself What do you think if we add a configuration parameter to activate this patch (to minimize any BC breaks)?
Sometimes a bug can be a feature.
What do you think if we add a configuration parameter to activate this patch (to minimize any BC breaks)?
If you ask me, I'd say this fix should live in your codebase as a set of stock framework classes overrides. You can do that with factories.yml (even though it'll require quite lots of overrides).
I believe we should not bring new features into this repo.
But I'm neither owning nor maintaining this repo. Thus it's just an opinion.
@e1himself Thank you for your feedback.
This PR exists because there is no way to provide this patch as a set of stock framework classes overrides.
This PR provide a bug fix for HTTP spec.
Your message is a bit confusing as you start to discuss about a fix, but you finish your message as a feature.
@alquerci
Hey Alexandre,
... because there is no way to provide this patch as a set of stock framework classes overrides.
I might be wrong, but I see nothing preventing this.
Your message is a bit confusing as you start to discuss about a fix, but you finish your message as a feature.
Yes, you're right. I've used inaccurate wording :) Technically this is a bug fix. But I wanted to say that this PR is bringing something new. Even if it's a missing aspect of an existing feature.
Again, it's not me who should decide on whether this should be merged or not :)
Cheers.
Hi Ivan,
Again, it's not me who should decide on whether this should be merged or not :)
@e1himself Indeed, but as open-source project the point of view of any code reviewer can be taken account.
As you shared your point of view it means that you are involved on the project. And just for that, thank you.
Aside I am agreed with you about:
View caching, for example, should be completely unloaded from the framework to a specialized tool like Varnish.
However for a project where there are more than 10 million of landing pages on cache. What is the best solution between :
- Migrate the fully cache system to Varnish or other system
- Contribute to the framework to provide the patch to share it to other users using the
view_cache. - Fork the framework and apply the patch, but closing door to open-source community (the company have not enough resources to maintain a custom version).
While solutions first and second are relatively reasonable the third is something to avoid if we want to have a useful @FriendsOfSymfony1 community (at least until the last sf1 project will migrate)
I think that such discussion is very constructive for the mindset of @FriendsOfSymfony1 community.
cc @thePanz