Skip to content

Conversation

@bradenkeith
Copy link
Member

@bradenkeith bradenkeith commented Nov 21, 2025

Summary by CodeRabbit

Release Notes

  • Chores
    • Enhanced version compatibility with the Spatie Laravel Data library to support a wider range of dependency versions without requiring hard dependencies
    • Standardized validation error message formatting across schema outputs for improved consistency

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 21, 2025

Walkthrough

This PR adds version-aware conditional resolution for Spatie's DataMorphClassResolver dependency. A new helper method in the service provider conditionally resolves the resolver based on runtime detection, while the resolver class now accepts a nullable object and uses reflection to determine if the dependency is required, enabling compatibility across different Spatie Laravel Data versions without forcing a hard dependency.

Changes

Cohort / File(s) Summary
Conditional dependency resolution
src/LaravelSchemaGeneratorServiceProvider.php
Added protected method resolveDataMorphClassResolver($app) that conditionally resolves DataMorphClassResolver, checking for version compatibility and returning null if not required. Updated call-site to use conditional resolution instead of direct container resolution.
Guarded resolver initialization
src/Resolvers/InheritingDataValidationRulesResolver.php
Changed constructor parameter from typed DataMorphClassResolver to nullable ?object. Added static method requiresDataMorphClassResolver() using reflection to detect if the resolver is needed. Implemented lazy parameter wiring with runtime enforcement via conditional checks.
Test fixture validation update
tests/Fixtures/Expected/unified-schemas.ts
Changed email field validation from custom error object style (z.string({ error: '...' })) to standard max constraint (z.string().max(255, '...')), maintaining the same 255-character limit and error message.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Reflection-based detection logic: Verify that requiresDataMorphClassResolver() correctly inspects BaseResolver signature and that reflection caching works as intended
  • Conditional parameter wiring: Confirm the lazy parameter building and parent constructor call properly handle both scenarios (with and without resolver)
  • Runtime exception handling: Review the condition that enforces providing a resolver when requiresDataMorphClassResolver() returns true
  • Test fixture semantics: Confirm that the validation style change (custom error → max constraint) is functionally equivalent and matches expected output

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'Prep work for PHP 8.5' is vague and does not clearly convey the actual changes made. The PR primarily modifies dependency resolution logic for Spatie Data morph classes and updates validation rules to be version-aware, which is unrelated to PHP 8.5 compatibility. Consider a more descriptive title that reflects the core changes, such as 'Make DataMorphClassResolver optional for Spatie Data version compatibility' or 'Add conditional resolution for DataMorphClassResolver'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch prep-for-8-5

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/LaravelSchemaGeneratorServiceProvider.php (1)

80-88: Version‑aware morph resolver wiring looks correct; consider clarifying the helper signature

The new resolveDataMorphClassResolver() helper plus InheritingDataValidationRulesResolver::requiresDataMorphClassResolver() gives clean, version‑aware wiring: you avoid a hard dependency when the morph resolver isn’t needed, and you fail fast when it is required but missing.

If you want a bit more clarity for static analysis, you could optionally type‑hint the container argument:

-use Illuminate\Support\ServiceProvider;
+use Illuminate\Support\ServiceProvider;
+use Illuminate\Contracts\Container\Container;
@@
-    protected function resolveDataMorphClassResolver($app): ?object
+    protected function resolveDataMorphClassResolver(Container $app): ?object

Behavior otherwise looks good as‑is.

Also applies to: 195-211

src/Resolvers/InheritingDataValidationRulesResolver.php (1)

6-7: Constructor refactor and runtime detection of morph resolver requirement look solid

The constructor’s shift to ?object $dataMorphClassResolver = null plus the $parameters array and requiresDataMorphClassResolver() guard cleanly supports both Spatie versions with and without the legacy morph resolver, and the explicit RuntimeException is a good fail‑fast path when it’s required but not provided. The static reflection‑based detector with a cached result is also reasonable here.

If you’d like to aid static analysis without re‑introducing a hard dependency, you could optionally document the expected type, for example:

/** @var object|\Spatie\LaravelData\Resolvers\DataMorphClassResolver|null $dataMorphClassResolver */

before using it, but that’s not strictly necessary for correctness.

Also applies to: 21-42, 189-199

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0a9932e and 3ebf418.

📒 Files selected for processing (3)
  • src/LaravelSchemaGeneratorServiceProvider.php (2 hunks)
  • src/Resolvers/InheritingDataValidationRulesResolver.php (3 hunks)
  • tests/Fixtures/Expected/unified-schemas.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Resolvers/InheritingDataValidationRulesResolver.php (6)
tests/Fixtures/DataClasses/InheritedPostalCodeWithLocalRulesData.php (1)
  • __construct (15-18)
tests/Fixtures/DataClasses/PostalCodeMethodRulesData.php (1)
  • __construct (14-16)
src/Attributes/InheritValidationFrom.php (1)
  • __construct (33-49)
tests/Fixtures/DataClasses/InheritedPostalCodeData.php (1)
  • __construct (14-17)
tests/Fixtures/DataClasses/InheritedPostalCodeRuntimeDisabledData.php (1)
  • __construct (14-17)
src/Resolvers/InheritingDataValidationMessagesAndAttributesResolver.php (1)
  • __construct (17-20)
src/LaravelSchemaGeneratorServiceProvider.php (1)
src/Resolvers/InheritingDataValidationRulesResolver.php (2)
  • InheritingDataValidationRulesResolver (19-200)
  • requiresDataMorphClassResolver (189-199)
🔇 Additional comments (1)
tests/Fixtures/Expected/unified-schemas.ts (1)

12-16: Email field constraint style is consistent and preserves behavior

Switching account_details.email to z.string().max(255, ...) keeps the 255‑character limit and matches how other string length constraints are expressed in this fixture. No issues from the schema perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants