Fix: reset temporaryFiles array in finally block #540
Conversation
Use a finally block in removeTemporaryFiles() to ensure the temporaryFiles array is always cleared after removal, even if an exception occurs. This prevents attempting to delete the same files multiple times when the method is called from both the shutdown function and destructor.
There was a problem hiding this comment.
Pull request overview
This PR improves the robustness of temporary file cleanup by using a finally block to ensure the temporaryFiles array is always cleared, even if an exception occurs during file deletion. This prevents potential issues where the same files could be attempted to be deleted multiple times when removeTemporaryFiles() is called from both the shutdown function and destructor.
Changes:
- Wrapped the file deletion loop in a try-finally block to guarantee array cleanup
- Added test assertions to verify the
temporaryFilesarray is emptied after callingremoveTemporaryFiles()
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Knp/Snappy/AbstractGenerator.php | Added try-finally block in removeTemporaryFiles() to ensure temporaryFiles array is always cleared |
| tests/Knp/Snappy/AbstractGeneratorTest.php | Added assertions to verify array is cleared after removal in both cleanup test methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $remove = new ReflectionMethod($generator, 'removeTemporaryFiles'); | ||
| $remove->invoke($generator); | ||
|
|
||
| $this->assertCount(0, $files->getValue($generator)); |
There was a problem hiding this comment.
Consider adding a test case that verifies the temporaryFiles array is cleared even when unlink throws an exception. This would fully validate the purpose of the finally block introduced in this PR. The test could mock unlink to throw an exception and then verify that temporaryFiles is still emptied despite the exception.
2 participants
Summary
removeTemporaryFiles()to ensure$this->temporaryFilesis always cleared after removalremoveTemporaryFiles()Test plan
temporaryFilesarray is emptied after removal