Feature Laravel 13#89
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the package for Laravel 13 compatibility while aligning adapter behavior and tests with Flysystem v3 expectations (exceptions, return types, and directory listing semantics).
Changes:
- Bumps runtime/dev requirements and CI tooling for Laravel 13 / newer PHP, Pest 4, Testbench 11, Larastan 3.9.
- Adjusts
FlysystemCloudinaryAdapterto better match Flysystem v3 behavior (typed attributes forlistContents, exception-based failures for read/copy/delete, prefix handling). - Splits out optional live Cloudinary integration tests into a dedicated
integrationPest group and documents usage.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/FlysystemCloudinaryAdapter.php |
Updates adapter methods for Flysystem v3 behavior (exceptions/streams/listing), prefixing changes, and temp-stream handling. |
tests/Pest.php |
Adds an integration group for tests under tests/Integration. |
tests/Integration/CloudinaryTest.php |
Updates integration tests for new exception/stream behaviors and adds skip logic for placeholder env. |
tests/Feature/AdapterTest.php |
Updates unit/feature tests for new return types and adds coverage for copy failure exception. |
README.md |
Updates compatibility matrix and documents disk env var, folder modes, and integration test behavior. |
phpunit.xml.dist |
Switches test env from FILESYSTEM_DRIVER to FILESYSTEM_DISK. |
phpstan.neon.dist |
Adds an ignore rule for Cloudinary upload argument type. |
composer.json |
Bumps PHP/Laravel/dev tool versions, updates scripts, adds branch alias, and changes minimum stability. |
CHANGELOG.md |
Documents v13.0.0 changes for Laravel 13 / Flysystem v3 behavior updates. |
.github/workflows/* |
Updates CI matrices and pins newer action versions; tweaks test command. |
.github/ISSUE_TEMPLATE/bug_report.yml |
Updates placeholders to PHP 8.5 / Laravel 13. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $sourceLogical = trim($path, '/'); | ||
| $destLogical = trim($newpath, '/'); | ||
| $prefixedSource = $this->ensureFolderIsPrefixed($sourceLogical); | ||
| $prefixedDest = $this->ensureFolderIsPrefixed($destLogical); | ||
|
|
||
| $metaRead = $this->readObject($path); | ||
| $metaRead = $this->readObject($prefixedSource); | ||
|
|
||
| if ($metaRead === false) { | ||
| $this->copied = false; | ||
|
|
||
| return; | ||
| throw UnableToCopyFile::fromLocationTo($sourceLogical, $destLogical); | ||
| } | ||
|
|
||
| $metaUpload = $this->upload($newpath, $metaRead['contents'], $config); | ||
| $metaUpload = $this->upload($prefixedDest, $metaRead['contents'], $config); | ||
|
|
There was a problem hiding this comment.
copy() passes a folder-prefixed destination into upload(). Since upload() already applies the configured folder via the Cloudinary folder option, this can result in a duplicated folder segment in the final public ID when flysystem-cloudinary.folder is set (e.g. folder/folder/file). Consider passing the logical destination path to upload() (and only using the prefixed path for API calls that require a full public ID), or adjusting upload() to avoid applying the folder twice.
| public function deleteDir($dirname): bool | ||
| { | ||
| $dirname = $this->ensureFolderIsPrefixed(trim($dirname, '/')); | ||
|
|
||
| $files = $this->listContents($dirname); | ||
| $files = $this->listContents($dirname, false); | ||
|
|
There was a problem hiding this comment.
deleteDir() prefixes $dirname and then passes it to listContents(), but listContents() prefixes the input again internally. With a configured flysystem-cloudinary.folder, this will list the wrong directory (double-prefixed) and leave files behind. Use separate logical/prefixed variables (as in move()/delete()), and call listContents() with the logical path.
| $tempFile = null; | ||
| if (is_string($body)) { | ||
| $tempFile = tmpfile(); | ||
|
|
||
| if ($tempFile === false) { | ||
| return false; | ||
| } | ||
| if (fwrite($tempFile, $body) === false) { | ||
| return false; | ||
| } | ||
| if (rewind($tempFile) === false) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
upload() passes a tmpfile() resource to UploadApi::upload(), and the PHPStan config now suppresses an argument-type error for this. If the Cloudinary SDK expects a string (file path), this can be a real runtime issue depending on SDK behavior. Consider passing the temp file path (e.g. via stream_get_meta_data($tempFile)['uri']) or using a real temp path (tempnam) instead of suppressing the type error.
| ignoreErrors: | ||
| - | ||
| identifier: argument.type | ||
| message: '#Parameter \#1 \$file of method Cloudinary\\Api\\Upload\\UploadApi::upload\(\) expects string#' | ||
| path: src/FlysystemCloudinaryAdapter.php |
There was a problem hiding this comment.
This new ignoreErrors rule suppresses a type error for UploadApi::upload() expecting a string, which may hide a real bug and weaken static analysis across future changes. Prefer to adjust FlysystemCloudinaryAdapter::upload() so it passes a string path (or otherwise matches the SDK signature), and then remove this ignore rule.
| ignoreErrors: | |
| - | |
| identifier: argument.type | |
| message: '#Parameter \#1 \$file of method Cloudinary\\Api\\Upload\\UploadApi::upload\(\) expects string#' | |
| path: src/FlysystemCloudinaryAdapter.php |
| beforeEach(function () { | ||
| if (in_array(env('CLOUDINARY_CLOUD_NAME'), [null, '', 'cloudinary_cloud_name'], true)) { | ||
| $this->markTestSkipped('Set real CLOUDINARY_CLOUD_NAME, CLOUDINARY_API_KEY, and CLOUDINARY_API_SECRET to run integration tests.'); | ||
| } |
There was a problem hiding this comment.
The integration-test skip guard only checks CLOUDINARY_CLOUD_NAME, but the tests also require CLOUDINARY_API_KEY and CLOUDINARY_API_SECRET. If CLOUDINARY_CLOUD_NAME is set (e.g. in CI) but key/secret are missing/placeholder, the suite will run and likely fail. Consider checking all three env vars for non-empty, non-placeholder values before running these integration tests (and keep the skip message consistent with the condition).
| } | ||
| }, | ||
| "minimum-stability": "stable", | ||
| "minimum-stability": "dev", |
There was a problem hiding this comment.
Setting the package minimum-stability to dev affects downstream consumers by allowing dev-only dependencies to be selected more readily, which can make installs less predictable. If the goal is only to support installing dev-main via the 13.x-dev branch alias, consider keeping minimum-stability at stable and documenting dev-main@dev usage instead.
| "minimum-stability": "dev", |
No description provided.