Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alquerci
Copy link
Contributor

@alquerci alquerci commented Oct 19, 2018

Fixes: #199

  • Add pages with status codes 200, 203, 300, 301, 302, 404, 410 to cache.
  • Add to cache response from HEAD method request.

@alquerci alquerci force-pushed the add-ability-to-not-only-cache-http-200-and-get-method branch 6 times, most recently from 02f12cc to 50570fa Compare October 25, 2018 00:40
@alquerci alquerci changed the title [WIP] Add ability to not only cache HTTP 200 and GET method Add ability to not only cache HTTP 200 and GET method Oct 25, 2018
@alquerci alquerci force-pushed the add-ability-to-not-only-cache-http-200-and-get-method branch 2 times, most recently from 9e9a5de to 4dbdd9f Compare October 25, 2018 08:01
@alquerci
Copy link
Contributor Author

@j0k3r @GromNaN Can you look for this patch?

I provided all required tests for it.

@alquerci
Copy link
Contributor Author

@GromNaN Thank you for your time and to take care of this.

@GromNaN
Copy link
Collaborator

GromNaN commented Oct 26, 2018

Not something I am willing to merge.
We are going to archive this repository #102.

@alquerci
Copy link
Contributor Author

alquerci commented Oct 26, 2018

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.

@alquerci
Copy link
Contributor Author

alquerci commented Oct 29, 2018

Hello @thePanz @mkopinsky

Can you take a look at this patch?

@e1himself
Copy link
Contributor

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.

@alquerci
Copy link
Contributor Author

alquerci commented Oct 30, 2018

Hello @e1himself

Indeed, but I proposed this patch because it fix a business issue.

@alquerci
Copy link
Contributor Author

alquerci commented Nov 9, 2018

@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.

@e1himself
Copy link
Contributor

e1himself commented Nov 9, 2018

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.

@alquerci
Copy link
Contributor Author

alquerci commented Nov 9, 2018

@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 alquerci changed the title Add ability to not only cache HTTP 200 and GET method Fix compatibility with HTTP specs about cacheable status codes and methods Nov 9, 2018
@alquerci alquerci force-pushed the add-ability-to-not-only-cache-http-200-and-get-method branch 2 times, most recently from 19e6cd1 to 559a56d Compare November 10, 2018 23:18
…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.
@alquerci alquerci force-pushed the add-ability-to-not-only-cache-http-200-and-get-method branch from 559a56d to 97b28a3 Compare November 11, 2018 00:30
…thods

- Add pages with response 200, 203, 300, 301, 302, 404, 410 to cache.
- Add to cache response from HEAD method request.
@alquerci alquerci force-pushed the add-ability-to-not-only-cache-http-200-and-get-method branch from 97b28a3 to 9dfd628 Compare November 11, 2018 00:59
@e1himself
Copy link
Contributor

e1himself commented Nov 26, 2018

@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.

@alquerci
Copy link
Contributor Author

alquerci commented Nov 26, 2018

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 :

  1. Migrate the fully cache system to Varnish or other system
  2. Contribute to the framework to provide the patch to share it to other users using the view_cache.
  3. 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

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.

3 participants