-
Notifications
You must be signed in to change notification settings - Fork 178
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
Fix compatibility with HTTP specs about cacheable status codes and methods #200
base: master
Are you sure you want to change the base?
Fix compatibility with HTTP specs about cacheable status codes and methods #200
Conversation
02f12cc
to
50570fa
Compare
9e9a5de
to
4dbdd9f
Compare
@GromNaN Thank you for your time and to take care of this. |
Not something I am willing to merge. |
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. |
Hello @thePanz @mkopinsky Can you take a look at this patch? |
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. |
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 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. |
19e6cd1
to
559a56d
Compare
…es and methods - Add pages with status codes 200, 203, 300, 301, 302, 404, 410 to cache. - Add to cache response from HEAD method request.
559a56d
to
97b28a3
Compare
…thods - Add pages with response 200, 203, 300, 301, 302, 404, 410 to cache. - Add to cache response from HEAD method request.
97b28a3
to
9dfd628
Compare
Hey Alexandre,
I might be wrong, but I see nothing preventing this.
Yes, you're right. I've used inaccurate wording :) Again, it's not me who should decide on whether this should be merged or not :) Cheers. |
Hi Ivan,
@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:
However for a project where there are more than 10 million of landing pages on cache. What is the best solution between :
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 |
Fixes: #199