-
Notifications
You must be signed in to change notification settings - Fork 0
Prep work for PHP 8.5 #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis 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
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this 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 signatureThe new
resolveDataMorphClassResolver()helper plusInheritingDataValidationRulesResolver::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): ?objectBehavior 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 solidThe constructor’s shift to
?object $dataMorphClassResolver = nullplus the$parametersarray andrequiresDataMorphClassResolver()guard cleanly supports both Spatie versions with and without the legacy morph resolver, and the explicitRuntimeExceptionis 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
📒 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 behaviorSwitching
account_details.emailtoz.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.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.