fix: purge Divi clone CSS cache#1352
Conversation
superdav42
added
status:in-review
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Divi compatibility to site duplication by clearing the duplicated site's Divi ChangesDivi Cache Clearing on Site Duplication
Conditional Performance Steps in CI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
![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
🧹 Nitpick comments (1)
tests/WP_Ultimo/General_Compat_Test.php (1)
52-79: ⚡ Quick winCover the legacy Divi cache layout too.
The production code also removes
wp-content/et-cache/{blog_id}as a fallback, but this test only exerciseset-cache/{network_id}/{blog_id}. Add a legacy fixture/assertion here so the fallback path cannot regress silently.Suggested coverage expansion
$network_id = (int) get_current_network_id(); $cache_root = trailingslashit(WP_CONTENT_DIR) . 'et-cache'; $cache_dir = trailingslashit($cache_root) . $network_id . '/' . $blog_id; + $legacy_dir = trailingslashit($cache_root) . $blog_id; $other_dir = trailingslashit($cache_root) . $network_id . '/' . $other_blog_id; - $this->cache_dirs = [$cache_dir, $other_dir]; + $this->cache_dirs = [$cache_dir, $legacy_dir, $other_dir]; wp_mkdir_p($cache_dir . '/9'); + wp_mkdir_p($legacy_dir . '/9'); wp_mkdir_p($other_dir . '/9'); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents -- Test fixture setup. file_put_contents($cache_dir . '/9/et-core-unified-deferred-9.min.css', 'stale divi css'); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents -- Test fixture setup. + file_put_contents($legacy_dir . '/9/et-core-unified-deferred-9.min.css', 'legacy divi css'); + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents -- Test fixture setup. file_put_contents($other_dir . '/9/et-core-unified-deferred-9.min.css', 'other divi css'); General_Compat::get_instance()->clear_divi_static_css_cache(['site_id' => $blog_id]); $this->assertDirectoryDoesNotExist($cache_dir); + $this->assertDirectoryDoesNotExist($legacy_dir); $this->assertDirectoryExists($other_dir);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/WP_Ultimo/General_Compat_Test.php` around lines 52 - 79, Extend the test_clear_divi_static_css_cache_deletes_cloned_site_cache_only test to also create the legacy cache layout (wp-content/et-cache/{blog_id}) alongside the networked layout, write a fixture file (e.g. et-core-unified-deferred-9.min.css) into that legacy dir, then call General_Compat::get_instance()->clear_divi_static_css_cache(['site_id' => $blog_id]) and assert the legacy cache directory is removed while the other blog's legacy/networked dirs still exist; update the test's $this->cache_dirs setup and add corresponding file creation and assertions so the fallback removal path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@inc/compat/class-general-compat.php`:
- Around line 448-479: The purge currently uses $file->getRealPath() which
resolves symlinks and can cause deletions outside $real_cache_dir; change the
loop to use $file->getPathname(), skip any symlink entries (use
SplFileInfo::isLink()/isLink), normalize the pathname (wp_normalize_path +
untrailingslashit/trailingslashit) and then verify the normalized path has the
$real_cache_dir prefix before deleting; keep the existing behavior for
directories (rmdir) and files (wp_delete_file()) but only after the symlink
check and prefix re-check to prevent path escape.
In `@tests/WP_Ultimo/General_Compat_Test.php`:
- Around line 1-17: Add the missing file guard and update the test namespace to
mirror the source: insert defined('ABSPATH') || exit; at the top of the file
immediately after <?php, and change the namespace from WP_Ultimo\Tests to
WP_Ultimo\Compat\Tests so the test namespace mirrors the source class
WP_Ultimo\Compat\General_Compat; keep the existing use/imports and class name
General_Compat_Test unchanged.
---
Nitpick comments:
In `@tests/WP_Ultimo/General_Compat_Test.php`:
- Around line 52-79: Extend the
test_clear_divi_static_css_cache_deletes_cloned_site_cache_only test to also
create the legacy cache layout (wp-content/et-cache/{blog_id}) alongside the
networked layout, write a fixture file (e.g. et-core-unified-deferred-9.min.css)
into that legacy dir, then call
General_Compat::get_instance()->clear_divi_static_css_cache(['site_id' =>
$blog_id]) and assert the legacy cache directory is removed while the other
blog's legacy/networked dirs still exist; update the test's $this->cache_dirs
setup and add corresponding file creation and assertions so the fallback removal
path is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ce5ca9d-4d26-43fb-b539-3f709fcb4a5a
📒 Files selected for processing (2)
inc/compat/class-general-compat.phptests/WP_Ultimo/General_Compat_Test.php
| $real_cache_dir = realpath($cache_dir); | ||
| $real_cache_root = realpath($cache_root); | ||
|
|
||
| if (false === $real_cache_dir || false === $real_cache_root) { | ||
| return; | ||
| } | ||
|
|
||
| $real_cache_dir = untrailingslashit(wp_normalize_path($real_cache_dir)); | ||
| $real_cache_root = untrailingslashit(wp_normalize_path($real_cache_root)); | ||
|
|
||
| if ($real_cache_root === $real_cache_dir || 0 !== strpos(trailingslashit($real_cache_dir), trailingslashit($real_cache_root))) { | ||
| return; | ||
| } | ||
|
|
||
| $iterator = new \RecursiveIteratorIterator( | ||
| new \RecursiveDirectoryIterator($real_cache_dir, \FilesystemIterator::SKIP_DOTS), | ||
| \RecursiveIteratorIterator::CHILD_FIRST | ||
| ); | ||
|
|
||
| foreach ($iterator as $file) { | ||
| $path = $file->getRealPath(); | ||
|
|
||
| if (false === $path) { | ||
| continue; | ||
| } | ||
|
|
||
| if ($file->isDir()) { | ||
| // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_rmdir, WordPress.PHP.NoSilencedErrors.Discouraged -- Removes an empty generated cache directory after deleting its contents. | ||
| @rmdir($path); | ||
| } else { | ||
| wp_delete_file($path); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does PHP SplFileInfo::getRealPath()resolve symlink targets, and doesRecursiveDirectoryIteratorstill yield symlink entries whenFilesystemIterator::FOLLOW_SYMLINKS is not set?
💡 Result:
Yes, PHP's SplFileInfo::getRealPath resolves all symbolic links [1][2]. It expands symlinks, resolves relative references (like /./ or /../), and returns the canonicalized absolute path to the file [3][2]. If the file does not exist, the method returns false [1][2]. Regarding RecursiveDirectoryIterator, it does still yield symlink entries even when the FilesystemIterator::FOLLOW_SYMLINKS flag is not set [4][5]. The iterator will return the symlink itself as an entry in the iteration [6]. The FilesystemIterator::FOLLOW_SYMLINKS flag controls whether the iterator should follow these symlinks to traverse into the target directories, but it does not prevent the iterator from encountering and yielding the symlink files themselves [7][4].
Citations:
- 1: https://www.php.net/splfileinfo.getrealpath
- 2: https://www.php.net/manual/en/splfileinfo.getrealpath.php
- 3: https://www.php.net/function.realpath
- 4: https://www.php.net/manual/en/recursivedirectoryiterator.construct.php
- 5: https://www.php.net/manual/en/recursivedirectoryiterator.construct
- 6: https://stackoverflow.com/questions/78785111/how-do-i-walk-a-directory-recursively-while-following-links-in-php
- 7: https://www.php.net/class.recursivedirectoryiterator
Skip symlinks when purging cache contents to prevent path escape
SplFileInfo::getRealPath() resolves symlink targets, and RecursiveDirectoryIterator still yields symlink entries even without FOLLOW_SYMLINKS. A symlink inside $real_cache_dir can therefore make wp_delete_file() act on a path outside the cache tree. Use getPathname(), skip links, and re-check each entry against a $real_cache_dir prefix before deleting.
Suggested hardening
$real_cache_dir = untrailingslashit(wp_normalize_path($real_cache_dir));
$real_cache_root = untrailingslashit(wp_normalize_path($real_cache_root));
if ($real_cache_root === $real_cache_dir || 0 !== strpos(trailingslashit($real_cache_dir), trailingslashit($real_cache_root))) {
return;
}
+
+ $real_cache_dir_prefix = trailingslashit($real_cache_dir);
$iterator = new \RecursiveIteratorIterator(
new \RecursiveDirectoryIterator($real_cache_dir, \FilesystemIterator::SKIP_DOTS),
\RecursiveIteratorIterator::CHILD_FIRST
);
foreach ($iterator as $file) {
- $path = $file->getRealPath();
+ if ($file->isLink()) {
+ continue;
+ }
- if (false === $path) {
+ $path = untrailingslashit(wp_normalize_path($file->getPathname()));
+
+ if ('' === $path || 0 !== strpos(trailingslashit($path), $real_cache_dir_prefix)) {
continue;
}
if ($file->isDir()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@inc/compat/class-general-compat.php` around lines 448 - 479, The purge
currently uses $file->getRealPath() which resolves symlinks and can cause
deletions outside $real_cache_dir; change the loop to use $file->getPathname(),
skip any symlink entries (use SplFileInfo::isLink()/isLink), normalize the
pathname (wp_normalize_path + untrailingslashit/trailingslashit) and then verify
the normalized path has the $real_cache_dir prefix before deleting; keep the
existing behavior for directories (rmdir) and files (wp_delete_file()) but only
after the symlink check and prefix re-check to prevent path escape.
| <?php | ||
| /** | ||
| * Tests for the General_Compat class. | ||
| * | ||
| * @package WP_Ultimo | ||
| * @subpackage Tests | ||
| */ | ||
|
|
||
| namespace WP_Ultimo\Tests; | ||
|
|
||
| use WP_Ultimo\Compat\General_Compat; | ||
| use WP_UnitTestCase; | ||
|
|
||
| /** | ||
| * Test general compatibility fixes. | ||
| */ | ||
| class General_Compat_Test extends WP_UnitTestCase { |
There was a problem hiding this comment.
Match the repo's PHP test-file conventions.
This file is missing the standard defined('ABSPATH') || exit; guard, and namespace WP_Ultimo\Tests; does not mirror the source namespace for WP_Ultimo\Compat\General_Compat.
Suggested cleanup
<?php
+defined('ABSPATH') || exit;
+
/**
* Tests for the General_Compat class.
*
* `@package` WP_Ultimo
* `@subpackage` Tests
*/
-namespace WP_Ultimo\Tests;
+namespace WP_Ultimo\Compat;
-use WP_Ultimo\Compat\General_Compat;
use WP_UnitTestCase;As per coding guidelines, Every PHP file must start with defined('ABSPATH') || exit; and tests/WP_Ultimo/**/*_Test.php: Test namespace must mirror source structure.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <?php | |
| /** | |
| * Tests for the General_Compat class. | |
| * | |
| * @package WP_Ultimo | |
| * @subpackage Tests | |
| */ | |
| namespace WP_Ultimo\Tests; | |
| use WP_Ultimo\Compat\General_Compat; | |
| use WP_UnitTestCase; | |
| /** | |
| * Test general compatibility fixes. | |
| */ | |
| class General_Compat_Test extends WP_UnitTestCase { | |
| <?php | |
| defined('ABSPATH') || exit; | |
| /** | |
| * Tests for the General_Compat class. | |
| * | |
| * `@package` WP_Ultimo | |
| * `@subpackage` Tests | |
| */ | |
| namespace WP_Ultimo\Compat; | |
| use WP_UnitTestCase; | |
| /** | |
| * Test general compatibility fixes. | |
| */ | |
| class General_Compat_Test extends WP_UnitTestCase { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/WP_Ultimo/General_Compat_Test.php` around lines 1 - 17, Add the missing
file guard and update the test namespace to mirror the source: insert
defined('ABSPATH') || exit; at the top of the file immediately after <?php, and
change the namespace from WP_Ultimo\Tests to WP_Ultimo\Compat\Tests so the test
namespace mirrors the source class WP_Ultimo\Compat\General_Compat; keep the
existing use/imports and class name General_Compat_Test unchanged.
Source: Coding guidelines
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
superdav42
added
the
review-feedback-scanned
1 participant
Summary
wu_duplicate_site.wp-content/et-cache/{network_id}/{blog_id}/directory, with a legacyet-cache/{blog_id}fallback.Verification
php -l inc/compat/class-general-compat.phpphp -l tests/WP_Ultimo/General_Compat_Test.phpvendor/bin/phpcs inc/compat/class-general-compat.php tests/WP_Ultimo/General_Compat_Test.phpvendor/bin/phpstan analyse inc/compat/class-general-compat.php tests/WP_Ultimo/General_Compat_Test.phpvendor/bin/phpunit --filter General_Compat_TestNotes
The customer report showed a cloned Divi page loading all assets successfully but using incomplete generated Divi layout CSS. Purging the cloned site's et-cache directory forces Divi to regenerate the CSS with the copied builder data on the next frontend request.
Summary by CodeRabbit
Bug Fixes
Tests
Chores