Loader Cache Build Failure Fixes - #39
Open
rleeson wants to merge 1 commit into
Open
Conversation
Build command (the one path that could still resurrect #30): - generate_cache() now explicitly verifies a non-empty cache file exists as Spatie will provide false positive cache builds on un-writeable locations. - Explicitly clear existing cache files for in place rebuilds to avoid stale files. - Tests cover both paths: an unwritable target fails with nothing written, and a regenerate that cannot replace an existing cache fails the build. Test isolation (correcting 90a3956): - Retesting showed @runTestsInSeparateProcesses was inherited, fixed annotations and added the trait to test_disable_constant_forces_live_discovery to avoid confusion - Dropped the ReflectionProperty/Method::setAccessible() calls, which have had no effect since PHP 8.1 and are deprecated in 8.5. Nits from review: - Updated all i18n text domains to tenup-plugin, matching the rest of the plugin - Trimmed the ModuleInitialization cache/live fallback documentation to concise statements - Revise Upgrade Guide documentation to list the five current cache badges/messages - Debugging: note that a tenup_framework_cache_load_failed listener has to be registered before the failing loader runs, so full coverage needs an mu-plugin. phpcs and phpstan (level 10) clean; phpunit green on PHP 8.3 (65 tests / 181 assertions). On local PHP 8.5 the three process-isolated tests error on a patchwork deprecation flood, unrelated to these changes. Refs #30 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of the Change
Stacked on #35 (branches from
fix/issue-30-autoloader-cache-refresh); this PR targets that branch, notdevelop. It closes the last path by which a stale cache could still reach a server, and resolves review nits on #35.Build command
This covers unlikely edge cases, where a cache is rebuilt in place versus an existing build and there are permissions or space issues.
generate_cache()now verifies a non-empty cache file exists after writing and throws otherwise. Spatie's file driver discards the return values ofmkdir()/file_put_contents(), so an un-writable target or a full disk produced only PHP warnings: the command printedCached N class(es)and exited 0 having written nothing. A pipeline that does not clean would then deploy the previous build's cache — issue #30 again, now with a build artifact's authority.Existing cache files are cleared before the write, so a failed rebuild in a writable location leaves nothing behind. Where the cache directory itself is un-writable the old file cannot be removed at all; there the guarantee is the non-zero exit, which stops the pipeline before it can ship the stale cache.
Test isolation
The trait-level
@runTestsInSeparateProcessesis inherited and in effect — PHPUnit merges annotations from used traits into the class's own. A comment added in #36 claimed otherwise; corrected, so the annotation is not removed as dead weight.test_disable_constant_forces_live_discoverynow carries its own@runInSeparateProcess, as itsdefine()is process-wide and cannot be undone.setAccessible()calls dropped: no effect since PHP 8.1, deprecated in 8.5.Nits
tenup-plugin, matching the rest of the package.get_classes()cache/live fallback comments to concise statements.tenup_framework_cache_load_failedlistener must be registered before the failing loader runs, so full coverage needs an mu-plugin.Refs #30 — #35 carries the fix for the issue; this PR hardens the build path it introduced.
How to test the Change
composer lint,composer static(PHPStan level 10) andcomposer test— all green (65 tests / 181 assertions).Cached N class(es), exit 0, no file written. After:Failed to write the class cache to "…", exit 1.class-loader-cache/made un-writable, rebuild: exit 1 withCould not remove the existing class cache at "…", rather than a green build over a stale file.wp-admin, confirm the badges onadmin.php?page=tenup-framework-loadersmatch the five listed in the Upgrade Guide.Local runs on PHP 8.5: three process-isolated tests error on an
antecedent/patchworkdeprecation flood, unrelated to this change. Green on 8.3.Changelog Entry
Credits
Props @rleeson, @darylldoyle
Checklist: