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

Add support for Nginx Cache #6308

Open
wants to merge 6 commits into
base: 2.5
Choose a base branch
from
Open

Add support for Nginx Cache #6308

wants to merge 6 commits into from

Conversation

ampaze
Copy link
Contributor

@ampaze ampaze commented Oct 18, 2021

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related issues/PRs #6302
License MIT

What's in this PR?

Add support for Nginx Cache

Add support for Nginx Cache
Add support for Nginx Cache
Add support for Nginx Cache
@ampaze
Copy link
Contributor Author

ampaze commented Oct 18, 2021

This code is working fine for me, not sure what PHP Lint / PHPStan wants?

@alexander-schranz
Copy link
Member

alexander-schranz commented Oct 18, 2021

@ampaze Thank you for the Pull Request. I think it is missing an import (use statement) of the Exception class, which is used when tags are enabled. But did not yet have time to test it out.

@alexander-schranz alexander-schranz added the Feature New functionality not yet included in Sulu label Oct 18, 2021
@ampaze
Copy link
Contributor Author

ampaze commented Nov 3, 2021

The PHP Lint error is in files that where not modified with this pull request.

Is there something else that prevents this PR from being merged?

@alexander-schranz
Copy link
Member

alexander-schranz commented Nov 9, 2021

@ampaze This part looks fine. We also need to add a nginx example configuration like we are doing in the docs for varnish currently, which will be based on the current nginx vhost file if possible, else its fine for me as a reverse proxy configuration. I currently found only https://foshttpcache.readthedocs.io/en/latest/nginx-configuration.html which configuration seems not longer work, atleast for the free nginx version.

Can you create a Pull Request with that also? Think I would like the current varnish configuration add it as a cookbook documentation Caching with nginx

@ampaze
Copy link
Contributor Author

ampaze commented Nov 10, 2021

I appreciate your desire to add better nginx support, but having an agreed upon example configuration ready before merging this will most likely take a long time. Smart caching with Nginx is unfortunately a bit more complex and not really copy and paste friendly. I am wiling to make my config available but I am not sure it is best suited for everyone out there.

Can you create a Pull Request with that also? Think I would like the current varnish configuration add it as a cookbook documentation Caching with nginx

Yes I hope to work on that when I had time to finish the Tagged Cached Invalidation for Nginx.

Be aware: a complete and clean solution for this is currently not possible, as you don't want to change the X-Reverse-Proxy-TTL header (or make it configurable). See #6307

@alexander-schranz
Copy link
Member

alexander-schranz commented Nov 10, 2021

Be aware: a complete and clean solution for this is currently not possible, as you don't want to change the X-Reverse-Proxy-TTL header (or make it configurable). See #6307

This is I think the easiest thing as you can set the s-maxage by adding a new subsriber here : https://github.com/sulu/skeleton/blob/07211a90df12f1a75c4bc40c782c25490857dc19/src/Kernel.php#L42-L46

if ($response->headers->has(SuluHttpCache::HEADER_REVERSE_PROXY_TT)) {
    $response->setSharedMaxAge($response->headers->get(SuluHttpCache::HEADER_REVERSE_PROXY_TTL));
}

You could add that subscriber(CustomTtlToShareMaxAgeListener) to the SuluHttpCache so it just need to be added to the Kernel HttpCache when nginx is used. That is more a documentation thing then.

Yes I hope to work on that when I had time to finish the Tagged Cached Invalidation for Nginx.

As nginx does not support tagged caching out of the box and nginx could be run on another server, the config in our case should just simple to allow caching and purging. So at current state it would be good to have a simple nginx config which take the s-maxage into account cache it by the defined cache lifetime and that the url is purged when a page is republished. I know that you want to implement cache tag invalidation on php side but this I think will only work in cases the php application runs on same system as your nginx server, so this I think is not required for a basic nginx support.

@ampaze
Copy link
Contributor Author

ampaze commented Nov 10, 2021

Fair enough. Keeping it simple is always good :)

the config in our case should just simple to allow caching and purging.

Unfortunately the free version does not support purging and you would need to compile nginx yourself to add support, like described in the link you posted (https://foshttpcache.readthedocs.io/en/latest/nginx-configuration.html). (This can be worked around with something like proxy_cache_bypass but I have not done so yet.)

This is one reason I think directly accessing the cache files is much more hassle free, but as you mentioned, this would be more difficult if nginx is used only for caching on a different server. But imho using nginx for caching dynamic pages is mostly done because nginx is already used for everything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New functionality not yet included in Sulu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants