Feature laravel13#48
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the package for Laravel 13 / Nova 5.8 compatibility and improves CloudinaryImage to support custom disk names (instead of assuming the disk is always cloudinary).
Changes:
- Bump runtime/dev dependencies (Laravel 13, PHP 8.3–8.5, Nova ^5.8, Testbench 11, Pest 4, Larastan 3.9) and update CI workflows accordingly.
- Update
CloudinaryImageto validate the configured disk based ongetStorageDisk()and use that disk for thumbnail/preview URLs. - Add tests for custom disk selection and misconfigured disks; update docs/changelog for the new release.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/CloudinaryImage.php |
Uses the field’s configured disk for URL generation and validates disk configuration. |
tests/ImageFieldTest.php |
Adds coverage for custom disk usage and missing disk configuration. |
tests/TestCase.php |
Removes Eloquent factory name guessing (factories no longer autoloaded). |
composer.json |
Bumps supported PHP/Laravel/Nova versions; adjusts scripts; changes minimum stability. |
README.md |
Updates compatibility matrix and install guidance for v13. |
CHANGELOG.md |
Adds 13.0.0 release notes. |
.github/workflows/run-tests.yml |
Updates matrix to PHP 8.3–8.5 + Laravel 13 and runs Pest without coverage. |
.github/workflows/phpstan.yml |
Runs PHPStan on PHP 8.5 and configures Nova auth before install. |
.github/workflows/fix-php-code-style-issues.yml |
Switches to running Pint after installing deps (incl. Nova auth). |
.github/workflows/composer-audit.yml |
Updates PHP version and adds Nova auth before dependency resolution/audit. |
.github/ISSUE_TEMPLATE/bug_report.yml |
Updates default PHP/Laravel version placeholders to 8.5 / 13.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $configuredDisk = config('filesystems.disks.'.$this->getStorageDisk()); | ||
| if (! $configuredDisk) { | ||
| throw new Exception('Cloudinary disk is not configured.'); |
There was a problem hiding this comment.
$configuredDisk only checks that a disk config array exists/non-empty, but it doesn’t validate that the selected disk is actually a Cloudinary disk. If a consumer passes a non-Cloudinary disk (e.g. local/s3), the thumbnail/preview callbacks will call Storage::disk(...)->url() with an array, which will throw a TypeError at runtime. Consider validating $configuredDisk['driver'] === 'cloudinary' (and/or that the disk’s adapter supports transformation options) and throwing a clear exception early.
| $configuredDisk = config('filesystems.disks.'.$this->getStorageDisk()); | |
| if (! $configuredDisk) { | |
| throw new Exception('Cloudinary disk is not configured.'); | |
| $storageDisk = $this->getStorageDisk(); | |
| $configuredDisk = config('filesystems.disks.'.$storageDisk); | |
| if (! is_array($configuredDisk) || ($configuredDisk['driver'] ?? null) !== 'cloudinary') { | |
| throw new Exception(sprintf( | |
| 'CloudinaryImage requires a configured Cloudinary disk. Disk [%s] is missing or is not using the [cloudinary] driver.', | |
| $storageDisk | |
| )); |
| if (! config('filesystems.disks.cloudinary')) { | ||
| $configuredDisk = config('filesystems.disks.'.$this->getStorageDisk()); | ||
| if (! $configuredDisk) { | ||
| throw new Exception('Cloudinary disk is not configured.'); |
There was a problem hiding this comment.
The exception message is hardcoded to “Cloudinary disk is not configured.” even when a custom disk name is passed (e.g. custom_cloudinary/other_disk). This makes debugging harder; consider including the disk name (and potentially the expected driver) in the message so users immediately see which disk is misconfigured.
| throw new Exception('Cloudinary disk is not configured.'); | |
| throw new Exception("Cloudinary disk [{$this->getStorageDisk()}] is not configured. Expected a filesystem disk configured for the [cloudinary] driver."); |
| } | ||
| }, | ||
| "minimum-stability": "stable", | ||
| "minimum-stability": "dev", |
There was a problem hiding this comment.
Setting "minimum-stability": "dev" at the package level is risky for consumers: it can allow dev versions of transitive dependencies to be selected unexpectedly, even with prefer-stable: true. If the goal is only to support Laravel 13/Nova 5.8, prefer keeping minimum-stability as stable and use explicit constraints (or per-package stability flags) for any dependency that truly requires dev stability.
| "minimum-stability": "dev", |
| config([ | ||
| 'filesystems.disks.custom_cloudinary' => [ | ||
| 'driver' => 'cloudinary', | ||
| 'cloud_name' => env('CLOUDINARY_CLOUD_NAME'), | ||
| 'api_key' => env('CLOUDINARY_API_KEY'), | ||
| 'api_secret' => env('CLOUDINARY_API_SECRET'), | ||
| 'url' => [ | ||
| 'secure' => (bool) env('CLOUDINARY_SECURE_URL', true), | ||
| ], | ||
| ], | ||
| ]); |
There was a problem hiding this comment.
The custom-disk test config pulls credentials from env(...). Tests here only assert the chosen disk name, so using environment values makes the suite less deterministic (and can fail in CI when env vars aren’t set). Prefer hardcoded dummy values (e.g. 'x') for cloud_name/api_key/api_secret (and secure) in this test setup.
| @@ -48,7 +48,7 @@ jobs: | |||
| run: cp phpunit.xml.dist phpunit.xml | |||
|
|
|||
| - name: Execute tests | |||
| run: vendor/bin/pest | |||
| run: vendor/bin/pest --no-coverage | |||
There was a problem hiding this comment.
There are two consecutive steps with the same name “Execute tests” (one copies phpunit.xml, the next runs Pest). This makes CI logs ambiguous. Consider renaming the first step (e.g. “Prepare phpunit.xml”) or combining the commands into a single step.
| - name: Apply Nova license | ||
| run: composer config http-basic.nova.laravel.com ${{ secrets.NOVA_USERNAME }} ${{ secrets.NOVA_LICENSE_KEY }} | ||
|
|
There was a problem hiding this comment.
composer config http-basic.nova.laravel.com ... will fail on PRs from forks because repository secrets aren’t available on pull_request workflows. Since this step was added here, consider guarding it (and/or the whole job) with a condition that secrets are present / the PR isn’t from a fork, otherwise skip with a clear message.
| - name: Apply Nova license | |
| run: composer config http-basic.nova.laravel.com ${{ secrets.NOVA_USERNAME }} ${{ secrets.NOVA_LICENSE_KEY }} | |
| - name: Apply Nova license | |
| if: ${{ secrets.NOVA_USERNAME != '' && secrets.NOVA_LICENSE_KEY != '' && (github.event_name != 'pull_request' || !github.event.pull_request.head.repo.fork) }} | |
| run: composer config http-basic.nova.laravel.com ${{ secrets.NOVA_USERNAME }} ${{ secrets.NOVA_LICENSE_KEY }} | |
| - name: Skip Nova license on forked PRs or when secrets are unavailable | |
| if: ${{ !(secrets.NOVA_USERNAME != '' && secrets.NOVA_LICENSE_KEY != '' && (github.event_name != 'pull_request' || !github.event.pull_request.head.repo.fork)) }} | |
| run: echo "Skipping Nova license configuration because Nova secrets are unavailable for this workflow run (for example, on pull requests from forks)." |
| - name: Apply Nova license | ||
| run: composer config http-basic.nova.laravel.com ${{ secrets.NOVA_USERNAME }} ${{ secrets.NOVA_LICENSE_KEY }} | ||
|
|
||
| - name: Install composer dependencies | ||
| uses: ramsey/composer-install@4.0.0 | ||
|
|
There was a problem hiding this comment.
This workflow now applies the Nova license using secrets.*. On push events from forks those secrets won’t exist, so the job will fail and may create noise for external contributors. Consider adding an if: guard (e.g. only run on the main repo / when secrets are set) or skipping the Nova step when credentials aren’t available.
| - name: Apply Nova license | ||
| run: composer config http-basic.nova.laravel.com ${{ secrets.NOVA_USERNAME }} ${{ secrets.NOVA_LICENSE_KEY }} | ||
|
|
||
| - name: Resolve dependencies and audit | ||
| run: | | ||
| composer update --no-interaction --no-ansi |
There was a problem hiding this comment.
This workflow now configures Nova HTTP basic auth using repository secrets. On pull_request runs from forks, secrets aren’t provided, so composer update/composer audit will fail. Consider guarding the job/step so it only runs when secrets are available (or only on push/scheduled runs in the main repo).
| |---------|-----|---------|------|----------------------| | ||
| | v13.0 | ^8.3 - ^8.5 | 13.x | ^5.8 | ^13.0 | | ||
| | v12.0 | ^8.2 - ^8.4 | 12.x | ^5.2.6 | ^12.0 | | ||
| | v11.0 | ^8.2 - ^8.3 | 11.x | ^4.3 | v11.x | |
There was a problem hiding this comment.
In the compatibility table, the “Flysystem Cloudinary” version for v11.0 is listed as v11.x, which isn’t a Composer constraint format and is inconsistent with the other rows (e.g. ^13.0, ^12.0). Consider changing it to a valid constraint (e.g. ^11.0 or 11.*) or clarifying that this column is not a Composer constraint.
| | v11.0 | ^8.2 - ^8.3 | 11.x | ^4.3 | v11.x | | |
| | v11.0 | ^8.2 - ^8.3 | 11.x | ^4.3 | ^11.0 | |
No description provided.