feat: added storage interface #3956
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a storage abstraction (StorageInterface, StorageException), filesystem and S3 implementations, a StorageFactory with DI wiring, AWS SDK dependency, PHPUnit tests for interface/implementations/factory, and hardens System::createHashes and Attachment\File::get() file handling. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Factory as phpMyFAQ\Storage\StorageFactory
participant Config as Configuration
participant FS as FilesystemStorage
participant S3 as S3Storage
participant S3Client as AWS S3 Client
rect rgba(200,230,255,0.5)
App->>Factory: create()
Factory->>Config: read storage.type (default "filesystem")
alt type == "filesystem"
Factory->>FS: new FilesystemStorage(rootPath, publicBaseUrl)
Factory-->>App: return FilesystemStorage (StorageInterface)
else type == "s3"
Factory->>Config: read S3 settings (bucket, region, endpoint, creds)
Factory->>S3Client: instantiate/configure client
S3Client-->>Factory: client instance
Factory->>S3: new S3Storage(client, bucket, prefix, publicBaseUrl)
Factory-->>App: return S3Storage (StorageInterface)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Storage/FilesystemStorage.php`:
- Around line 136-144: In normalizePath, don't reject any path that merely
contains the substring '..' (which disallows valid filenames like
"file..backup.txt"); instead, after normalizing separators (the existing
str_replace('\\','/')), split the normalized path into segments (e.g.,
explode('/') or preg_split) and throw StorageException only if any segment ===
'..' or any segment === '' (if you want to also guard against empty segments),
then continue to return the path with DIRECTORY_SEPARATOR; update the check that
currently uses str_contains($normalizedPath, '..') to this segment-based
validation within normalizePath.
- Around line 109-117: The url() method can return backslashes on Windows
because normalizePath() converts slashes to DIRECTORY_SEPARATOR; update url() to
use a forward-slash-normalized path when building URLs (e.g., call a
forward-slash-safe variant of normalizePath() or replace backslashes with '/' on
the normalizedPath before concatenating with publicBaseUrl), and ensure
buildFullPath() similarly returns forward-slash paths for URLs; reference
normalizePath(), url(), buildFullPath(), and publicBaseUrl when making this
change.
In `@phpmyfaq/src/phpMyFAQ/Storage/S3Storage.php`:
- Around line 134-146: The current path-traversal guard in S3Storage::buildKey
(and the similar FilesystemStorage::normalizePath) uses
str_contains($normalizedPath, '..') which wrongly rejects legitimate filenames
containing ".."; replace it with a precise check that detects path-segment
traversal only (e.g., split the normalized path on '/' and ensure no segment is
exactly '..', or check for the sequence '../' or '/..' or a path that starts
with '..') and throw StorageException only when such a traversal segment is
found; update both buildKey and normalizePath to use this segment-based check so
filenames like "file..backup.txt" remain allowed while "../secret" is blocked.
- Around line 62-75: The is_array() guard in S3Storage::get and S3Storage::size
incorrectly fails for Aws\Result objects (which implement ArrayAccess); remove
the is_array() checks and only verify the expected keys on the result (e.g. in
get() check array_key_exists('Body', $result) or isset($result['Body']) and in
size() check array_key_exists('ContentLength', $result) or
isset($result['ContentLength'])), leaving the run(...) call, buildKey(...) usage
and StorageException throw intact so valid Aws\Result responses from
client->getObject() and client->headObject() are accepted.
In `@phpmyfaq/src/phpMyFAQ/Storage/StorageFactory.php`:
- Around line 67-74: The current code in StorageFactory that reads
storage.s3.key and storage.s3.secret using readStringConfig silently omits
credentials if only one value is provided; update the logic in StorageFactory
(where $key and $secret are read and $s3Config is built) to validate that either
both $key and $secret are non-null or both are null, and if exactly one is
provided throw a StorageException with a clear message (e.g., "Both
storage.s3.key and storage.s3.secret must be provided together"); leave the
existing credentials assignment to $s3Config unchanged when both are present.
In `@tests/phpMyFAQ/Storage/FilesystemStorageTest.php`:
- Around line 26-37: The test
FilesystemStorageTest::testPutGetExistsSizeDeleteAndUrl fails on Windows because
FilesystemStorage::url() uses normalizePath that yields backslashes; update
FilesystemStorage::url() to produce a web-friendly path by normalizing the
stored path to use forward slashes (replace backslashes with '/'), ensure proper
trimming/adding of slashes between the base URL and the normalized path, and
return the combined URL with forward slashes so the test expectation in
testPutGetExistsSizeDeleteAndUrl passes cross-platform.
🧹 Nitpick comments (4)
composer.json (1)
31-31: Consider makingaws/aws-sdk-phpa suggested (optional) dependency instead of required.The AWS SDK for PHP is a very large package (~100 MB+). Requiring it unconditionally means every phpMyFAQ installation pulls it in, even if they only use filesystem storage. The
StorageFactory::createS3Storage()already has aclass_exists(S3Client::class)guard, so the code is already structured to handle its absence.Move
aws/aws-sdk-phpfromrequiretosuggestand throwStorageExceptionincreateS3Storage()when the SDK is missing (which it already does).Additionally,
^3.0is an extremely broad constraint — it spans releases from 2015 to the present. Consider pinning to a more recent minimum (e.g.,^3.300) for predictability.phpmyfaq/src/phpMyFAQ/Storage/S3Storage.php (1)
26-32: Consider type-hinting$clientasS3Clientinstead ofobject.Using
objectloses static analysis benefits and allows injection of any object, which would fail at runtime on the first S3 call. If testability is the concern, you could use an interface or keepobjectbut document the expected contract.This is a minor observation — the current approach works given that
StorageFactorycreates the realS3Client.phpmyfaq/src/phpMyFAQ/Storage/StorageFactory.php (1)
81-84: The@var objectannotation suppresses the actualS3Clienttype.This annotation exists to satisfy
S3Storage'sobjecttype hint. IfS3Storage::$clientis updated to acceptS3Client(as suggested in the S3Storage review), this annotation can be removed and the types will flow naturally.phpmyfaq/src/phpMyFAQ/Storage/FilesystemStorage.php (1)
89-102: Minor TOCTOU indelete(): file could disappear betweenfile_existsandunlink.If another process deletes the file between the
file_existscheck (line 92) and theunlinkcall (line 96),unlinkwill fail and throwStorageException. For a more resilient approach, attemptunlinkdirectly and catch the warning using@unlinkor check the result.This is a low-probability edge case with a safe fallback (the exception), so it's not urgent.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Storage/StorageFactory.php`:
- Around line 91-103: resolveFilesystemRoot and the FilesystemStorage
constructor currently only trim/return a path, deferring failures until
put()/putStream(); validate the filesystem root early by checking the resolved
root (from resolveFilesystemRoot or inside StorageFactory.create) exists and is
writable (or attempt to create it with mkdir(..., recursive:true) and verify
success), and if validation fails throw a clear exception (or propagate a
Storage-related exception) so misconfiguration surfaces at initialization rather
than runtime; update FilesystemStorage::__construct or StorageFactory::create to
perform this validation on the root returned by resolveFilesystemRoot and
include the directory path in the error message.
In `@tests/phpMyFAQ/Storage/FilesystemStorageTest.php`:
- Around line 39-50: The test testPutStreamWritesContents currently assumes
fopen returns a resource and may leak the stream if an assertion fails; update
it to check fopen succeeded (e.g., assertIsResource or if ($stream === false)
$this->fail(...)) and wrap use of the stream in a try/finally so fclose($stream)
is always called. Locate the test method testPutStreamWritesContents and adjust
the logic around fopen('php://memory', 'rb+') / fwrite / rewind so you validate
the $stream resource before using it and ensure fclose runs in a finally block
after the storage assertions (referencing FilesystemStorage and the
putStream/get methods).
🧹 Nitpick comments (6)
tests/phpMyFAQ/Storage/FilesystemStorageTest.php (1)
123-149:removeDirectoryhelper is correct and defensive.Handles
scandirfailure, skips./.., and recurses safely. Consider using PHP'sRecursiveDirectoryIterator+RecursiveIteratorIteratorfor brevity, but this is fine as-is.phpmyfaq/src/phpMyFAQ/Storage/S3Storage.php (2)
24-32: Looseobjecttype hint for$client— consider a narrow interface.The
objecttype on$clientallows any object, including ones that don't implement the required S3 methods (putObject,getObject, etc.). Errors will only surface at runtime as "method not found" calls. A lightweight interface (or at least a union type withS3Client) would catch misconfigurations earlier.That said, I understand this simplifies testing with fakes, so this is optional.
125-132:run()catches allThrowable— consider preserving the exception code.The wrapper discards the original exception's code. While not critical, forwarding
$throwable->getCode()can be useful for debugging AWS-specific error codes.Optional improvement
- throw new StorageException($throwable->getMessage(), previous: $throwable); + throw new StorageException($throwable->getMessage(), (int) $throwable->getCode(), $throwable);tests/phpMyFAQ/Storage/S3StorageTest.php (2)
11-27: Good comprehensive integration test, but consider testingexists()afterdelete().The test calls
delete()at the end but doesn't assertexists()returnsfalseafterward. Adding one line would verify the full lifecycle.Suggested addition
$this->assertTrue($storage->delete('docs/readme.txt')); + $this->assertFalse($storage->exists('docs/readme.txt'));
100-103:getObjectreturns empty string for missing keys — doesn't simulate S3 errors.The real S3 client throws
NoSuchKeyExceptionwhen the key doesn't exist. The fake returns['Body' => ''], which meansget()on a missing object silently returns an empty string instead of throwing. Consider adding a test for the error path with a throwing fake, or adjusting the fake to throw for missing keys.phpmyfaq/src/phpMyFAQ/Storage/StorageFactory.php (1)
57-59: Pin the S3 API model version to avoid breaking changes during SDK updates.The
'version' => 'latest'configuration uses AWS's latest service API model, which can change when you update the AWS SDK for PHP, potentially introducing breaking changes in production. AWS explicitly recommends pinning the API version for production deployments. For S3, use'version' => '2006-03-01':Suggested change
$s3Config = [ 'version' => '2006-03-01', 'region' => $region, ];
# Conflicts: # composer.lock # phpmyfaq/src/phpMyFAQ/System.php # phpmyfaq/src/services.php
thorsten
force-pushed
the
feat/storage-abstraction
branch
from
81df663 to
c8e8399
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Attachment/File.php`:
- Around line 158-159: The get() method in class File is an empty stub that
declares a string return type and will cause a TypeError under strict_types;
replace the empty body with an explicit behavior (e.g., throw a descriptive
exception such as BadMethodCallException or return an empty string) so the
method always returns a string or fails explicitly, and update the method brace
placement to follow PER Coding Style 3.0 (opening brace on the next line and
method body non-empty) — locate the public function get(): string declaration in
File.php and implement one of these two fixes consistently.
🧹 Nitpick comments (4)
phpmyfaq/src/phpMyFAQ/Storage/StorageFactory.php (2)
85-86: Misleading@var objectannotation on a typedS3Clientinstantiation.The
S3Clientconstructor returns anS3Clientinstance, not a genericobject. This annotation was likely added to satisfy theS3Storageconstructor which acceptsobject, but it's misleading at the call site.
57-59: Consider pinning the S3 SDK API version instead of using'latest'.Using
'version' => 'latest'means the SDK will use whatever API version it ships with, which could change behavior oncomposer update. Pinning to a specific API version (e.g.,'2006-03-01') improves reproducibility.phpmyfaq/src/phpMyFAQ/Storage/S3Storage.php (1)
26-32: Consider using a narrower type or an interface for$clientinstead ofobject.Typing the S3 client as
objectsacrifices all static analysis and IDE support. An interface or the concreteS3Clienttype (with mocking viacreateMockin tests) would be safer. If the goal is testability with fakes, a thin adapter interface would preserve type safety.tests/phpMyFAQ/Storage/S3StorageTest.php (1)
29-41: Stream is not closed if an assertion fails beforefclose.Same pattern as the past review on
FilesystemStorageTest— wrap intry/finallyto ensure the stream resource is always cleaned up.Proposed fix
public function testPutStreamWritesObject(): void { $client = new FakeS3Client(); $storage = new S3Storage($client, 'pmf-bucket', 'tenant/attachments'); $stream = fopen('php://memory', 'rb+'); + $this->assertIsResource($stream); - fwrite($stream, 'stream-data'); - rewind($stream); - $this->assertTrue($storage->putStream('stream.txt', $stream)); - $this->assertSame('stream-data', (string) $client->objects['tenant/attachments/stream.txt']); - - fclose($stream); + try { + fwrite($stream, 'stream-data'); + rewind($stream); + $this->assertTrue($storage->putStream('stream.txt', $stream)); + $this->assertSame('stream-data', (string) $client->objects['tenant/attachments/stream.txt']); + } finally { + fclose($stream); + } }
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores