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

Cloudflare support #507

Closed
wants to merge 30 commits into from
Closed

Cloudflare support #507

wants to merge 30 commits into from

Conversation

simonrjones
Copy link
Contributor

Background
I'm working on the w3.org redesign project and we'd like to add Cloudflare support to FOSHttpCache so we can use this library to help control full page caching.

Changes

  • Add clear, purge and invalidateTags methods for Cloudflare API
  • Purge by path queues requests and sends in one request (can send 30 purge path requests per API request)
  • Add docs & test (using Mockery)

Notes
I've tested this with real-world code (w3.org) for clear and purge. I need to test clearing tags but manual API tests work fine.

I can't build the docs locally so I'm not sure if I've got the formatting right for this. Also, the docs confused me a little since I had to specify the Cloudflare API URL in the HttpDispatcher to get this working. Not sure if this is correct & expected when working with an external CDN like Cloudflare:

 $httpDispatcher = new HttpDispatcher(['https://api.cloudflare.com']);
 $cloudflare = new Cloudflare($httpDispatcher);

For details about Cloudflare API:

https://api.cloudflare.com/#zone-purge-all-files
https://developers.cloudflare.com/cache/how-to/purge-cache#h_6d756ac9-c476-45e8-a5d4-e2a6e45d9dc7

Related discussion: #457

@simonrjones simonrjones mentioned this pull request Dec 17, 2021
@simonrjones
Copy link
Contributor Author

I am not certain why the PHPStan, Travis CI and Scrutinizer status checks are failing. Can anyone give me any tips on this, or are there issues with the current GitHub action setup?

@diimpp
Copy link

diimpp commented Dec 17, 2021

Judging by commit history, CI is dead since May.

@simonrjones
Copy link
Contributor Author

@diimpp The CI seems to fail on:

  • static / PHPStan src - I'm not sure what this does since phpstan-tests already runs phpstan and passes fine?
  • I can't really see where Travis is failing unless this is from the risky & skipped tests? But if you have GitHub Actions now which run PHPUnit is there a need for TravisCI as well?
  • Scrutinizer fails with the following error. This looks like a configuration issue.

The code coverage data was not received within the specified time. Please make sure your third-party service is configured to send code coverage data.

@diimpp
Copy link

diimpp commented Dec 20, 2021

@diimpp The CI seems to fail on:

* static / PHPStan src - I'm not sure what this does since phpstan-tests already runs phpstan and passes fine?

* I can't really see where [Travis is failing](https://app.travis-ci.com/github/FriendsOfSymfony/FOSHttpCache/jobs/552574390) unless this is from the risky & skipped tests? But if you have GitHub Actions now which run PHPUnit is there a need for TravisCI as well?

* Scrutinizer fails with the following error. This looks like a configuration issue.

The code coverage data was not received within the specified time. Please make sure your third-party service is configured to send code coverage data.

/cc @dbu

@dbu
Copy link
Contributor

dbu commented Dec 20, 2021

thanks a lot for the pull request! and happy to hear this library is powering w3.org 😊

travis-ci changed their offering and many project are moving away to github actions since then. i moved my other projects, but got stuck in #500 with trying to set up varnish in github actions. i will give it another go.

scrutinizer fails because it would be the phpunit build that should push data to it...

phpstan build probably fell apart with the symfony 6 release, i will look into that as well.

the rendered documentation is built with an action in readthedocs - when you click on "Details" in the build action for readthedocs, you can check the docs for this branch.

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i looked at the code, and seems good to me. i commented on a few nitpicks, and replied to your question about the HttpDispatcher.

doc/proxy-clients.rst Outdated Show resolved Hide resolved
doc/proxy-clients.rst Outdated Show resolved Hide resolved
src/ProxyClient/Cloudflare.php Show resolved Hide resolved
src/ProxyClient/Cloudflare.php Outdated Show resolved Hide resolved
the cache by URL and clearing all cache items. You need a `Cloudflare Enterprise`_ account to purge by cache tags.

For purging the cache by URL please see the docs `Cloudflare Purge by URL`_ for information about headers you can
pass to clear the cache correctly.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am a bit confused by this sentence. does the user have to configure something specifically, or do invalidation requests in a specific way? or is that more of an implementation note? if the later, i would put it in the class phpdoc of the cloudflare ProxyClient

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It contains details on how Cloudflare purges the cache by URL and which headers you need to pass to clear the cache correctly (if these are set in HTTP responses).

I have added this to the code phpdoc, but I think this is useful to know about in the docs too since it may confuse users if the cache is not cleared as they expect. I've reworded to try to make this clearer.

src/ProxyClient/Cloudflare.php Outdated Show resolved Hide resolved
dbu and others added 5 commits December 21, 2021 08:48
setting up old varnish versions did not work, commenting out the legacy
builds
* have no risky tests
* fix deprecations
@dbu
Copy link
Contributor

dbu commented Dec 21, 2021

i fixed the build in the master branch. can you rebase please?

@simonrjones
Copy link
Contributor Author

Rebased master & updated my branch with a few changes as suggested above.

@dbu
Copy link
Contributor

dbu commented Jan 12, 2022

finished in #516 - thanks a lot @simonrjones for this contribution!

@dbu dbu closed this Jan 12, 2022
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.

4 participants