feat!: laravel 13 support, PHP 8.3 requirement, Composer dependencies update#766
Open
JauheniS wants to merge 16 commits intowayofdev:masterfrom
Open
feat!: laravel 13 support, PHP 8.3 requirement, Composer dependencies update#766JauheniS wants to merge 16 commits intowayofdev:masterfrom
JauheniS wants to merge 16 commits intowayofdev:masterfrom
Conversation
…ed support of Laravel 10 (OOS by Laravel)
…nd installation.mdx
…es, muted frequent warnings about calling ENV and not used Trait in PHPStan
…nd installation.mdx
…mply with PHP 8.3
|
@JauheniS is attempting to deploy a commit to the wayofdev Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughBumps PHP and Docker images to 8.3/8.4, updates many Composer constraints, refactors DB rule/constraint queries to use the query builder, extends testing assertion helpers to accept iterables and model-to-table normalization, adds docblocks/#[Override] attributes, updates docs, CI, pre-commit hooks, and adds new DB-focused tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as "Test\n(caller)"
participant Trait as "InteractsWithDatabase\n(assertion trait)"
participant Resolver as "ORM/Schema\nResolver"
participant DB as "DatabaseProvider\n(select/count)"
participant Constraint as "Constraint\n(Count/Has/SoftDelete)"
Test->>Trait: call assertDatabaseX(tableOrIterable, ...)
Trait->>Trait: if iterable -> for each item -> recurse
Trait->>Resolver: normalizeTable(item) (Model/class -> tableName)
Resolver-->>Trait: tableName
Trait->>DB: select()->from(tableName)->where(...)->count()
DB-->>Trait: rowCount
Trait->>Constraint: evaluate(rowCount vs expectation)
Constraint-->>Test: assertion result (pass/fail)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Testing/Concerns/InteractsWithDatabase.php (1)
90-111:⚠️ Potential issue | 🟠 MajorIterable support is documented but not implemented in four assertions.
Line 90, Line 106, Line 130, and Line 152 advertise
iterable<Model>|string, butassertDatabaseCount,assertDatabaseEmpty,assertSoftDeleted, andassertNotSoftDeleteddo not branch onis_iterable($table). This creates a contract/runtime mismatch.Either add the same iterable loop used in
assertDatabaseHas/assertDatabaseMissingor narrow those docblocks back to supported input types.Also applies to: 129-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Testing/Concerns/InteractsWithDatabase.php` around lines 90 - 111, The docblocks for assertDatabaseCount, assertDatabaseEmpty, assertSoftDeleted, and assertNotSoftDeleted claim they accept iterable<Model>|string but the methods don’t handle iterables; update each method (assertDatabaseCount, assertDatabaseEmpty, assertSoftDeleted, assertNotSoftDeleted) to branch on is_iterable($table) and iterate over each Model/string entry, calling the existing single-item assertion logic for each element (same pattern as assertDatabaseHas/assertDatabaseMissing), or alternatively remove iterable from the docblocks if you prefer to restrict inputs—implement the iterable loop to preserve the documented contract.
🧹 Nitpick comments (5)
docs/pages/getting-started/installation.mdx (1)
11-21: Clarify that Laravel 10 rows are legacy compatibility entries.Line 11 says current prerequisites are Laravel 11.x–13.x, but Lines 18–20 still list Laravel 10 combinations. A short note like “legacy adapter compatibility” above those rows would remove ambiguity for new installs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/pages/getting-started/installation.mdx` around lines 11 - 21, Add a short clarifying note above the Compatibility Map table (or directly above the rows containing `^10.28`) stating that entries referencing Laravel 10 are legacy adapter compatibility entries; update the table label or insert a one-line annotation such as "Note: rows referencing `^10.28` indicate legacy adapter compatibility for Laravel 10" so readers understand the current prerequisites are Laravel 11.x–13.x while still preserving the older compatibility rows.docker-compose.yaml (1)
5-5: Consider parameterizing the PHP dev image tag.Line 5 hardcodes
8.4-cli-alpine-latest; exposing this via env (with 8.4 default) would make local compatibility checks against 8.3 easier without editing the compose file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yaml` at line 5, The docker-compose service hardcodes the PHP dev image tag ("wayofdev/php-dev:8.4-cli-alpine-latest") which makes switching versions cumbersome; change the service's image field to use an environment variable (e.g. PHP_DEV_IMAGE) with a default of "wayofdev/php-dev:8.4-cli-alpine-latest" so consumers can override at runtime, and add a note/example in the project .env or README showing how to set PHP_DEV_IMAGE to "wayofdev/php-dev:8.3-cli-alpine-latest" for local compatibility checks.tests/src/Testing/InteractsWithDatabaseTest.php (1)
20-23: Consider centralizing DB migration bootstrap for tests.This migration sequence is duplicated in
tests/src/Bridge/Laravel/Rules/ValidationRulesTest.php(Line 28-30). Moving it into a shared test helper/trait would reduce maintenance drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/Testing/InteractsWithDatabaseTest.php` around lines 20 - 23, The three artisan migration calls are duplicated; extract them into a shared test trait (e.g., DatabaseMigrationBootstrapTrait) with a single public method (e.g., bootstrapDatabaseMigrations() or use setUpDatabaseMigrations() called from setUp) that runs $this->artisan('cycle:migrate:init'), $this->artisan('cycle:orm:migrate', ['--force' => true]) and $this->artisan('cycle:migrate', ['--force' => true]); add that trait to InteractsWithDatabaseTest and Bridge/Laravel/Rules/ValidationRulesTest and replace the duplicated lines with a single call to the trait method (or rely on the trait to run in setUp), ensuring method and trait names match where referenced..pre-commit-config.yaml (1)
24-24: Avoid duplicating the php-cs-fixer invocation string.The same command flags are repeated in both branches, which is easy to desynchronize later. Consider extracting it into one variable.
♻️ Suggested simplification
- entry: bash -c "if command -v php &> /dev/null; then php vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.dist.php --verbose --diff; else docker compose run --rm --no-deps app php vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.dist.php --verbose --diff; fi" + entry: bash -c 'cmd="php vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.dist.php --verbose --diff"; if command -v php > /dev/null 2>&1; then $cmd; else docker compose run --rm --no-deps app $cmd; fi'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml at line 24, The pre-commit entry duplicates the php-cs-fixer invocation; refactor the bash -c string to define a single variable for the php-cs-fixer command (e.g., CMD="php vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.dist.php --verbose --diff") and then use that variable in both branches of the if/else so the flags are declared once; update the existing entry value (the entry string currently containing the full if/then/else) to set the variable and invoke it in both branches (use the same variable name) to avoid duplication.tests/src/Bridge/Laravel/Rules/ValidationRulesTest.php (1)
44-48: Minor duplication: extract the fail-callback builder.The same closure body appears four times; pulling it into a tiny private helper would keep this test easier to evolve.
Also applies to: 53-57, 73-77, 82-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/Bridge/Laravel/Rules/ValidationRulesTest.php` around lines 44 - 48, In ValidationRulesTest, extract the duplicated fail-callback closure used in calls to $rule->validate(...) that constructs a PotentiallyTranslatedString into a small private helper (e.g., a method on the test class like buildFailCallback or makeFailCallback) so each test calls $rule->validate('id', $post->id(), $this->makeFailCallback()) instead of duplicating the closure; ensure the helper returns a Closure matching the validate signature and constructs new PotentiallyTranslatedString($message, app('translator')) so you can replace the four identical inline closures (lines shown around the validate calls) with a single helper invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Line 45: The composer.json currently requires "php": "^8.3" but pins
config.platform.php to 8.4.19; update composer.json so config.platform.php
matches the declared minimum by either changing config.platform.php to "8.3.0"
or removing the entire "platform" override section; locate the "config" →
"platform" → "php" entry in composer.json and make the change so dependency
resolution and tests run against the documented minimum PHP version.
In `@phpstan.neon.dist`:
- Around line 13-15: The phpstan suppression for the message "#Called 'env'
outside of the config directory#" is a blanket rule that should be removed or
justified; either delete the ignoreErrors entry that contains that message from
phpstan.neon.dist, or retain it but add a clear comment immediately above the
ignoreErrors block documenting why it was added (include the original risk,
file/PR/ticket that introduced it, and any conditions under which it can be
safely removed) so future reviewers know why the suppression exists; reference
the message string "#Called 'env' outside of the config directory#" to locate
the exact rule to remove or annotate.
In `@README.md`:
- Around line 62-63: The adapter range in the row with "`11.x, 12.x, 13.x`"
currently reads "`≥5.0.0`" which overlaps the above row's "`5.0.0`"; update that
cell to a non-overlapping value by replacing "`≥5.0.0`" with either the exact
first post-5.0.0 release that added Laravel 13 support (e.g. "`5.0.1`" if that's
the correct release) or with a strictly-greater range such as "`>5.0.0`" so the
table no longer implies `5.0.0` belongs to both rows.
In `@src/Bridge/Laravel/Console/Commands/Database/ListCommand.php`:
- Around line 55-58: The error branch in ListCommand (when count($databases) ===
0) currently calls $this->error('No databases found.') but returns
self::SUCCESS; change the return to self::FAILURE so the handle/execute method
returns a non-zero exit code on error. Locate the conditional in the ListCommand
class (the method handling the command, e.g., handle/execute) and replace the
success constant with self::FAILURE to ensure CI/scripts detect the failure.
In `@src/Testing/Concerns/InteractsWithDatabase.php`:
- Around line 39-57: The documented support for Model|iterable<Model>|string
isn't being normalized to table names; update all six assertion
methods—assertDatabaseHas, assertDatabaseMissing, assertDatabaseCount,
assertDatabaseEmpty, assertSoftDeleted, and assertNotSoftDeleted—to call
$this->normalizeTable($table) before passing the value to the constraint classes
(e.g., HasInDatabase, CountInDatabase, etc.), and ensure iterable handling
mirrors assertDatabaseHas/assertDatabaseMissing by normalizing each item when
$table is iterable so constraints always receive a string table name.
In `@src/Testing/Constraints/NotSoftDeletedInDatabase.php`:
- Around line 52-55: The toString() method in NotSoftDeletedInDatabase returns
json_encode($this->data, JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE) which can
return false despite the string return type; update the flags to include
JSON_THROW_ON_ERROR (matching the approach used in HasInDatabase) so
json_encode() will throw on failure and guarantee a string return, i.e., adjust
NotSoftDeletedInDatabase::toString() to pass JSON_THROW_ON_ERROR along with the
existing flags.
In `@src/Testing/Constraints/SoftDeletedInDatabase.php`:
- Around line 52-55: The toString() method in SoftDeletedInDatabase currently
returns json_encode(...), which can return false and break the declared string
return type; change it to call json_encode($this->data, JSON_PRETTY_PRINT |
JSON_UNESCAPED_UNICODE) into a variable, check if the result is === false, and
if so return a safe string fallback (e.g. a compact fallback using
print_r($this->data, true) or a JSON string containing json_last_error_msg()),
otherwise return the encoded string—this ensures toString() always returns a
string and preserves error details for debugging.
---
Outside diff comments:
In `@src/Testing/Concerns/InteractsWithDatabase.php`:
- Around line 90-111: The docblocks for assertDatabaseCount,
assertDatabaseEmpty, assertSoftDeleted, and assertNotSoftDeleted claim they
accept iterable<Model>|string but the methods don’t handle iterables; update
each method (assertDatabaseCount, assertDatabaseEmpty, assertSoftDeleted,
assertNotSoftDeleted) to branch on is_iterable($table) and iterate over each
Model/string entry, calling the existing single-item assertion logic for each
element (same pattern as assertDatabaseHas/assertDatabaseMissing), or
alternatively remove iterable from the docblocks if you prefer to restrict
inputs—implement the iterable loop to preserve the documented contract.
---
Nitpick comments:
In @.pre-commit-config.yaml:
- Line 24: The pre-commit entry duplicates the php-cs-fixer invocation; refactor
the bash -c string to define a single variable for the php-cs-fixer command
(e.g., CMD="php vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.dist.php
--verbose --diff") and then use that variable in both branches of the if/else so
the flags are declared once; update the existing entry value (the entry string
currently containing the full if/then/else) to set the variable and invoke it in
both branches (use the same variable name) to avoid duplication.
In `@docker-compose.yaml`:
- Line 5: The docker-compose service hardcodes the PHP dev image tag
("wayofdev/php-dev:8.4-cli-alpine-latest") which makes switching versions
cumbersome; change the service's image field to use an environment variable
(e.g. PHP_DEV_IMAGE) with a default of "wayofdev/php-dev:8.4-cli-alpine-latest"
so consumers can override at runtime, and add a note/example in the project .env
or README showing how to set PHP_DEV_IMAGE to
"wayofdev/php-dev:8.3-cli-alpine-latest" for local compatibility checks.
In `@docs/pages/getting-started/installation.mdx`:
- Around line 11-21: Add a short clarifying note above the Compatibility Map
table (or directly above the rows containing `^10.28`) stating that entries
referencing Laravel 10 are legacy adapter compatibility entries; update the
table label or insert a one-line annotation such as "Note: rows referencing
`^10.28` indicate legacy adapter compatibility for Laravel 10" so readers
understand the current prerequisites are Laravel 11.x–13.x while still
preserving the older compatibility rows.
In `@tests/src/Bridge/Laravel/Rules/ValidationRulesTest.php`:
- Around line 44-48: In ValidationRulesTest, extract the duplicated
fail-callback closure used in calls to $rule->validate(...) that constructs a
PotentiallyTranslatedString into a small private helper (e.g., a method on the
test class like buildFailCallback or makeFailCallback) so each test calls
$rule->validate('id', $post->id(), $this->makeFailCallback()) instead of
duplicating the closure; ensure the helper returns a Closure matching the
validate signature and constructs new PotentiallyTranslatedString($message,
app('translator')) so you can replace the four identical inline closures (lines
shown around the validate calls) with a single helper invocation.
In `@tests/src/Testing/InteractsWithDatabaseTest.php`:
- Around line 20-23: The three artisan migration calls are duplicated; extract
them into a shared test trait (e.g., DatabaseMigrationBootstrapTrait) with a
single public method (e.g., bootstrapDatabaseMigrations() or use
setUpDatabaseMigrations() called from setUp) that runs
$this->artisan('cycle:migrate:init'), $this->artisan('cycle:orm:migrate',
['--force' => true]) and $this->artisan('cycle:migrate', ['--force' => true]);
add that trait to InteractsWithDatabaseTest and
Bridge/Laravel/Rules/ValidationRulesTest and replace the duplicated lines with a
single call to the trait method (or rely on the trait to run in setUp), ensuring
method and trait names match where referenced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22af0bdc-46ef-419c-8250-69fbe8feb207
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.github/SECURITY.md.github/workflows/testing-mssql.yml.pre-commit-config.yamlREADME.mdcomposer.jsondocker-compose.yamldocs/pages/getting-started/installation.mdxdocs/pages/services/testing.mdxphpstan.neon.distpsalm-baseline.xmlsrc/Bridge/Laravel/Console/Commands/Database/ListCommand.phpsrc/Bridge/Laravel/Console/Commands/Database/TableCommand.phpsrc/Bridge/Laravel/Rules/Exists.phpsrc/Bridge/Laravel/Rules/Unique.phpsrc/Contracts/GeneratorLoader.phpsrc/Testing/Concerns/InteractsWithDatabase.phpsrc/Testing/Constraints/HasInDatabase.phpsrc/Testing/Constraints/NotSoftDeletedInDatabase.phpsrc/Testing/Constraints/SoftDeletedInDatabase.phptests/src/Bridge/Laravel/Providers/CycleServiceProviderTest.phptests/src/Bridge/Laravel/Rules/ValidationRulesTest.phptests/src/TestCase.phptests/src/Testing/InteractsWithDatabaseTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Testing/Concerns/InteractsWithDatabase.php (1)
205-225: Well-structured normalization with graceful fallback.The logic correctly prioritizes:
- Eloquent Model →
getTable()- Cycle ORM entity/class → schema lookup
- Fallback → string cast
Consider adding a brief comment to the empty catch block to clarify intent for future maintainers:
💡 Optional: Document the silent catch
if ($schema->defines($role)) { return (string) $schema->define($role, SchemaInterface::TABLE); } - } catch (Throwable) { + } catch (Throwable) { + // ORM unavailable or role not defined; fall through to string cast } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Testing/Concerns/InteractsWithDatabase.php` around lines 205 - 225, The empty catch in normalizeTable is intentional to silently ignore errors during schema lookup; add a brief inline comment inside the catch block (in the normalizeTable method) such as "// ignore schema resolution errors — fall back to string table name" so future maintainers understand the silence, and ensure you do not change the existing behavior or remove the fallback return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Testing/Concerns/InteractsWithDatabase.php`:
- Around line 50-65: The assertion methods (assertDatabaseHas,
assertDatabaseMissing, assertDatabaseCount, assertDatabaseEmpty,
assertSoftDeleted, assertNotSoftDeleted) accept a $connection parameter but
never forward it to the constraint classes; update each method so the created
constraint instances (HasInDatabase, CountInDatabase, SoftDeletedInDatabase,
NotSoftDeletedInDatabase) receive the $connection argument when constructed
(using app(DatabaseProviderInterface::class) as the first arg remains the same),
and ensure recursive iterable handling continues to pass $connection through;
this will make multi-connection assertions actually use the provided connection.
---
Nitpick comments:
In `@src/Testing/Concerns/InteractsWithDatabase.php`:
- Around line 205-225: The empty catch in normalizeTable is intentional to
silently ignore errors during schema lookup; add a brief inline comment inside
the catch block (in the normalizeTable method) such as "// ignore schema
resolution errors — fall back to string table name" so future maintainers
understand the silence, and ensure you do not change the existing behavior or
remove the fallback return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75aa7d69-bfda-4071-887c-7ac94cb57c8e
📒 Files selected for processing (1)
src/Testing/Concerns/InteractsWithDatabase.php
…ensure it is compatible with iterable tables
JauheniS
changed the title
…ensuring consistency and clarity.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/src/Testing/InteractsWithDatabaseTest.php (1)
34-38: Consider extracting a small post-persistence helper to reduce repetition.The repeated
EntityManagerInterface+Postpersist/run blocks can be centralized for cleaner tests and easier future edits.♻️ Optional refactor
final class InteractsWithDatabaseTest extends TestCase { + private function persistPost(string $title = 'Title', string $description = 'Description'): Post + { + $em = app(EntityManagerInterface::class); + $post = new Post($title, $description); + $em->persist($post); + $em->run(); + + return $post; + } + #[Test] public function it_asserts_database_has_and_missing(): void { - $em = app(EntityManagerInterface::class); - $post = new Post('Title', 'Description'); - $em->persist($post); - $em->run(); + $post = $this->persistPost();Also applies to: 55-60, 80-84, 97-103, 113-117, 130-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/Testing/InteractsWithDatabaseTest.php` around lines 34 - 38, Extract a small helper that creates and persists a Post to remove the duplicated EntityManagerInterface + Post + persist + run pattern: add a private method (e.g., createAndPersistPost(string $title = 'Title', string $description = 'Description')) in the InteractsWithDatabaseTest class or a test trait that calls app(EntityManagerInterface::class), instantiates new Post($title, $description), calls $em->persist($post) and $em->run(), then returns the persisted Post; replace the repeated blocks (the snippets using EntityManagerInterface, new Post(...), persist, run at lines referenced) with calls to this helper to centralize behavior and make future changes easier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/src/Testing/InteractsWithDatabaseTest.php`:
- Around line 34-38: Extract a small helper that creates and persists a Post to
remove the duplicated EntityManagerInterface + Post + persist + run pattern: add
a private method (e.g., createAndPersistPost(string $title = 'Title', string
$description = 'Description')) in the InteractsWithDatabaseTest class or a test
trait that calls app(EntityManagerInterface::class), instantiates new
Post($title, $description), calls $em->persist($post) and $em->run(), then
returns the persisted Post; replace the repeated blocks (the snippets using
EntityManagerInterface, new Post(...), persist, run at lines referenced) with
calls to this helper to centralize behavior and make future changes easier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fc8d9b8-9e20-419d-91ee-105bc29fb31b
📒 Files selected for processing (23)
src/Bridge/Laravel/Console/Commands/Database/ListCommand.phpsrc/Bridge/Laravel/Console/Commands/Database/TableCommand.phpsrc/Bridge/Laravel/Facades/Cycle.phpsrc/Bridge/Laravel/Providers/CycleServiceProvider.phpsrc/Bridge/Laravel/Rules/Exists.phpsrc/Bridge/Laravel/Rules/Unique.phpsrc/Contracts/CacheManager.phpsrc/Contracts/GeneratorLoader.phpsrc/Schema/Cache/Manager.phpsrc/Schema/Compiler.phpsrc/Schema/Config/SchemaConfig.phpsrc/Schema/Generators/GeneratorQueue.phpsrc/Support/Arr.phpsrc/Testing/Concerns/InteractsWithDatabase.phpsrc/Testing/Constraints/CountInDatabase.phpsrc/Testing/Constraints/HasInDatabase.phpsrc/Testing/Constraints/NotSoftDeletedInDatabase.phpsrc/Testing/Constraints/SoftDeletedInDatabase.phpsrc/Testing/RefreshDatabase.phptests/src/Bridge/Laravel/Providers/CycleServiceProviderTest.phptests/src/Bridge/Laravel/Rules/ValidationRulesTest.phptests/src/TestCase.phptests/src/Testing/InteractsWithDatabaseTest.php
✅ Files skipped from review due to trivial changes (11)
- src/Testing/RefreshDatabase.php
- src/Bridge/Laravel/Facades/Cycle.php
- src/Schema/Generators/GeneratorQueue.php
- src/Support/Arr.php
- src/Schema/Compiler.php
- src/Bridge/Laravel/Console/Commands/Database/TableCommand.php
- src/Schema/Config/SchemaConfig.php
- src/Contracts/CacheManager.php
- src/Bridge/Laravel/Providers/CycleServiceProvider.php
- src/Schema/Cache/Manager.php
- src/Testing/Constraints/CountInDatabase.php
🚧 Files skipped from review as they are similar to previous changes (11)
- src/Bridge/Laravel/Rules/Exists.php
- tests/src/Bridge/Laravel/Providers/CycleServiceProviderTest.php
- src/Contracts/GeneratorLoader.php
- tests/src/Bridge/Laravel/Rules/ValidationRulesTest.php
- tests/src/TestCase.php
- src/Testing/Constraints/HasInDatabase.php
- src/Bridge/Laravel/Console/Commands/Database/ListCommand.php
- src/Bridge/Laravel/Rules/Unique.php
- src/Testing/Concerns/InteractsWithDatabase.php
- src/Testing/Constraints/SoftDeletedInDatabase.php
- src/Testing/Constraints/NotSoftDeletedInDatabase.php
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Testing/Constraints/NotSoftDeletedInDatabase.php (1)
61-64:⚠️ Potential issue | 🟡 MinorAdd
JSON_THROW_ON_ERRORintoString()to keep thestringcontract safe.Line 63 uses
json_encode(...)withoutJSON_THROW_ON_ERROR, so encode failures can still break the declaredstringreturn guarantee.💡 Proposed fix
public function toString(): string { - return json_encode($this->data, JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE); + return json_encode( + $this->data, + JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE | JSON_THROW_ON_ERROR + ); }#!/bin/bash # Verify all constraint toString/json_encode usages and check for JSON_THROW_ON_ERROR coverage. rg -n --type=php 'function\s+toString\s*\(|json_encode\(|JSON_THROW_ON_ERROR' src/Testing/Constraints -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Testing/Constraints/NotSoftDeletedInDatabase.php` around lines 61 - 64, Update the NotSoftDeletedInDatabase::toString method to call json_encode with JSON_THROW_ON_ERROR combined with the existing JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE flags, and wrap the call in a try/catch for \JsonException so the method still always returns a string (e.g., return a safe fallback string or a simple error message when encoding fails); adjust the return path to guarantee the declared string contract while keeping the original flags (JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE | JSON_THROW_ON_ERROR).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Testing/Constraints/NotSoftDeletedInDatabase.php`:
- Around line 61-64: Update the NotSoftDeletedInDatabase::toString method to
call json_encode with JSON_THROW_ON_ERROR combined with the existing
JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE flags, and wrap the call in a
try/catch for \JsonException so the method still always returns a string (e.g.,
return a safe fallback string or a simple error message when encoding fails);
adjust the return path to guarantee the declared string contract while keeping
the original flags (JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE |
JSON_THROW_ON_ERROR).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58a6a853-8f77-4673-81be-4a7c47795776
📒 Files selected for processing (23)
src/Bridge/Laravel/Console/Commands/Database/ListCommand.phpsrc/Bridge/Laravel/Console/Commands/Database/TableCommand.phpsrc/Bridge/Laravel/Facades/Cycle.phpsrc/Bridge/Laravel/Providers/CycleServiceProvider.phpsrc/Bridge/Laravel/Rules/Exists.phpsrc/Bridge/Laravel/Rules/Unique.phpsrc/Contracts/CacheManager.phpsrc/Contracts/GeneratorLoader.phpsrc/Schema/Cache/Manager.phpsrc/Schema/Compiler.phpsrc/Schema/Config/SchemaConfig.phpsrc/Schema/Generators/GeneratorQueue.phpsrc/Support/Arr.phpsrc/Testing/Concerns/InteractsWithDatabase.phpsrc/Testing/Constraints/CountInDatabase.phpsrc/Testing/Constraints/HasInDatabase.phpsrc/Testing/Constraints/NotSoftDeletedInDatabase.phpsrc/Testing/Constraints/SoftDeletedInDatabase.phpsrc/Testing/RefreshDatabase.phptests/src/Bridge/Laravel/Providers/CycleServiceProviderTest.phptests/src/Bridge/Laravel/Rules/ValidationRulesTest.phptests/src/TestCase.phptests/src/Testing/InteractsWithDatabaseTest.php
✅ Files skipped from review due to trivial changes (12)
- src/Bridge/Laravel/Facades/Cycle.php
- src/Testing/RefreshDatabase.php
- src/Schema/Generators/GeneratorQueue.php
- src/Schema/Compiler.php
- src/Schema/Config/SchemaConfig.php
- src/Bridge/Laravel/Console/Commands/Database/TableCommand.php
- src/Schema/Cache/Manager.php
- src/Bridge/Laravel/Providers/CycleServiceProvider.php
- src/Contracts/GeneratorLoader.php
- src/Contracts/CacheManager.php
- src/Testing/Constraints/CountInDatabase.php
- src/Support/Arr.php
🚧 Files skipped from review as they are similar to previous changes (10)
- src/Bridge/Laravel/Console/Commands/Database/ListCommand.php
- src/Bridge/Laravel/Rules/Exists.php
- src/Bridge/Laravel/Rules/Unique.php
- src/Testing/Constraints/HasInDatabase.php
- tests/src/Bridge/Laravel/Providers/CycleServiceProviderTest.php
- tests/src/Bridge/Laravel/Rules/ValidationRulesTest.php
- src/Testing/Constraints/SoftDeletedInDatabase.php
- src/Testing/Concerns/InteractsWithDatabase.php
- tests/src/TestCase.php
- tests/src/Testing/InteractsWithDatabaseTest.php
lotyp
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔍 Changes Made
composer.jsonandcomposer.lockto keep the project aligned with the current supported stackdocker-compose.yamlto enable interactive terminal support, also to use mssql18 in command for the sqlserver🤔 Rationale
🎟️ Related Tickets & Documents
📋 Checklist
make testpasses locally with my changesmake lintpasses locally with my changes🧪 Added or updated tests?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores