Skip to content

Update configurations to work with Cloudflare as our CDN [WEB-3107, WEB-3108] #825

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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

nikhiltri
Copy link
Member

No description provided.

Copy link

github-actions bot commented May 20, 2025

Automated PR Summary

This PR integrates support for the Cloudflare CDN into an existing project that previously only supported AWS CloudFront. It refactors CDN-related components to be more generic, facilitating operations with either CloudFront or Cloudflare based on configuration settings. The PR introduces new commands for CDN IP updates and cache invalidation, updates existing commands and middleware for these operations, and adjusts configurations and environmental settings to accommodate these changes.

Potential bugs

  1. The InvalidateCDN command relies on configuration flags (services.cloudflare.enabled and services.cloudfront.enabled) to determine which CDN's cache to invalidate. If both flags are mistakenly set to true, the command may not behave as expected, potentially sending invalidation requests to both CDNs when only one is intended.
  2. The UpdateCDNIps command fetches CDN IP addresses from Cloudflare and stores the response in a local file. If the response from Cloudflare is not in the expected format or is incomplete, subsequent operations relying on this data (e.g., the TrustProxies middleware) could behave unexpectedly or throw errors.
  3. The deletion of the UpdateCloudfrontIps and InvalidateCloudfront commands could lead to unforeseen issues if other parts of the project still depend on these commands. This could occur if not all instances of these command calls have been adequately refactored or documented.

@nikhiltri nikhiltri force-pushed the feature/cloudflare branch from 6b3d139 to 49a8c08 Compare May 21, 2025 14:48
@nikhiltri nikhiltri force-pushed the feature/cloudflare branch from 49a8c08 to 332f5df Compare June 6, 2025 21:28
Copy link

deepsource-io bot commented Jun 6, 2025

Here's the code health analysis summary for commits ffada01..332f5df. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource PHP LogoPHP❌ Failure
❗ 3 occurences introduced
View Check ↗
DeepSource JavaScript LogoJavaScript✅ SuccessView Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

Copy link

sonarqubecloud bot commented Jun 6, 2025

Copy link
Member

@zachgarwood zachgarwood left a comment

Choose a reason for hiding this comment

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

Looks good, just left a couple FYIs

]);

$contents = curl_exec($ch);
curl_close($ch);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be changed, but just FYI, file_get_contents can also be passed header options:
https://www.php.net/manual/en/function.file-get-contents.php#example-2353

But personally, I think that Guzzle has a more readable interface than either of these options.

@@ -49,4 +49,10 @@
'id' => env('GTM_ID', '')
],

'cloudflare' => [
'enabled' => (bool) env('CLOUDFLARE_ENABLED', false),
Copy link
Member

Choose a reason for hiding this comment

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

Setting the value in the .env to CLOUDFLARE_ENABLED=true or CLOUDFLARE_ENABLED=(true) should convert the value returned by env() to a boolean without the need to explicitly cast it:
https://laravel.com/docs/11.x/configuration#environment-variable-types

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.

2 participants