Conversation
|
Warning Rate limit exceeded@f3l1x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds CDN support: a Latte extension (macro, filter, provider), a DI CompilerExtension to register it, an AST node for the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant DI as DI Container
participant CdnDI as CdnExtension (DI)
participant LatteFac as LatteFactory
participant Latte as Latte Engine
participant CdnExt as CdnExtension (Latte)
participant CdnNode as CdnNode
participant Filter as CdnFilter / Provider
App->>DI: register/compile extensions
DI->>CdnDI: beforeCompile()
CdnDI->>DI: resolve LatteFactory
CdnDI->>LatteFac: add setup to include CdnExtension
LatteFac->>Latte: initialize engine with CdnExt
App->>Latte: render template with {cdn "path"} or |cdn
Latte->>CdnNode: parse tag → CdnNode.create()
Latte->>CdnNode: print() → emits code calling provider/filter
CdnNode->>Filter: call cdnBuilder / CdnFilter::filter(path, config)
Filter->>Filter: normalize URL, join base+path, maybe append time
Filter-->>Latte: return final URL string
Latte-->>App: rendered output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Potential focus areas:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.docs/README.md(2 hunks)src/DI/CdnExtension.php(1 hunks)src/Extensions/CdnExtension.php(1 hunks)src/Extensions/Node/CdnNode.php(1 hunks)src/Filters/CdnFilter.php(1 hunks)tests/Cases/DI/CdnExtension.phpt(1 hunks)tests/Cases/Extensions/CdnExtension.phpt(1 hunks)tests/Cases/Filters/CdnFilter.phpt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/DI/CdnExtension.php (2)
src/Exception/LogicalException.php (1)
LogicalException(7-10)src/Extensions/CdnExtension.php (1)
CdnExtension(8-61)
src/Extensions/CdnExtension.php (2)
src/Extensions/Node/CdnNode.php (1)
CdnNode(11-37)src/DI/CdnExtension.php (1)
CdnExtension(18-46)
🪛 markdownlint-cli2 (0.18.1)
.docs/README.md
64-64: Hard tabs
Column: 1
(MD010, no-hard-tabs)
71-71: Hard tabs
Column: 1
(MD010, no-hard-tabs)
72-72: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
91-91: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
src/Extensions/Node/CdnNode.php (1)
11-37: LGTM! Clean AST node implementation.The node correctly implements Latte's AST patterns:
- Properly parses expressions via the tag parser
- Generates secure output using
escapeHtmlAttr- References the
cdnBuilderprovider for URL construction- Implements iterator for AST traversal
tests/Cases/Filters/CdnFilter.phpt (1)
1-78: LGTM! Comprehensive test coverage.The test suite thoroughly validates the CDN filter functionality:
- Base URL construction with and without CDN
- Time-based cache busting in various scenarios
- Trailing slash normalization
- Query string preservation and appending
Edge cases are well covered, providing confidence in the implementation.
| public function getProviders(): array | ||
| { | ||
| return [ | ||
| 'cdnBuilder' => function (string $path): string { | ||
| $baseUrl = $this->config['url'] ?? ''; | ||
| $url = rtrim($baseUrl, '/') . '/' . ltrim($path, '/'); | ||
|
|
||
| if (($this->config['cacheBusting'] ?? false) === 'time') { | ||
| $separator = str_contains($url, '?') ? '&' : '?'; | ||
| $url .= $separator . 'time=' . time(); | ||
| } | ||
|
|
||
| return $url; | ||
| }, | ||
| ]; | ||
| } | ||
|
|
||
| public function getFilters(): array | ||
| { | ||
| return [ | ||
| 'cdn' => function (string $path): string { | ||
| $baseUrl = $this->config['url'] ?? ''; | ||
| $url = rtrim($baseUrl, '/') . '/' . ltrim($path, '/'); | ||
|
|
||
| if (($this->config['cacheBusting'] ?? false) === 'time') { | ||
| $separator = str_contains($url, '?') ? '&' : '?'; | ||
| $url .= $separator . 'time=' . time(); | ||
| } | ||
|
|
||
| return $url; | ||
| }, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Extract duplicated URL building logic.
The getProviders and getFilters methods contain identical URL construction and cache-busting logic (lines 30-39 duplicate lines 47-56). This violates the DRY principle and creates maintenance burden.
Based on the test file importing Contributte\Latte\Filters\CdnFilter, both methods should delegate to a shared implementation (likely CdnFilter::filter).
Suggested refactoring:
public function getProviders(): array
{
return [
- 'cdnBuilder' => function (string $path): string {
- $baseUrl = $this->config['url'] ?? '';
- $url = rtrim($baseUrl, '/') . '/' . ltrim($path, '/');
-
- if (($this->config['cacheBusting'] ?? false) === 'time') {
- $separator = str_contains($url, '?') ? '&' : '?';
- $url .= $separator . 'time=' . time();
- }
-
- return $url;
- },
+ 'cdnBuilder' => fn(string $path): string => CdnFilter::filter($path, $this->config),
];
}
public function getFilters(): array
{
return [
- 'cdn' => function (string $path): string {
- $baseUrl = $this->config['url'] ?? '';
- $url = rtrim($baseUrl, '/') . '/' . ltrim($path, '/');
-
- if (($this->config['cacheBusting'] ?? false) === 'time') {
- $separator = str_contains($url, '?') ? '&' : '?';
- $url .= $separator . 'time=' . time();
- }
-
- return $url;
- },
+ 'cdn' => fn(string $path): string => CdnFilter::filter($path, $this->config),
];
}Verify that CdnFilter::filter exists and accepts the config array:
🏁 Script executed:
#!/bin/bash
# Verify CdnFilter::filter method exists and check its signature
ast-grep --pattern $'class CdnFilter {
$$$
static function filter($$$) {
$$$
}
$$$
}'Length of output: 0
I need to investigate further to understand the actual structure of CdnFilter and confirm whether the refactoring suggestion is valid.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for CdnFilter class definition and methods
rg -nP "class\s+CdnFilter" --type=php -A 20
Length of output: 1032
🏁 Script executed:
#!/bin/bash
# Search for filter method in CdnFilter (not necessarily static)
rg -nP "function\s+filter\s*\(" --type=php -B 5 -A 10Length of output: 3816
🏁 Script executed:
#!/bin/bash
# Check CdnExtension.php imports and class definition
rg -nP "use|class CdnExtension" --type=php src/Extensions/CdnExtension.php -A 5Length of output: 208
🏁 Script executed:
#!/bin/bash
# List all PHP files in the Filters directory
fd --type f "\.php$" --path "*/Filters/*"Length of output: 233
I need to verify the full CdnExtension.php file and check if CdnFilter is imported.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full content of CdnExtension.php
cat src/Extensions/CdnExtension.php
Length of output: 1241
🏁 Script executed:
#!/bin/bash
# Check if CdnFilter is imported in CdnExtension.php
rg -n "CdnFilter" src/Extensions/CdnExtension.phpLength of output: 0
Extract duplicated URL building logic into CdnFilter::filter.
The getProviders and getFilters methods contain identical URL construction and cache-busting logic (lines 30-39 and 47-56). CdnFilter::filter at src/Filters/CdnFilter.php already implements this logic and accepts the config structure via its $parameters argument.
Refactor both methods to delegate to CdnFilter::filter:
+use Contributte\Latte\Filters\CdnFilter;
use Contributte\Latte\Extensions\Node\CdnNode;
use Latte\Extension;
public function getProviders(): array
{
return [
- 'cdnBuilder' => function (string $path): string {
- $baseUrl = $this->config['url'] ?? '';
- $url = rtrim($baseUrl, '/') . '/' . ltrim($path, '/');
-
- if (($this->config['cacheBusting'] ?? false) === 'time') {
- $separator = str_contains($url, '?') ? '&' : '?';
- $url .= $separator . 'time=' . time();
- }
-
- return $url;
- },
+ 'cdnBuilder' => fn(string $path): string => CdnFilter::filter($path, $this->config),
];
}
public function getFilters(): array
{
return [
- 'cdn' => function (string $path): string {
- $baseUrl = $this->config['url'] ?? '';
- $url = rtrim($baseUrl, '/') . '/' . ltrim($path, '/');
-
- if (($this->config['cacheBusting'] ?? false) === 'time') {
- $separator = str_contains($url, '?') ? '&' : '?';
- $url .= $separator . 'time=' . time();
- }
-
- return $url;
- },
+ 'cdn' => fn(string $path): string => CdnFilter::filter($path, $this->config),
];
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Extensions/CdnExtension.php around lines 27 to 59, remove the duplicated
URL-building and cache-busting logic in both getProviders() and getFilters() and
delegate to CdnFilter::filter instead: instantiate or reference the existing
src/Filters/CdnFilter and call its filter method passing the path as the value
and the extension config as the $parameters argument (so the filter can read url
and cacheBusting), return the result, and ensure the filter is
referenced/imported properly or fully-qualified; keep signatures (closure
accepting string $path) and preserve behavior for empty config.
| public static function filter(string $path, array $parameters = []): string | ||
| { | ||
| $baseUrl = $parameters['url'] ?? ''; | ||
| $url = rtrim($baseUrl, '/') . '/' . ltrim($path, '/'); | ||
|
|
||
| if (($parameters['cacheBusting'] ?? false) === 'time') { | ||
| $separator = str_contains($url, '?') ? '&' : '?'; | ||
| $url .= $separator . 'time=' . time(); | ||
| } | ||
|
|
||
| return $url; |
There was a problem hiding this comment.
Handle absolute URLs safely
Right now we always prepend the CDN base, so passing an absolute or protocol-relative path (e.g., https://libs.example/app.js or //cdn.thirdparty/foo.css) produces broken URLs like https://cdn.example.com/https://.... Templates often funnel both local and external assets through the same filter, so this is a real correctness bug. Please short-circuit when the incoming path is already absolute (scheme or //) and keep the macro/provider code in sync with the same guard.
- $baseUrl = $parameters['url'] ?? '';
- $url = rtrim($baseUrl, '/') . '/' . ltrim($path, '/');
+ $baseUrl = $parameters['url'] ?? '';
+
+ if (preg_match('#^(?:[a-z][a-z0-9+\-.]*:|//)#i', $path)) {
+ $url = $path;
+ } else {
+ $url = rtrim($baseUrl, '/') . '/' . ltrim($path, '/');
+ }
if (($parameters['cacheBusting'] ?? false) === 'time') {
$separator = str_contains($url, '?') ? '&' : '?';
$url .= $separator . 'time=' . time();
}
return $url;🤖 Prompt for AI Agents
In src/Filters/CdnFilter.php around lines 11 to 21, the filter currently always
prepends the CDN base which breaks absolute (scheme-prefixed) and
protocol-relative ("//") URLs; update the function to detect absolute URLs
(either starting with '//' or matching a URI scheme like 'https://') before
concatenating the base and, if absolute, use the incoming path as the URL (still
applying cacheBusting if requested). Place this guard before rtrim/ltrim
concatenation and mirror the same check in the corresponding macro/provider code
so both paths behave identically.
Implements CDN (Content Delivery Network) support for managing asset URLs
across different environments with automatic URL prefixing and optional
cache-busting.
Features:
- Macro syntax: {cdn 'assets/style.css'}
- Filter syntax: {='assets/style.css'|cdn}
- Standalone filter: CdnFilter::filter()
- Environment-aware URL generation
- Optional cache-busting with ?time=timestamp
- Full DI integration with CdnExtension
Configuration:
cdn:
url: https://cdn.example.com
cacheBusting: time # or false
Implementation:
- Created CdnNode for macro compilation
- Created CdnExtension (Latte) for tag/filter registration
- Created CdnExtension (DI) for Nette integration
- Created CdnFilter for standalone usage
- Comprehensive tests with 12 passing tests
- Compact documentation following existing style
Requirements:
- PHP 8.2+ (updated from 8.1)
- Updated CI to test PHP 8.2 and 8.3 only
Closes #3
f3l1x
force-pushed
the
claude/implement-latte-issue-3-011CUxDfhnoPpaJt6o672wev
branch
from
3102b1f to
db07a8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Filters/CdnFilter.php (1)
11-21: Handle absolute URLs to prevent breaking external assets.The current implementation always prepends the CDN base URL, which breaks absolute URLs. If a template passes
https://external.com/lib.jsor//cdn.thirdparty.com/file.css, the result would be malformed likehttps://cdn.example.com/https://external.com/lib.js.This is a common real-world scenario where templates mix local assets (that should use CDN) with external assets (that should remain unchanged).
Apply this fix to detect and skip absolute URLs:
public static function filter(string $path, array $parameters = []): string { $baseUrl = $parameters['url'] ?? ''; - $url = rtrim($baseUrl, '/') . '/' . ltrim($path, '/'); + + // Skip prepending CDN URL for absolute URLs (scheme or protocol-relative) + if (preg_match('#^(?:[a-z][a-z0-9+\-.]*:|//)#i', $path)) { + $url = $path; + } else { + $url = rtrim($baseUrl, '/') . '/' . ltrim($path, '/'); + } if (($parameters['cacheBusting'] ?? false) === 'time') { $separator = str_contains($url, '?') ? '&' : '?'; $url .= $separator . 'time=' . time(); } return $url; }src/Extensions/CdnExtension.php (1)
33-68: Eliminate code duplication by delegating to CdnFilter.The
getProvidersandgetFiltersmethods contain identical URL-building logic (lines 36-45 duplicate lines 56-65), violating DRY. This creates maintenance burden and inconsistency risk. SinceCdnFilter::filteralready implements this logic with the same$parametersstructure, both methods should delegate to it.Additionally, both methods currently don't handle absolute URLs, but delegating to
CdnFilterwill automatically inherit that fix once applied there.Apply this refactoring:
namespace Contributte\Latte\Extensions; +use Contributte\Latte\Filters\CdnFilter; use Contributte\Latte\Extensions\Node\CdnNode; use Latte\Extension; public function getProviders(): array { return [ - 'cdnBuilder' => function (string $path): string { - $baseUrl = $this->config['url'] ?? ''; - $url = rtrim($baseUrl, '/') . '/' . ltrim($path, '/'); - - if (($this->config['cacheBusting'] ?? false) === 'time') { - $separator = str_contains($url, '?') ? '&' : '?'; - $url .= $separator . 'time=' . time(); - } - - return $url; - }, + 'cdnBuilder' => fn(string $path): string => CdnFilter::filter($path, $this->config), ]; } public function getFilters(): array { return [ - 'cdn' => function (string $path): string { - $baseUrl = $this->config['url'] ?? ''; - $url = rtrim($baseUrl, '/') . '/' . ltrim($path, '/'); - - if (($this->config['cacheBusting'] ?? false) === 'time') { - $separator = str_contains($url, '?') ? '&' : '?'; - $url .= $separator . 'time=' . time(); - } - - return $url; - }, + 'cdn' => fn(string $path): string => CdnFilter::filter($path, $this->config), ]; }
🧹 Nitpick comments (1)
.docs/README.md (1)
56-94: Fix documentation formatting issues.The markdownlint tool has identified several minor formatting issues:
- Hard tabs at lines 64, 71, 72 should be replaced with spaces
- Fenced code blocks at lines 86-88 and 91-93 should specify the language (e.g.,
textorhtml)Apply these fixes:
```neon extensions: - cdn: Contributte\Latte\DI\CdnExtension + cdn: Contributte\Latte\DI\CdnExtensionConfiguration
cdn: - url: https://cdn.example.com - cacheBusting: time # or false + url: https://cdn.example.com + cacheBusting: time # or falseUsage
{* Macro syntax *} <link rel="stylesheet" href="{cdn 'assets/style.css'}"> {* Filter syntax *} <script src="{='assets/dist/app.js'|cdn}"></script>Development (empty url):
-+text
/assets/style.css?time=123456789**Production (with CDN url):** -``` +```text https://cdn.example.com/assets/style.css?time=123456789</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3102b1f91bd695f5a7aec65947be6a1b00adade8 and e74c4a3c471086ab7fd6888ce3d9c10d133c741c. </details> <details> <summary>📒 Files selected for processing (10)</summary> * `.docs/README.md` (2 hunks) * `.github/workflows/tests.yml` (1 hunks) * `composer.json` (1 hunks) * `src/DI/CdnExtension.php` (1 hunks) * `src/Extensions/CdnExtension.php` (1 hunks) * `src/Extensions/Node/CdnNode.php` (1 hunks) * `src/Filters/CdnFilter.php` (1 hunks) * `tests/Cases/DI/CdnExtension.phpt` (1 hunks) * `tests/Cases/Extensions/CdnExtension.phpt` (1 hunks) * `tests/Cases/Filters/CdnFilter.phpt` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * tests/Cases/Extensions/CdnExtension.phpt * src/DI/CdnExtension.php </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>src/Extensions/CdnExtension.php (2)</summary><blockquote> <details> <summary>src/Extensions/Node/CdnNode.php (1)</summary> * `CdnNode` (11-39) </details> <details> <summary>src/DI/CdnExtension.php (1)</summary> * `CdnExtension` (18-46) </details> </blockquote></details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>.docs/README.md</summary> 64-64: Hard tabs Column: 1 (MD010, no-hard-tabs) --- 71-71: Hard tabs Column: 1 (MD010, no-hard-tabs) --- 72-72: Hard tabs Column: 1 (MD010, no-hard-tabs) --- 86-86: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 91-91: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>tests/Cases/DI/CdnExtension.phpt (1)</summary><blockquote> `1-106`: **LGTM! Comprehensive DI integration tests.** The test suite thoroughly covers the CDN extension's DI integration across four key scenarios: 1. Basic CDN URL prefixing 2. Cache busting with timestamps 3. Localhost mode (empty URL) 4. Minimal configuration fallback The test structure is clean and the assertions correctly validate both macro and filter syntax. </blockquote></details> <details> <summary>src/Extensions/Node/CdnNode.php (1)</summary><blockquote> `1-39`: **LGTM! Clean AST node implementation.** The `CdnNode` implementation correctly: - Parses the expression from the tag - Delegates URL construction to the `cdnBuilder` provider - Escapes output using `escapeHtmlAttr` for safety - Follows Latte's AST node patterns with proper iterator implementation </blockquote></details> <details> <summary>.github/workflows/tests.yml (1)</summary><blockquote> `14-18`: **Confirm nette-tester.yml implementation handles PHP 8.5 properly.** The original concern is valid: PHP 8.5 is not yet released as of Nov 9, 2025 (GA scheduled for Nov 20). Whether this causes CI failures depends on the shared workflow implementation. PHP 8.5 will reach general availability on Nov 20, 2025, but community setup actions like shivammathur/setup-php currently expose 8.5 as a nightly option. Verify that `contributte/.github/workflows/nette-tester.yml@master` uses a setup action that supports PHP 8.5 nightly, or expect CI failures until the official GA release. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| ], | ||
| "require": { | ||
| "php": ">=8.1", | ||
| "php": ">=8.2", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify PHP 8.2 requirement is necessary.
The minimum PHP version was raised from 8.1 to 8.2, which is a breaking change for users. Please confirm that PHP 8.2-specific features are actually being used in the new CDN extension code.
Run the following script to check for PHP 8.2-specific features:
🏁 Script executed:
#!/bin/bash
# Check for PHP 8.2-specific features in the new CDN code
echo "=== Checking for PHP 8.2+ features ==="
# Check for readonly classes (PHP 8.2+)
rg -nP '\breadonly\s+class\b' --type=php src/
# Check for DNF types (PHP 8.2+)
rg -nP '\([\w\|]+\)\s*&' --type=php src/
# Check for true/false/null as standalone types (PHP 8.2+)
rg -nP ':\s*(true|false|null)\s*[,\)]' --type=php src/
# Check for constants in traits (PHP 8.2+)
rg -nP 'trait\s+\w+.*const\s+' --type=php -U src/
echo "=== If no matches found, PHP 8.1 should be sufficient ==="Length of output: 96
---
🏁 Script executed:
```shell
#!/bin/bash
# Check composer.json for dependencies requiring PHP >= 8.2
echo "=== Checking require dependencies for PHP requirements ==="
rg -nP '"php":\s*"[^"]*8\.[2-9]' .
# Check composer.json for extensions requiring specific PHP versions
echo ""
echo "=== Checking for dependencies in require section ==="
sed -n '/"require":/,/}/p' composer.json | head -20
# Look for any recent changes or comments about PHP version
echo ""
echo "=== Searching for PHP version comments or documentation ==="
rg -nP 'php.*8\.[12]' -i
# Check if there are any PHP 8.2-specific attributes or other features
echo ""
echo "=== Checking for Attributes (PHP 8.0+) or other advanced features ==="
rg -nP '^\s*#\[' --type=php src/
# Check union type usage (not 8.2 specific but worth noting)
echo ""
echo "=== Checking for union types ==="
rg -nP ':\s*\w+\|\w+' --type=php src/
Length of output: 626
---
🌐 Web query:
```web
latte/latte 3.0 PHP version requirements
💡 Result:
Latte 3.0 requires PHP 8.0 through 8.2. [1]
Sources:
[1] Latte documentation — "Supported PHP versions" (Latte 3.0: PHP 8.0–8.2).
Revert PHP 8.2 requirement to 8.1.
The code contains no PHP 8.2-specific features, and the only dependency (latte/latte 3.0) explicitly supports PHP 8.1. The minimum version requirement should remain at >=8.1 unless there is a documented technical reason for the bump.
🤖 Prompt for AI Agents
In composer.json around line 19, the PHP version requirement was bumped to
">=8.2"; revert it back to ">=8.1" because the project and dependencies (e.g.,
latte/latte 3.0) support PHP 8.1; update the php constraint to ">=8.1" and run
composer validate to ensure the file remains valid.
| <?php declare(strict_types = 1); | ||
|
|
||
| use Contributte\Latte\Filters\CdnFilter; | ||
| use Contributte\Tester\Toolkit; | ||
| use Tester\Assert; | ||
|
|
||
| require_once __DIR__ . '/../../bootstrap.php'; | ||
|
|
||
| // Test CDN filter with base URL | ||
| Toolkit::test(function (): void { | ||
| $url = CdnFilter::filter('assets/style.css', [ | ||
| 'url' => 'https://cdn.example.com', | ||
| ]); | ||
| Assert::equal('https://cdn.example.com/assets/style.css', $url); | ||
|
|
||
| $url = CdnFilter::filter('js/app.js', [ | ||
| 'url' => 'https://cdn.example.com', | ||
| ]); | ||
| Assert::equal('https://cdn.example.com/js/app.js', $url); | ||
| }); | ||
|
|
||
| // Test CDN filter with base URL and cacheBusting | ||
| Toolkit::test(function (): void { | ||
| $url = CdnFilter::filter('assets/style.css', [ | ||
| 'url' => 'https://cdn.example.com', | ||
| 'cacheBusting' => 'time', | ||
| ]); | ||
| Assert::true(str_starts_with($url, 'https://cdn.example.com/assets/style.css?time=')); | ||
| Assert::match('~^https://cdn\.example\.com/assets/style\.css\?time=\d+$~', $url); | ||
| }); | ||
|
|
||
| // Test CDN filter without base URL (localhost) | ||
| Toolkit::test(function (): void { | ||
| $url = CdnFilter::filter('assets/style.css', [ | ||
| 'url' => '', | ||
| ]); | ||
| Assert::equal('/assets/style.css', $url); | ||
|
|
||
| $url = CdnFilter::filter('assets/style.css'); | ||
| Assert::equal('/assets/style.css', $url); | ||
| }); | ||
|
|
||
| // Test CDN filter without base URL with cacheBusting | ||
| Toolkit::test(function (): void { | ||
| $url = CdnFilter::filter('assets/style.css', [ | ||
| 'url' => '', | ||
| 'cacheBusting' => 'time', | ||
| ]); | ||
| Assert::true(str_starts_with($url, '/assets/style.css?time=')); | ||
| Assert::match('~^/assets/style\.css\?time=\d+$~', $url); | ||
| }); | ||
|
|
||
| // Test CDN filter with trailing slash handling | ||
| Toolkit::test(function (): void { | ||
| $url = CdnFilter::filter('assets/style.css', [ | ||
| 'url' => 'https://cdn.example.com/', | ||
| ]); | ||
| Assert::equal('https://cdn.example.com/assets/style.css', $url); | ||
|
|
||
| $url = CdnFilter::filter('/assets/style.css', [ | ||
| 'url' => 'https://cdn.example.com/', | ||
| ]); | ||
| Assert::equal('https://cdn.example.com/assets/style.css', $url); | ||
|
|
||
| $url = CdnFilter::filter('/assets/style.css', [ | ||
| 'url' => 'https://cdn.example.com', | ||
| ]); | ||
| Assert::equal('https://cdn.example.com/assets/style.css', $url); | ||
| }); | ||
|
|
||
| // Test CDN filter with query string already present | ||
| Toolkit::test(function (): void { | ||
| $url = CdnFilter::filter('assets/style.css?v=1.0', [ | ||
| 'url' => 'https://cdn.example.com', | ||
| 'cacheBusting' => 'time', | ||
| ]); | ||
| Assert::match('~^https://cdn\.example\.com/assets/style\.css\?v=1\.0&time=\d+$~', $url); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test cases for absolute URLs.
The test suite is comprehensive but should include test cases for absolute URLs (e.g., https://external.com/lib.js or //cdn.other.com/file.css) to verify they're handled correctly. Currently, the filter would incorrectly prepend the CDN base URL to absolute URLs, breaking them.
Add this test case after the existing tests:
// Test CDN filter with absolute URLs
Toolkit::test(function (): void {
// Should not prepend CDN URL to absolute HTTPS URLs
$url = CdnFilter::filter('https://external.com/lib.js', [
'url' => 'https://cdn.example.com',
]);
Assert::equal('https://external.com/lib.js', $url);
// Should not prepend CDN URL to protocol-relative URLs
$url = CdnFilter::filter('//cdn.other.com/file.css', [
'url' => 'https://cdn.example.com',
]);
Assert::equal('//cdn.other.com/file.css', $url);
// But should still apply cache busting if configured
$url = CdnFilter::filter('https://external.com/lib.js', [
'url' => 'https://cdn.example.com',
'cacheBusting' => 'time',
]);
Assert::match('~^https://external\.com/lib\.js\?time=\d+$~', $url);
});🤖 Prompt for AI Agents
In tests/Cases/Filters/CdnFilter.phpt around lines 1 to 78, add test cases for
absolute and protocol-relative URLs so the filter does not prepend the CDN base
URL for inputs like "https://external.com/lib.js" or "//cdn.other.com/file.css",
and verify cacheBusting still appends to absolute URLs when configured; insert a
new Toolkit::test block after the existing tests that asserts absolute URLs are
returned unchanged (for both https and protocol-relative) and that cacheBusting
adds the query param to absolute URLs.
Changes: - Added PHP 8.4 to test matrix in GitHub workflows - Fixed CodeSniffer issues in CdnNode and CdnExtension - Added blank line before return statement - Added @inheritdoc annotations for traversable return types - Added phpcs:disable for required reference return (Latte framework requirement) QA Results: - PHPStan: ✅ No errors - CodeSniffer: ✅ All new code clean (3 pre-existing issues in old Node files) - Tests: ✅ 12/12 passing
f3l1x
force-pushed
the
claude/implement-latte-issue-3-011CUxDfhnoPpaJt6o672wev
branch
from
e74c4a3 to
437c9bd
Compare
f3l1x
deleted the
claude/implement-latte-issue-3-011CUxDfhnoPpaJt6o672wev
branch
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests