Refractor for better maintability #20

MayMeow · 2025-11-10T13:36:10Z

This pull request introduces significant improvements to file upload and download handling, error checking, and test coverage. It refactors the storage manager logic for better reliability, enhances error handling for file operations, and updates the CI workflow to include PHPUnit tests. Additionally, it makes several code quality improvements and bug fixes across the codebase.

File Storage and Upload/Download Improvements:

Refactored DownloadComponent and UploadComponent to cache the storage manager instance, add stricter error handling for misconfiguration, and validate the storage manager class and interface implementation. Now throws HttpException for misconfiguration or missing classes. (src/Controller/Component/DownloadComponent.php [1] [2]; src/Controller/Component/UploadComponent.php [3] [4]
Improved BunnyStorageManager to add detailed error checking for cURL operations, validate stream URIs, and provide clearer error messages when file uploads fail. Also refactored URL and hostname composition for better reliability. (src/Storage/BunnyStorageManager.php [1] [2]
Normalized storage paths in LocalStorageManager to avoid path errors and ensure consistent file handling. (src/Storage/LocalStorageManager.php [1] [2]
Improved StoredFile to cache file content and MIME type, and throw a custom exception if file content cannot be loaded. (src/File/StoredFile.php src/File/StoredFile.phpR6-R58)

API and Interface Adjustments:

Updated StorageManagerInterface and related implementations for stricter type safety and improved method signatures. (src/Storage/StorageManagerInterface.php [1] src/Storage/StorageManager.php [2]
Updated UploadedFileDecorator to use the PSR-7 UploadedFileInterface and improved option handling with default values. (src/File/UploadedFileDecorator.php [1] [2]

Testing and CI Enhancements:

Added PHPUnit tests for DownloadComponent, including tests for correct file retrieval and error cases when misconfigured. (tests/TestCase/Controller/Component/DownloadComponentTest.php tests/TestCase/Controller/Component/DownloadComponentTest.phpL4-R81)
Updated the CI workflow to use the latest composer-run-action and added a PHPUnit test job. Also added a test script to composer.json. (.github/workflows/ci.yml [1] [2]; composer.json [3]
Removed unused CakePHP fixture listeners from phpunit.xml.dist for cleaner test configuration. (phpunit.xml.dist phpunit.xml.distL20-L28)

Bug Fixes and Utilities:

Fixed PathUtils::fileNameSanitize to handle empty extensions correctly. (src/File/PathUtils.php src/File/PathUtils.phpR17-R20)

These changes collectively improve the reliability, maintainability, and testability of the file upload/download subsystem.

Refactored DownloadComponent and UploadComponent to validate and cache storage manager instances, with improved error handling. Enhanced file name sanitization and file content loading in PathUtils and StoredFile, including better MIME type detection and exception handling. Updated UploadedFileDecorator for broader PSR-7 compatibility and improved option access. Refined BunnyStorageManager and LocalStorageManager for safer path handling, error reporting, and more robust upload logic. Adjusted StorageManager and StorageManagerInterface for more flexible configuration access and clearer type hints.

Bump MayMeowHQ/composer-run-action to v8.4 in CI and add a PHPUnit test job. Update composer.json to use vendor/bin/phpunit for the test script. Remove the fixture listener from phpunit.xml.dist. Adjust PathUtilsTest assertions to match updated file name sanitization behavior.

Copilot

Pull Request Overview

This pull request enhances the file upload plugin with comprehensive test coverage, improved error handling, and better code maintainability. The changes introduce unit tests for core components, improve input validation and error handling across storage managers and components, and add CI automation for running tests.

Added unit tests for UploadedFileDecorator, StoredFile, PathUtils, and component classes
Enhanced error handling in BunnyStorageManager with proper resource cleanup and validation
Improved input validation in UploadComponent and DownloadComponent with proper error messages

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
tests/TestCase/File/UploadedFileDecoratorTest.php	New test suite for UploadedFileDecorator validating option retrieval and filename fallback behavior
tests/TestCase/File/StoredFileTest.php	New tests for StoredFile verifying content caching and error handling for missing files
tests/TestCase/File/PathUtilsTest.php	New tests for PathUtils filename sanitization with various edge cases
tests/TestCase/Controller/Component/UploadComponentTest.php	Enhanced test coverage with typed properties and comprehensive error scenario tests
tests/TestCase/Controller/Component/TestStorageManager.php	New test helper classes for mocking storage managers in component tests
tests/TestCase/Controller/Component/DownloadComponentTest.php	Added test coverage for DownloadComponent with error handling tests
src/Storage/StorageManagerInterface.php	Updated documentation for the put() method parameter
src/Storage/StorageManager.php	Enhanced getConfig() method to support nullable keys and mixed return types
src/Storage/LocalStorageManager.php	Improved path normalization to handle empty paths and directory separators consistently
src/Storage/BunnyStorageManager.php	Enhanced error handling with proper resource cleanup, validation checks, and improved URL composition
src/File/UploadedFileDecorator.php	Changed to use UploadedFileInterface for better flexibility and improved get() method with default values
src/File/StoredFile.php	Added content caching, improved error handling with custom exception, and refactored MIME type detection
src/File/PathUtils.php	Added handling for filenames without extensions
src/Controller/Component/UploadComponent.php	Added input validation, proper error messages, and storage manager caching
src/Controller/Component/DownloadComponent.php	Added comprehensive error handling and storage manager validation
phpunit.xml.dist	Removed deprecated PHPUnit listener configuration
composer.json	Added test script for running PHPUnit tests
.github/workflows/ci.yml	Updated GitHub Actions workflow version and added automated test execution

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-10T13:44:11Z

src/Storage/LocalStorageManager.php

+        $storagePath = (string)$this->getConfig('storagePath');
+        $normalizedPath = rtrim($storagePath, DIRECTORY_SEPARATOR . '/');
+
+        if ($normalizedPath !== '') {
+            $normalizedPath .= DIRECTORY_SEPARATOR;
+        }


The path normalization logic (lines 23-28 and 50-55) is duplicated in both the put() and pull() methods. Consider extracting this into a private method like normalizeStoragePath(string $path): string to improve maintainability and reduce code duplication.

Copilot · 2025-11-10T13:44:11Z

.github/workflows/ci.yml

      - uses: actions/checkout@v2
      - name: Composer run action PHPC_CS
-        uses: MayMeowHQ/composer-run-action@v3
+        uses: MayMeowHQ/composer-run-action@v8.4


The version v8.4 for this GitHub Action is unusual. GitHub Actions typically use either major version tags (e.g., v3, v8) or full semantic versions (e.g., v3.2.1). Please verify that v8.4 is a valid and existing tag in the MayMeowHQ/composer-run-action repository.

Copilot · 2025-11-10T13:44:11Z

.github/workflows/ci.yml

      - uses: actions/checkout@v2
      - name: Composer run action PHPStan
-        uses: MayMeowHQ/composer-run-action@v3
+        uses: MayMeowHQ/composer-run-action@v8.4


The version v8.4 for this GitHub Action is unusual. GitHub Actions typically use either major version tags (e.g., v3, v8) or full semantic versions (e.g., v3.2.1). Please verify that v8.4 is a valid and existing tag in the MayMeowHQ/composer-run-action repository.

Copilot · 2025-11-10T13:44:11Z

.github/workflows/ci.yml

+    steps:
+      - uses: actions/checkout@v2
+      - name: Composer run action PHPUnit
+        uses: MayMeowHQ/[email protected]


The version v8.4 for this GitHub Action is unusual. GitHub Actions typically use either major version tags (e.g., v3, v8) or full semantic versions (e.g., v3.2.1). Please verify that v8.4 is a valid and existing tag in the MayMeowHQ/composer-run-action repository.

Copilot · 2025-11-10T13:44:12Z

tests/TestCase/Controller/Component/UploadComponentTest.php

+        $size = ftell($stream);
+        rewind($stream);
+
+        $uploadedFile = new UploadedFile($stream, $size ?: null, UPLOAD_ERR_OK, $fileName, $mimeType);


[nitpick] The expression $size ?: null on line 141 can be simplified to $size !== false ? $size : null or handle the false case explicitly for better clarity, since ftell() returns int|false.

Suggested change

$uploadedFile = new UploadedFile($stream, $size ?: null, UPLOAD_ERR_OK, $fileName, $mimeType);

$uploadedFile = new UploadedFile($stream, $size !== false ? $size : null, UPLOAD_ERR_OK, $fileName, $mimeType);

Copilot · 2025-11-10T13:44:12Z

tests/TestCase/Controller/Component/UploadComponentTest.php

+    public function testGetFileReturnsDecorator(): void
+    {
+        TestStorageManager::reset();
+        $controller = $this->createControllerWithUpload('example.txt', 'plain/text');


The MIME type 'plain/text' is incorrect. It should be 'text/plain' following the standard format of type/subtype.

Suggested change

$controller = $this->createControllerWithUpload('example.txt', 'plain/text');

$controller = $this->createControllerWithUpload('example.txt', 'text/plain');

Copilot · 2025-11-10T13:44:12Z

src/File/UploadedFileDecorator.php

+            return $fileName;
        }

        return $this->originalData->getClientFilename();


The getClientFilename() method from PSR-7's UploadedFileInterface returns string|null, but the getFileName() method has a return type of string. This could cause a type error if getClientFilename() returns null. Consider handling the null case explicitly, e.g., return $this->originalData->getClientFilename() ?? '';

Suggested change

return $this->originalData->getClientFilename();

return $this->originalData->getClientFilename() ?? '';

Copilot · 2025-11-10T13:44:13Z

tests/TestCase/File/UploadedFileDecoratorTest.php

+        $size = ftell($stream);
+        rewind($stream);
+
+        return new UploadedFile($stream, $size ?: null, UPLOAD_ERR_OK, $fileName, 'text/plain');


[nitpick] The expression $size ?: null can be simplified to $size !== false ? $size : null or just handle the false case explicitly for better clarity, since ftell() returns int|false.

Suggested change

return new UploadedFile($stream, $size ?: null, UPLOAD_ERR_OK, $fileName, 'text/plain');

return new UploadedFile($stream, $size !== false ? $size : null, UPLOAD_ERR_OK, $fileName, 'text/plain');

Copilot · 2025-11-10T13:44:13Z

src/Controller/Component/UploadComponent.php

    protected function _getStorageManager(): StorageManagerInterface
    {
+        if ($this->storageManager instanceof StorageManagerInterface) {
+            return $this->storageManager;
+        }
+
        $sm = $this->getConfig('managerClass');

-        return new $sm($this->getConfig());
+        if (!is_string($sm) || $sm === '') {
+            throw new HttpException('Storage manager class is not configured.');
+        }
+
+        if (!class_exists($sm)) {
+            throw new HttpException(sprintf('Storage manager class "%s" does not exist.', $sm));
+        }
+
+        $storageManager = new $sm($this->getConfig());
+
+        if (!$storageManager instanceof StorageManagerInterface) {
+            throw new HttpException(sprintf('Storage manager must implement %s.', StorageManagerInterface::class));
+        }
+
+        $this->storageManager = $storageManager;
+
+        return $this->storageManager;
    }


The _getStorageManager() method (lines 57-82) is duplicated in both UploadComponent and DownloadComponent. Consider extracting this logic into a shared trait or base class to reduce code duplication and improve maintainability.

MayMeow added 3 commits November 10, 2025 14:18

add tests

f2371a3

MayMeow requested a review from Copilot November 10, 2025 13:36

Copilot AI reviewed Nov 10, 2025

View reviewed changes

Remove redundant comment about allowed storage types in UploadComponent

329fcc4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Refractor for better maintability #20

Refractor for better maintability #20

Uh oh!

MayMeow commented Nov 10, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	$uploadedFile = new UploadedFile($stream, $size ?: null, UPLOAD_ERR_OK, $fileName, $mimeType);
	$uploadedFile = new UploadedFile($stream, $size !== false ? $size : null, UPLOAD_ERR_OK, $fileName, $mimeType);

	$controller = $this->createControllerWithUpload('example.txt', 'plain/text');
	$controller = $this->createControllerWithUpload('example.txt', 'text/plain');

	return $this->originalData->getClientFilename();
	return $this->originalData->getClientFilename() ?? '';

	return new UploadedFile($stream, $size ?: null, UPLOAD_ERR_OK, $fileName, 'text/plain');
	return new UploadedFile($stream, $size !== false ? $size : null, UPLOAD_ERR_OK, $fileName, 'text/plain');

Uh oh!

Refractor for better maintability #20

Are you sure you want to change the base?

Refractor for better maintability #20

Uh oh!

Conversation

MayMeow commented Nov 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants