From 887266c7dc7fe460e164961df801cdead145563a Mon Sep 17 00:00:00 2001 From: Satish Ashilwar Date: Thu, 11 Dec 2025 00:00:38 +0530 Subject: [PATCH 1/2] Fix: Correct getPathInfo usage in captcha deletion cron The DeleteExpiredImages cron was not working since Magento 2.4.8 due to incorrect usage of getPathInfo() method. The method was called with a second parameter (PATHINFO_EXTENSION) which it doesn't support, causing the PNG file extension check to always fail. Changes: - Fixed getPathInfo() call to use array indexing instead of second parameter - Changed from getPathInfo($filePath, PATHINFO_EXTENSION) == 'png' - Changed to getPathInfo($filePath)['extension'] == 'png' - Added comprehensive unit test to verify correct getPathInfo usage Root cause: The getPathInfo() method only accepts one parameter and returns the full pathinfo array. Using PATHINFO_EXTENSION as second parameter was comparing array to string, which always resulted in false. Manual testing: 1. Enable captcha on frontend/admin login 2. Generate captcha images by failing login attempts 3. Wait 20+ minutes (default timeout) 4. Run bin/magento cron:run --group=default 5. Verify PNG files are deleted from media/captcha/ directories Before fix: Files never get deleted, directory grows indefinitely After fix: Expired PNG files are properly cleaned up Fixes: #40192 --- .../Captcha/Cron/DeleteExpiredImages.php | 2 +- .../Unit/Cron/DeleteExpiredImagesTest.php | 71 +++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/app/code/Magento/Captcha/Cron/DeleteExpiredImages.php b/app/code/Magento/Captcha/Cron/DeleteExpiredImages.php index afacad9b249e3..e7fd3c3689e92 100644 --- a/app/code/Magento/Captcha/Cron/DeleteExpiredImages.php +++ b/app/code/Magento/Captcha/Cron/DeleteExpiredImages.php @@ -97,7 +97,7 @@ protected function _deleteExpiredImagesForWebsite( $imageDirectory = $this->_mediaDirectory->getRelativePath($helper->getImgDir($website)); foreach ($this->_mediaDirectory->read($imageDirectory) as $filePath) { if ($this->_mediaDirectory->isFile($filePath) - && $this->_fileInfo->getPathInfo($filePath, PATHINFO_EXTENSION) == 'png' + && $this->_fileInfo->getPathInfo($filePath)['extension'] == 'png' && $this->_mediaDirectory->stat($filePath)['mtime'] < $expire ) { $this->_mediaDirectory->delete($filePath); diff --git a/app/code/Magento/Captcha/Test/Unit/Cron/DeleteExpiredImagesTest.php b/app/code/Magento/Captcha/Test/Unit/Cron/DeleteExpiredImagesTest.php index 97fdb1b6380fa..9203ba52a2094 100644 --- a/app/code/Magento/Captcha/Test/Unit/Cron/DeleteExpiredImagesTest.php +++ b/app/code/Magento/Captcha/Test/Unit/Cron/DeleteExpiredImagesTest.php @@ -150,6 +150,77 @@ public function testDeleteExpiredImages($website, $isFile, $filename, $mTime, $t $this->_deleteExpiredImages->execute(); } + /** + * Test that getPathInfo is called with correct parameters for PNG file detection + * + * @return void + */ + public function testGetPathInfoIsCalledCorrectlyForPngExtension() + { + $filename = 'test.png'; + $website = $this->getMockForWebsiteClass(); + + $this->_storeManager->expects($this->once()) + ->method('getWebsites') + ->willReturn([$website]); + + $this->_helper->expects($this->once()) + ->method('getConfig') + ->with('timeout', $website->getDefaultStore()) + ->willReturn(20); + + $this->_adminHelper->expects($this->once()) + ->method('getConfig') + ->with('timeout', null) + ->willReturn(20); + + $this->_helper->expects($this->once()) + ->method('getImgDir') + ->with($website) + ->willReturn('captcha/base'); + + $this->_adminHelper->expects($this->once()) + ->method('getImgDir') + ->with(null) + ->willReturn('captcha/admin'); + + $this->_directory->expects($this->exactly(2)) + ->method('getRelativePath') + ->willReturnOnConsecutiveCalls('captcha/base', 'captcha/admin'); + + $this->_directory->expects($this->exactly(2)) + ->method('read') + ->willReturnOnConsecutiveCalls([$filename], [$filename]); + + $this->_directory->expects($this->exactly(2)) + ->method('isFile') + ->with($filename) + ->willReturn(true); + + // This is the key test - verify getPathInfo is called with only the filename + // and returns the proper array structure + $this->_fileInfo->expects($this->exactly(2)) + ->method('getPathInfo') + ->with($filename) + ->willReturn([ + 'dirname' => 'captcha/base', + 'basename' => 'test.png', + 'extension' => 'png', + 'filename' => 'test' + ]); + + $this->_directory->expects($this->exactly(2)) + ->method('stat') + ->with($filename) + ->willReturn(['mtime' => time() - 1500]); // Expired + + $this->_directory->expects($this->exactly(2)) + ->method('delete') + ->with($filename); + + $this->_deleteExpiredImages->execute(); + } + protected function getMockForWebsiteClass() { $website = $this->createPartialMock(Website::class, ['__wakeup', 'getDefaultStore']); From d86b5c5584ccb199d530dc70dd1382f20f712560 Mon Sep 17 00:00:00 2001 From: Satish Ashilwar Date: Mon, 15 Dec 2025 14:58:55 +0530 Subject: [PATCH 2/2] Fix: Update test to properly mock getPathInfo array return - Add proper mocking of getPathInfo method in existing test - Remove complex test method that was causing mock conflicts - Ensure test works with array access pattern [extension] instead of PATHINFO_EXTENSION parameter --- .../Unit/Cron/DeleteExpiredImagesTest.php | 75 ++----------------- 1 file changed, 8 insertions(+), 67 deletions(-) diff --git a/app/code/Magento/Captcha/Test/Unit/Cron/DeleteExpiredImagesTest.php b/app/code/Magento/Captcha/Test/Unit/Cron/DeleteExpiredImagesTest.php index 9203ba52a2094..5704c436e6b49 100644 --- a/app/code/Magento/Captcha/Test/Unit/Cron/DeleteExpiredImagesTest.php +++ b/app/code/Magento/Captcha/Test/Unit/Cron/DeleteExpiredImagesTest.php @@ -147,77 +147,18 @@ public function testDeleteExpiredImages($website, $isFile, $filename, $mTime, $t $this->_directory->expects($this->exactly($timesToCall))->method('isFile')->willReturn($isFile); $this->_directory->expects($this->any())->method('stat')->willReturn(['mtime' => $mTime]); - $this->_deleteExpiredImages->execute(); - } - - /** - * Test that getPathInfo is called with correct parameters for PNG file detection - * - * @return void - */ - public function testGetPathInfoIsCalledCorrectlyForPngExtension() - { - $filename = 'test.png'; - $website = $this->getMockForWebsiteClass(); - - $this->_storeManager->expects($this->once()) - ->method('getWebsites') - ->willReturn([$website]); - - $this->_helper->expects($this->once()) - ->method('getConfig') - ->with('timeout', $website->getDefaultStore()) - ->willReturn(20); - - $this->_adminHelper->expects($this->once()) - ->method('getConfig') - ->with('timeout', null) - ->willReturn(20); - - $this->_helper->expects($this->once()) - ->method('getImgDir') - ->with($website) - ->willReturn('captcha/base'); - - $this->_adminHelper->expects($this->once()) - ->method('getImgDir') - ->with(null) - ->willReturn('captcha/admin'); - - $this->_directory->expects($this->exactly(2)) - ->method('getRelativePath') - ->willReturnOnConsecutiveCalls('captcha/base', 'captcha/admin'); - - $this->_directory->expects($this->exactly(2)) - ->method('read') - ->willReturnOnConsecutiveCalls([$filename], [$filename]); - - $this->_directory->expects($this->exactly(2)) - ->method('isFile') - ->with($filename) - ->willReturn(true); - - // This is the key test - verify getPathInfo is called with only the filename - // and returns the proper array structure - $this->_fileInfo->expects($this->exactly(2)) + // Mock getPathInfo to return proper array structure for our fix + $extension = pathinfo($filename, PATHINFO_EXTENSION); + $this->_fileInfo->expects($this->any()) ->method('getPathInfo') ->with($filename) ->willReturn([ - 'dirname' => 'captcha/base', - 'basename' => 'test.png', - 'extension' => 'png', - 'filename' => 'test' + 'dirname' => '.', + 'basename' => $filename, + 'extension' => $extension, + 'filename' => pathinfo($filename, PATHINFO_FILENAME) ]); - - $this->_directory->expects($this->exactly(2)) - ->method('stat') - ->with($filename) - ->willReturn(['mtime' => time() - 1500]); // Expired - - $this->_directory->expects($this->exactly(2)) - ->method('delete') - ->with($filename); - + $this->_deleteExpiredImages->execute(); }