アノテーションを PHP 8 Attributes に移行し依存関係を更新#24
Conversation
Reviewer's GuideMigrates from Doctrine/Ray.Di docblock annotations to PHP 8 attributes, updates DI wiring to use attribute-based metadata, and modernizes the test and build setup for PHP 8 and PHPUnit 9.5. Class diagram for migrated PHP 8 attribute annotationsclassDiagram
class AbstractValidation {
+string form
+__construct(form string = "form")
}
class FormValidation {
+bool antiCsrf
+string onFailure
+__construct(form string = "form", antiCsrf bool = false, onFailure string)
}
class InputValidation {
}
class VndError {
+string message
+array href
+string logref
+string path
+__construct(message string, href array, logref string, path string)
}
AbstractValidation <|-- FormValidation
AbstractValidation <|-- InputValidation
class InjectAttribute {
}
class PostConstructAttribute {
}
class AbstractForm {
+setBaseDependencies(builder BuilderInterface, filterFactory FilterFactory, helperFactory HelperLocatorFactory) void
+postConstruct() void
}
class SetAntiCsrfTrait {
+setAntiCsrf(antiCsrf AntiCsrfInterface) void
}
AbstractForm ..> InjectAttribute : uses
AbstractForm ..> PostConstructAttribute : uses
SetAntiCsrfTrait ..> InjectAttribute : uses
FormValidation ..> AbstractValidation : calls_parent_constructor
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace Doctrine-style docblock annotations with PHP 8 attributes across source, tests, and README; add strict typing and tighten method/property signatures; remove Doctrine annotation Reader usage from interceptors/handlers; modernize tests and PHPUnit/config, update composer requirements, and add small .gitignore entries. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
AuraInputInterceptorTest::testInvalidFormPropertyByInvalidInstanceyou callexpectException(InvalidFormPropertyException::class)twice, which is redundant and can be reduced to a single call for clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AuraInputInterceptorTest::testInvalidFormPropertyByInvalidInstance` you call `expectException(InvalidFormPropertyException::class)` twice, which is redundant and can be reduced to a single call for clarity.
## Individual Comments
### Comment 1
<location> `README.JA.md:187` </location>
<code_context>
`@VndError`アノテーションで`vnd.error+json`に必要な情報を加えることができます。
```php
</code_context>
<issue_to_address>
**suggestion (typo):** Update the inline `@VndError` reference to match the new attribute-style usage shown in the example.
This sentence still refers to the old `@VndError` annotation, while the example now uses PHP 8 attributes (`#[VndError(...)]`). Please update the inline code to match the attribute style, e.g. ``#[VndError]`` or just `VndError`, to keep the documentation consistent.
```suggestion
`#[VndError]`属性で`vnd.error+json`に必要な情報を加えることができます。
```
</issue_to_address>
### Comment 2
<location> `README.md:116` </location>
<code_context>
More detail for `vnd.error+json`can be add with `@VndError` annotation.
```php
</code_context>
<issue_to_address>
**issue (typo):** Fix grammar/spacing and update `@VndError` to align with the new attribute-style example.
Update the sentence to: "More detail for `vnd.error+json` can be added with..." (add a space and use "added"). Also, align the inline `@VndError` reference with the new attribute-style example below, e.g. ``#[VndError]`` or `VndError` instead of the annotation form.
```suggestion
// validation success
More detail for `vnd.error+json` can be added with `#[VndError]`.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/AbstractAuraFormTest.php (1)
60-64:assertContainsdeprecated for string haystacks in PHPUnit 9.x.The
assertContains()method no longer supports string haystacks in PHPUnit 9.x. This test will fail at runtime.🔎 Proposed fix
public function tesetInputDataReamainedOnValidationFailure($html) { $expected = '<input id="name" type="text" name="name" value="@invalid@" />'; - $this->assertContains($expected, $html); + $this->assertStringContainsString($expected, $html); }
🤖 Fix all issues with AI Agents
In @src/Annotation/VndError.php:
- Around line 22-28: The constructor for VndError requires an $href parameter
that is unused by VndErrorHandler::optionalAttribute(); change the constructor
signature in class VndError (the __construct method) to make $href optional by
giving it a default empty array (e.g., public array $href = []) so callers need
not pass it and existing behavior remains unchanged.
In @src/AuraInputModule.php:
- Line 17: The project now references Koriym\Attributes\AttributeReader (bound
to Reader::class) but composer.json lacks an explicit dependency; open
composer.json and add "koriym/attributes": "*" (or a specific version) to the
"require" section so AttributeReader is installed, or alternatively verify that
the installed ray/di ^2.16 transitively requires koriym/attributes and update
composer.json accordingly to ensure AttributeReader::class can be autoloaded at
runtime.
In @tests/AuraInputInterceptorTest.php:
- Around line 128-129: Remove the duplicate expectException call: keep a single
$this->expectException(InvalidFormPropertyException::class) in the test and
delete the redundant second invocation so the test only declares the expected
exception once.
🧹 Nitpick comments (4)
src/SetAntiCsrfTrait.php (1)
13-15: LGTM! Attribute injection properly applied.The migration from
@Injectannotation to#[\Ray\Di\Di\Inject]attribute is correct and consistent with the pattern used insrc/AbstractForm.php.The
@paramPHPDoc on line 13 is redundant given the typed parameterAntiCsrfInterface $antiCsrf. Consider removing it:🔎 Optional cleanup
- /** @param AntiCsrfInterface $antiCsrf */ #[\Ray\Di\Di\Inject] public function setAntiCsrf(AntiCsrfInterface $antiCsrf)README.JA.md (2)
98-105: Fix indentation in code examples.The attributes have an extra leading space before
#(e.g.,#[Inject]instead of#[Inject]). This inconsistent indentation may look odd in the documentation and doesn't match standard PHP formatting conventions.🔎 Proposed fix
- #[Inject] - #[Named("contact_form")] + #[Inject] + #[Named("contact_form")] public function setForm(FormInterface $form) { $this->contactForm = $form; } - #[FormValidation(form: "contactForm", onFailure: "badRequestAction")] + #[FormValidation(form: "contactForm", onFailure: "badRequestAction")] public function createAction()
150-151: Same indentation issue here.The
#[InputValidation]attribute has an extra leading space.🔎 Proposed fix
- #[InputValidation(form: "form1")] + #[InputValidation(form: "form1")] public function createAction($name)README.md (1)
106-113: Fix indentation in code examples.Same issue as in README.JA.md - the attributes have an extra leading space before
#. This should be consistent with standard PHP formatting.🔎 Proposed fix
- #[Inject] - #[Named("contact_form")] + #[Inject] + #[Named("contact_form")] public function setForm(FormInterface $form) { $this->contactForm = $form; } - #[FormValidation(form: "contactForm", onFailure: "badRequestAction")] + #[FormValidation(form: "contactForm", onFailure: "badRequestAction")] public function createAction()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.gitignore.scrutinizer.ymlREADME.JA.mdREADME.mdcomposer.jsonphpunit.xml.distsrc/AbstractForm.phpsrc/Annotation/AbstractValidation.phpsrc/Annotation/FormValidation.phpsrc/Annotation/InputValidation.phpsrc/Annotation/VndError.phpsrc/AuraInputModule.phpsrc/SetAntiCsrfTrait.phptests/AbstractAuraFormTest.phptests/AbstractFormTest.phptests/AntiCsrfTest.phptests/AuraInputInterceptorTest.phptests/AuraInputModuleTest.phptests/Fake/FakeController.phptests/Fake/FakeControllerVndError.phptests/Fake/FakeInputValidationController.phptests/Fake/FakeInvalidController1.phptests/Fake/FakeInvalidController2.phptests/Fake/FakeInvalidController3.phptests/Fake/FakeInvalidInstanceController.phptests/FormFactoryTest.phptests/VndErrorHandlerTest.phptests/bootstrap.php
💤 Files with no reviewable changes (1)
- .scrutinizer.yml
🧰 Additional context used
🧬 Code graph analysis (17)
src/SetAntiCsrfTrait.php (2)
src/AbstractForm.php (2)
Ray(76-85)Ray(92-99)tests/Fake/FakeController.php (1)
Inject(16-21)
tests/Fake/FakeInvalidController2.php (4)
tests/Fake/FakeController.php (1)
FormValidation(23-27)tests/Fake/FakeInvalidController1.php (1)
FormValidation(9-12)tests/Fake/FakeInvalidController3.php (1)
FormValidation(16-19)tests/Fake/FakeInvalidInstanceController.php (1)
FormValidation(11-14)
src/Annotation/InputValidation.php (3)
src/Annotation/AbstractValidation.php (1)
Attribute(12-18)src/Annotation/FormValidation.php (1)
Attribute(12-22)src/Annotation/VndError.php (1)
Attribute(12-29)
tests/Fake/FakeController.php (6)
tests/Fake/FakeControllerVndError.php (1)
Inject(17-22)tests/Fake/FakeInputValidationController.php (1)
Inject(16-21)tests/Fake/FakeInvalidController3.php (2)
setForm(11-14)FormValidation(16-19)src/AbstractForm.php (1)
form(136-144)tests/Fake/FakeInvalidController1.php (1)
FormValidation(9-12)tests/Fake/FakeInvalidController2.php (1)
FormValidation(11-14)
tests/VndErrorHandlerTest.php (2)
src/Exception/ValidationException.php (1)
ValidationException(11-20)tests/FormFactoryTest.php (1)
setUp(18-22)
tests/Fake/FakeInputValidationController.php (2)
tests/Fake/FakeController.php (1)
Inject(16-21)tests/Fake/FakeControllerVndError.php (2)
Inject(17-22)InputValidation(24-28)
tests/AntiCsrfTest.php (5)
tests/AbstractAuraFormTest.php (1)
setUp(18-22)tests/AbstractFormTest.php (1)
setUp(27-31)tests/AuraInputInterceptorTest.php (1)
setUp(28-38)tests/FormFactoryTest.php (1)
setUp(18-22)tests/VndErrorHandlerTest.php (1)
setUp(20-24)
tests/Fake/FakeInvalidController3.php (4)
tests/Fake/FakeController.php (1)
FormValidation(23-27)tests/Fake/FakeInvalidController1.php (1)
FormValidation(9-12)tests/Fake/FakeInvalidController2.php (1)
FormValidation(11-14)tests/Fake/FakeInvalidInstanceController.php (1)
FormValidation(11-14)
src/Annotation/AbstractValidation.php (2)
src/Annotation/FormValidation.php (2)
Attribute(12-22)__construct(15-21)src/Annotation/InputValidation.php (1)
Attribute(11-14)
src/Annotation/VndError.php (3)
src/Annotation/AbstractValidation.php (2)
Attribute(12-18)__construct(15-17)src/Annotation/FormValidation.php (2)
Attribute(12-22)__construct(15-21)src/Annotation/InputValidation.php (1)
Attribute(11-14)
tests/AbstractAuraFormTest.php (4)
tests/AbstractFormTest.php (1)
setUp(27-31)tests/AuraInputInterceptorTest.php (1)
setUp(28-38)tests/FormFactoryTest.php (1)
setUp(18-22)tests/VndErrorHandlerTest.php (1)
setUp(20-24)
tests/AuraInputInterceptorTest.php (8)
src/Exception/InvalidFormPropertyException.php (1)
InvalidFormPropertyException(9-11)src/Exception/ValidationException.php (1)
ValidationException(11-20)tests/Fake/FakeInvalidController1.php (1)
FakeInvalidController1(7-13)tests/Fake/FakeInvalidController2.php (1)
FakeInvalidController2(7-15)tests/Fake/FakeInvalidController3.php (1)
FakeInvalidController3(7-20)tests/Fake/FakeControllerVndError.php (1)
FakeControllerVndError(10-29)src/FormValidationError.php (1)
FormValidationError(9-25)src/FormInterface.php (1)
error(29-29)
tests/AbstractFormTest.php (3)
src/Exception/CsrfViolationException.php (1)
CsrfViolationException(11-13)src/Exception/ValidationException.php (1)
ValidationException(11-20)src/AuraInputInterceptor.php (1)
AuraInputInterceptor(17-122)
tests/Fake/FakeControllerVndError.php (2)
tests/Fake/FakeController.php (1)
Inject(16-21)tests/Fake/FakeInputValidationController.php (2)
Inject(16-21)InputValidation(23-26)
tests/Fake/FakeInvalidInstanceController.php (4)
tests/Fake/FakeController.php (1)
FormValidation(23-27)tests/Fake/FakeInvalidController1.php (1)
FormValidation(9-12)tests/Fake/FakeInvalidController2.php (1)
FormValidation(11-14)tests/Fake/FakeInvalidController3.php (1)
FormValidation(16-19)
src/AbstractForm.php (2)
src/SetAntiCsrfTrait.php (1)
Ray(14-18)tests/Fake/FakeInputValidationController.php (1)
Inject(16-21)
src/Annotation/FormValidation.php (2)
src/Annotation/AbstractValidation.php (2)
Attribute(12-18)__construct(15-17)src/Annotation/InputValidation.php (1)
Attribute(11-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (37)
tests/AntiCsrfTest.php (1)
17-17: LGTM! PHPUnit modernization is correct.The changes successfully update the test to PHPUnit 9 style: importing
PHPUnit\Framework\TestCase, extendingTestCase, and adding thevoidreturn type tosetUp().Also applies to: 19-19, 33-33
.gitignore (1)
14-16: LGTM! Appropriate ignore rules for test artifacts.The additions correctly ignore temporary test files and PHPUnit cache while preserving the directory structure via the placeholder exception.
tests/Fake/FakeInvalidInstanceController.php (1)
11-11: LGTM! Correct attribute syntax.The migration from docblock annotation to PHP 8 attribute is correctly implemented.
tests/Fake/FakeInvalidController1.php (1)
9-9: LGTM! Correct attribute with named parameter.The PHP 8 attribute syntax with the named parameter is correctly implemented.
tests/AuraInputInterceptorTest.php (3)
9-9: LGTM! PHPUnit modernization is correct.The test class has been successfully updated to PHPUnit 9 style with the correct import, base class, and
voidreturn type onsetUp().Also applies to: 16-16, 28-28
106-106: LGTM! Correct migration to modern PHPUnit exception API.The replacement of
setExpectedExceptionwithexpectExceptionis the correct PHPUnit 9+ API.Also applies to: 113-113, 120-120
136-154: LGTM! VndError test correctly updated.The test now uses
FakeControllerVndError, includes an explicit failure assertion, and validates the updated JSON structure with thelogreffield.tests/AuraInputModuleTest.php (1)
9-14: LGTM! PHPUnit 9.x migration is correct.The test class properly extends
TestCaseand usesexpectException()before the code that triggers the exception, which is the correct pattern for PHPUnit 9.x.Also applies to: 23-30
src/Annotation/AbstractValidation.php (1)
10-17: LGTM! Clean migration to PHP 8 attribute with constructor promotion.The conversion to a PHP 8 attribute with constructor property promotion is well-implemented. Note that both this base class and its subclasses (
FormValidation,InputValidation) declare#[Attribute], which allows each to be used independently as method attributes.src/Annotation/FormValidation.php (1)
10-21: LGTM! Well-structured constructor with selective property promotion.Good pattern:
$formis correctly not promoted (no visibility modifier) since it's passed to the parent constructor, while$antiCsrfand$onFailureare promoted as class-specific properties. Thestring|nullunion type is appropriate for PHP 8+.tests/AbstractAuraFormTest.php (1)
9-11: LGTM! PHPUnit 9.x base class and setUp signature updated correctly.Also applies to: 18-18
tests/Fake/FakeController.php (1)
16-17: LGTM! Clean migration to PHP 8 attributes for DI and validation.The dependency injection (
#[Inject],#[Named]) and validation (#[FormValidation]) attributes are correctly applied and consistent with the patterns used across other test fixtures in this PR.Also applies to: 23-23
src/AbstractForm.php (1)
76-76: LGTM! Clean migration to PHP 8 attributes.The dependency injection and post-construction attributes are correctly applied using fully-qualified class names. The method signatures remain unchanged, ensuring backward compatibility at the API level.
Also applies to: 92-92
composer.json (3)
37-41: Configuration for Composer plugin looks correct.The
allow-pluginsconfiguration foraura/installer-defaultis required by Composer 2.2+ for security. This prevents the installer from running without explicit permission.
18-18: PHPUnit 9.5 compatibility is already ensured in the test suite.The test suite is fully compatible with PHPUnit 9.5. All tests correctly use
PHPUnit\Framework\TestCasenamespace, employexpectException()instead of deprecatedsetExpectedException(), and no deprecated PHPUnit patterns are present. While test methods lack explicitvoidreturn type declarations, this is optional and not required for PHPUnit 9.5 compatibility.Likely an incorrect or invalid review comment.
9-11: Dependency versions verified for PHP 8 attribute support.The versions
ray/di ^2.16andray/aop ^2.14are available and fully support PHP 8 attributes:
- Ray.Di 2.16.0+ removed doctrine/annotation dependency and uses PHP 8 attributes read via reflection through Ray.Aop
- Ray.Aop 2.17.0+ explicitly supports method parameter attributes
- Both support PHP 8.3+ and maintain active development with attribute-focused features
tests/bootstrap.php (1)
7-7: LGTM! Correctly removed Doctrine annotation registry.With PHP 8 native attributes, the Doctrine
AnnotationRegistryis no longer needed. The simplified bootstrap now only requires the Composer autoloader, which is sufficient for attribute-based metadata.tests/Fake/FakeInvalidController2.php (1)
11-11: Flag inconsistency between code and AI summary.The AI summary indicates this file should have
#[FormValidation(form: "contactForm", onFailure: "badRequestAction")], but the actual code shows bare#[FormValidation]without parameters. Given that this isFakeInvalidController2(a test fixture for invalid configurations) and$formisnull, this appears intentional. However, the discrepancy should be clarified.src/Annotation/InputValidation.php (1)
9-11: LGTM! Correct PHP 8 attribute declaration.The attribute is properly declared with
Attribute::TARGET_METHOD, allowing it to be applied to methods. The class correctly extendsAbstractValidationto inherit base validation functionality while serving as a marker attribute for input validation.phpunit.xml.dist (1)
1-21: LGTM! PHPUnit configuration modernized correctly.The configuration has been properly updated to PHPUnit 9.5+ standards: testsuite naming, coverage configuration migrated from deprecated
whitelisttoinclude/reportstructure, and logging consolidated. All changes align with modern PHPUnit conventions.tests/FormFactoryTest.php (1)
9-18: LGTM! Test class properly modernized for PHPUnit 9.5.The test class has been correctly updated with the namespaced
TestCaseimport, modern class extension, and void-typedsetUp()method, consistent with PHPUnit 9.5+ requirements and other migrated tests in the codebase.tests/Fake/FakeInputValidationController.php (2)
23-26: LGTM! InputValidation attribute correctly applied.The
#[InputValidation]attribute has been properly migrated from the docblock annotation, consistent with other test controllers in the codebase.
16-21: Ray.Di 2.16.0 fully supports PHP 8 attributes for dependency injection.The
#[Inject]and#[Named('contact_form')]attributes are correctly implemented and consistent with other migrated controllers in the codebase (e.g., FakeController.php). No further action needed.tests/AbstractFormTest.php (5)
14-20: LGTM! Imports and class signature properly updated.The imports have been correctly updated to include
AttributeReaderfor PHP 8 attribute support, the namespacedTestCasefor PHPUnit 9.5+, andValidationExceptionfor explicit exception handling. The class signature modernization is consistent with other test files.
27-31: LGTM! setUp method signature modernized.The
setUp()method now has the void return type required by PHPUnit 8+, consistent with other modernized test files.
66-69: LGTM! Exception handling modernized.The test correctly uses
expectException()instead of the deprecatedsetExpectedException(), following PHPUnit 8+ conventions.
94-103: LGTM! Exception expectation properly updated.The CSRF violation test has been correctly updated to use the modern
expectException()API.
44-55: ReflectiveMethodInvocation constructor accepts the parameters correctly.The constructor signature in ray/aop 2.14.0 expects:
object $object— the controllerstring $method— the method name as a stringarray $arguments— the method argumentsarray $interceptors— optional array of interceptorsThe code correctly passes
'createAction'as a string for the method name, along with the proper argument order. The switch toAttributeReaderand this constructor usage are both correct for the attribute migration to PHP 8.tests/Fake/FakeInvalidController3.php (1)
16-19: LGTM! FormValidation attribute correctly migrated.The
#[FormValidation(onFailure: "missing_method")]attribute has been properly migrated from the docblock annotation with correct named parameter syntax. The intentionally invalid method reference is appropriate for this test fixture.tests/Fake/FakeControllerVndError.php (2)
17-18: LGTM!The migration to PHP 8 attributes for
#[Inject]and#[Named('contact_form')]is correct and consistent with other test controllers likeFakeController.phpandFakeInputValidationController.php.
24-25: LGTM!The
#[InputValidation]and#[VndError]attributes are correctly applied with proper named parameters. Thehrefarray syntax within the attribute is valid PHP 8 syntax and matches theVndErrorclass constructor signature.README.JA.md (1)
190-191: LGTM!The
#[FormValidation]and#[VndError]attribute examples are correctly formatted and demonstrate the proper PHP 8 attribute syntax with thehrefassociative array.tests/VndErrorHandlerTest.php (3)
9-9: LGTM!Correct import of the namespaced PHPUnit TestCase class for PHPUnit 9.x compatibility.
13-13: LGTM!The class correctly extends
PHPUnit\Framework\TestCaseand thesetUp()method has the requiredvoidreturn type for PHPUnit 8+/9.x.Also applies to: 20-20
28-28: LGTM!Correctly updated from the deprecated
setExpectedException()toexpectException(), which is the modern PHPUnit 9.x API.README.md (1)
185-186: LGTM!The
#[FormValidation]and#[VndError]examples correctly demonstrate PHP 8 attribute syntax with all the named parameters including thehrefassociative array.src/Annotation/VndError.php (1)
10-12: LGTM!The
#[Attribute(Attribute::TARGET_METHOD)]declaration is correct for targeting method-level attributes, consistent with the other annotation classes in this module (AbstractValidation,FormValidation,InputValidation).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.JA.md (1)
187-191: Minor: Simplify redundant Japanese expression.The phrase "加えることができます" on line 187 is somewhat verbose. In Japanese documentation, "加えます" is more direct and concise.
🔎 Proposed simplification
-`#[VndError]`属性で`vnd.error+json`に必要な情報を加えることができます。 +`#[VndError]`属性で`vnd.error+json`に必要な情報を加えます。This change maintains the same meaning while improving readability of the documentation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.JA.mdREADME.md
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🪛 LanguageTool
README.JA.md
[uncategorized] ~187-~187: 「ことができる」という表現は冗長な可能性があります。
Context: ...[VndError]属性でvnd.error+json`に必要な情報を加えることができます。 ```php #[FormValidation(form: "...
(DOUSI_KOTOGADEKIRU)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
README.JA.md (2)
98-105: ✓ Controller example correctly migrated to PHP 8 attributes.The
setForm()andcreateAction()methods now correctly use the#[Inject],#[Named("contact_form")], and#[FormValidation(...)]attributes, properly reflecting the migration from docblock annotations to PHP 8 attributes.
150-150: ✓ InputValidation example correctly shows attribute syntax.The
#[InputValidation(form: "form1")]attribute is correctly displayed in the example.
|
@apple-x-co Aura.Htmlがおそらくすぐにメジャーバージョンでリリースされると思います。それに対応してからにしましょうか？ |
|
@koriym 承知いたしました。 |
|
いえいえ、これでいきましょう！
Scrutinizer のエラーですが、他のレポジトリのをそのままコピーしてペーストしてもらえばOKです。 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.scrutinizer.yml (1)
1-12: Consider retaining at least basic code quality tools.While the PR objectives mention relaxing static analysis settings, the complete removal of all tools (php_mess_detector, php_code_sniffer, php_analyzer, php_cpd, etc.) eliminates automated code quality checks entirely.
As noted in the PR comments, koriym suggested copying Scrutinizer configuration from other repositories to address errors. Consider retaining at least a minimal set of tools (e.g., php_code_sniffer, php_analyzer) to maintain basic quality gates while the migration is finalized.
Is this the intended final configuration, or will additional tools be re-added based on other repository configurations?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.scrutinizer.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (1)
.scrutinizer.yml (1)
1-9: Build configuration aligns with PHP 8.0 migration.The new build block correctly specifies PHP 8.0 and the default-jammy image, which aligns well with the repository's migration to PHP 8 attributes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/AuraInputInterceptorTest.php (2)
40-82: Consider removing commented code.This 40+ line block of commented methods adds noise without providing value. If these methods are obsolete after the attribute migration, remove them. If they serve as reference, version control history is more appropriate.
🔎 Proposed cleanup
-// /** -// * @param $method -// */ -// public function getMethodInvocation(string $method, array $submit, FailureHandlerInterface $handler = null) -// { -// $handler = $handler ?: new OnFailureMethodHandler; -// $object = $this->getController($submit); -// -// $invocation = new ReflectiveMethodInvocation( -// $object, -// $method, -// $submit, -// [ -// new AuraInputInterceptor(new AnnotationReader, $handler) -// ] -// ); -// -// return $invocation; -// } -// -// public function getController(array $submit) -// { -// $controller = new FakeController; -// /** @var $fakeForm FakeForm */ -// $fakeForm = (new FormFactory)->newInstance(FakeForm::class); -// $fakeForm->setSubmit($submit); -// $controller->setForm($fakeForm); -// -// return $controller; -// } -// -// public function proceed($controller) -// { -// $invocation = new ReflectiveMethodInvocation( -// $controller, -// new \ReflectionMethod($controller, 'createAction'), -// [], -// [ -// new AuraInputInterceptor(new AnnotationReader, new OnFailureMethodHandler) -// ] -// ); -// $invocation->proceed(); -// } -
96-102: Remove unusedinvalidControllerProvidermethod.This method is not referenced by any test (no
@dataProviderannotation) and appears to be dead code, possibly leftover from refactoring.🔎 Proposed cleanup
- public function invalidControllerProvider() - { - return [ - [$this->injector->getInstance(FakeInvalidController1::class)], - [$this->injector->getInstance(FakeInvalidController2::class)] - ]; - } -
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.scrutinizer.ymltests/AuraInputInterceptorTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/AuraInputInterceptorTest.php (5)
src/Exception/InvalidFormPropertyException.php (1)
InvalidFormPropertyException(9-11)src/Exception/ValidationException.php (1)
ValidationException(11-20)tests/Fake/FakeControllerVndError.php (1)
FakeControllerVndError(10-29)src/FormValidationError.php (1)
FormValidationError(9-25)src/FormInterface.php (1)
error(29-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
.scrutinizer.yml (2)
1-9: LGTM! PHP 8.2 configuration is appropriate.The build configuration correctly sets up PHP 8.2 for Scrutinizer analysis, which aligns well with the PR's migration to PHP 8.0+ and attributes. The
php-scrutinizer-runcommand is the standard approach for running Scrutinizer's analysis tools.
1-12: Verify that removing all static analysis tools is intentional.The configuration has been simplified by removing all explicit tool configurations (php_mess_detector, php_code_sniffer, php_analyzer, etc.). While this aligns with the commit message "設定構造を最適化", the PR comments discuss potentially addressing phpcs issues in the future.
Please confirm whether:
- These tools are intentionally removed in favor of
php-scrutinizer-run's default behavior- Code style checks (phpcs) will be re-added later, or handled through a different mechanism
Based on the PR discussion, koriym mentioned copying configuration from other repositories to resolve Scrutinizer errors, which might include some of these tools.
tests/AuraInputInterceptorTest.php (3)
9-9: LGTM: PHPUnit 9 modernization correctly applied.The migration to namespaced
PHPUnit\Framework\TestCaseand the addition of avoidreturn type onsetUp()correctly align with PHPUnit 9 requirements.Also applies to: 16-16, 28-28
106-106: LGTM: Exception expectations correctly updated.All
expectException()calls use the modern PHPUnit 9 API correctly. The duplicate issue flagged in previous reviews has been resolved.Also applies to: 113-113, 120-120, 128-128
133-153: LGTM: VndError test correctly updated for attribute-based metadata.The test properly validates the new
#[VndError]attribute behavior:
- Correctly instantiates
FakeControllerVndErrorwith attribute-based configuration- Explicit
fail()assertion improves test robustness- Expected JSON payload matches the attribute parameters (
message,path,logref) defined inFakeControllerVndError
|
|
|
先日の対応（アノテーションからアトリビュートに移行）に加えて以下を対応
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/FormValidationError.php (1)
27-30:⚠️ Potential issue | 🟡 Minor
json_encode()can returnfalse, which violates__toString(): string.If the array contains values that can't be encoded (e.g., malformed UTF-8),
json_encode()returnsfalse. Withdeclare(strict_types=1)and PHP 8's strict__toString()return type enforcement, this will throw aTypeError.Proposed fix
public function __toString() { - return json_encode($this->value, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); + return json_encode($this->value, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES) ?: ''; }
🤖 Fix all issues with AI agents
In `@src/AbstractForm.php`:
- Around line 159-167: The docblock for getFailureMessages() is wrong:
getFailures()->getMessages() returns an associative array keyed by field name
with arrays of message strings, not a list; update the PHPDoc return annotation
in AbstractForm::getFailureMessages() from "list<string>" to a keyed array type
such as "array<string, list<string>>" or "array<string, string[]>" (e.g.
array<string, string[]>) to accurately reflect the shape returned by
getFailures()->getMessages(); keep the method signature as is.
In `@src/AntiCsrf.php`:
- Around line 37-41: Add a unit test that asserts AntiCsrf::setField() actually
produces a hidden input with the CSRF token in rendered HTML: instantiate the
real AntiCsrf, call setField($fieldset) on a Fieldset instance (matching how
FakeAntiCsrf was tested in testAntiCsrfForm()), render the field/fieldset and
assert the output contains an input[type="hidden"] whose name equals
AntiCsrf::TOKEN_KEY and whose value equals the token returned by getToken();
this mirrors the existing FakeAntiCsrf coverage and verifies the switch from
setValue() to setAttribs(['value'=>...]) behaves correctly.
In `@src/Exception/InvalidOnFailureMethod.php`:
- Around line 13-15: InvalidOnFailureMethod currently imports and extends the
global LogicException without implementing the package's ExceptionInterface,
causing inconsistency with other exceptions; update the class declaration for
InvalidOnFailureMethod (and similarly InvalidFormPropertyException) by either
removing the "use LogicException" so the class extends the package-local
LogicException that already implements ExceptionInterface, or keep the global
LogicException but add "implements ExceptionInterface" to the class declaration
so it explicitly fulfills the package exception contract; modify the class
header accordingly and ensure ExceptionInterface is referenced/imported as
needed.
🧹 Nitpick comments (8)
src/OnFailureMethodHandler.php (1)
30-40: Use the already-assigned$objectvariable consistently.
$objectis assigned on line 30 but$invocation->getThis()is called again on lines 32, 37, and 40. Reusing$objectavoids redundant method calls and improves readability.♻️ Proposed fix
$object = $invocation->getThis(); if (! $formValidation instanceof FormValidation) { - throw new InvalidOnFailureMethod(get_class($invocation->getThis())); + throw new InvalidOnFailureMethod(get_class($object)); } $onFailureMethod = $formValidation->onFailure ?: $invocation->getMethod()->getName() . self::FAILURE_SUFFIX; if (! method_exists($object, $onFailureMethod)) { - throw new InvalidOnFailureMethod(get_class($invocation->getThis())); + throw new InvalidOnFailureMethod(get_class($object)); } - return call_user_func_array([$invocation->getThis(), $onFailureMethod], $args); + return call_user_func_array([$object, $onFailureMethod], $args); }src/AntiCsrf.php (1)
23-25: Test-specific constant exposed in production code.
TEST_TOKENis only meaningful in CLI/test mode. Exposing it as apublic constleaks a testing detail into the production API surface. Consider keeping itprivate(or moving it to test fixtures) if external consumers don't need it.src/Exception/InvalidFormPropertyException.php (1)
13-15: Ambiguous import — consider extending the siblingLogicExceptioninstead.
use LogicException;imports\LogicExceptionfrom the global namespace, not the siblingRay\WebFormModule\Exception\LogicExceptionin the same namespace. This is functionally identical to the previousextends \LogicException, but the import makes it look like it refers to the local class.Every other exception in this namespace implements
ExceptionInterface(either directly or via the localLogicException). If that's the intended contract here too, drop the import and extend the local class instead:♻️ Suggested change
-use LogicException; - -class InvalidFormPropertyException extends LogicException +class InvalidFormPropertyException extends \Ray\WebFormModule\Exception\LogicExceptionOr simply, since it's in the same namespace:
-use LogicException; - -class InvalidFormPropertyException extends LogicException +class InvalidFormPropertyException extends LogicException(remove the
usestatement — the unqualifiedLogicExceptionwill then resolve to the sibling class inRay\WebFormModule\Exception).src/VndErrorHandler.php (1)
13-13: Unused import:Doctrine\Common\Annotations\Reader.This import is a leftover from the pre-migration code — the
Readeris no longer injected or used anywhere in the class. Removing it keeps the file consistent with the migration goal.Remove unused import
-use Doctrine\Common\Annotations\Reader;src/FormInterface.php (1)
21-24: Inconsistent return type declarations betweeninput()anderror().
error()now has an explicit: stringreturn type (line 24), whileinput()still relies only on the@return stringdocblock (line 21). If both are guaranteed to returnstring, consider adding: stringtoinput()as well for consistency. Ifinput()intentionally omits the return type for flexibility, a brief docblock note would clarify the reasoning.Add return type to `input()` for consistency
- public function input(string $input); + public function input(string $input): string;src/AbstractForm.php (1)
102-106: Missing type hint on$inputparameter.The
FormInterfacedefinesinput(string $input), but this implementation omits the type hint. While PHP allows this (contravariant parameter types), it's inconsistent with the rest of the file's strict-typing effort and with theerror(string $input)method on line 109.Proposed fix
- public function input($input) + public function input(string $input)src/FormFactory.php (1)
19-27: Consider validating$classis a subclass ofAbstractForm.
new $classwill instantiate any class. If a non-AbstractFormclass string is passed, the error will surface as a method-not-found onsetBaseDependencies()rather than a clear, descriptive message. A quickis_subclass_ofcheck would improve DX.Proposed improvement
public function newInstance(string $class): AbstractForm { + if (! is_subclass_of($class, AbstractForm::class)) { + throw new \InvalidArgumentException($class . ' must extend ' . AbstractForm::class); + } + /** `@var` $form AbstractForm */ $form = new $class;src/Exception/ValidationException.php (1)
18-24: Implicitly nullable parameter types are deprecated in PHP 8.4+.
Exception $e = nullandFormValidationError $error = nulluse implicit nullable syntax, which is deprecated as of PHP 8.4. Use explicit nullable types for forward compatibility.Also,
$errorproperty on line 18 lacks a type declaration, inconsistent with the strict typing effort.Proposed fix
- public $error; + public ?FormValidationError $error; - public function __construct($message = '', $code = 0, Exception $e = null, FormValidationError $error = null) + public function __construct(string $message = '', int $code = 0, ?Exception $e = null, ?FormValidationError $error = null) {
koriym
left a comment
There was a problem hiding this comment.
再レビュー（手元でチェックアウトして phpunit を実行した結果ベース）
PR の方向性（Doctrine Annotations → PHP 8 Attributes、declare(strict_types=1)、PHPUnit 9 への更新）は素晴らしいと思います。ただし、CI/テスト実行に関する重大な問題が残っているのでまとめます。
🔴 Critical: #[Named] の配置によりテストが落ちる
PR ブランチ（dfafacb）を checkout して composer install && vendor/bin/phpunit を実行すると、27 テスト中 11 errors / 1 failure が発生します（成功 15）。
Ray\Di\Exception\Unbound: 'Ray\WebFormModule\FormInterface-' in tests/Fake/FakeController.php:18 ($form)
末尾の - は ray/di の内部表現で「名前付きバインディングの name 部分が空文字列」であることを示しています。
原因: Doctrine 時代の @Named("contact_form") はメソッドレベルの DocBlock でもパラメータに伝播していましたが、ray/di ^2.16（手元で 2.16.0 と 2.20.0 の両方で確認）の Ray\Di\Name::withAttributes() は、PHP 8 Attributes ではパラメータレベルの ReflectionParameter::getAttributes() しか参照しません。BcParameterQualifier のフォールバックも #[Inject] と Qualifier を同時に満たす属性のみが対象で、Ray\Di\Di\Named には適用されません。
該当ファイル:
tests/Fake/FakeController.php:16-17tests/Fake/FakeControllerVndError.php:17-18tests/Fake/FakeInputValidationController.php:16-17README.md:106-108/README.JA.md:98-100
修正例（パラメータレベルに移動）:
// Before（落ちる）
#[Inject]
#[Named('contact_form')]
public function setForm(FormInterface $form)
// After（テストが通る）
#[Inject]
public function setForm(#[Named('contact_form')] FormInterface $form)手元で 3 つの Fake コントローラを上記のように直すと、27 tests / 29 assertions すべて通過することを確認済みです。
🟠 Major: src/VndErrorHandler.php:13 の未使用 import
use Doctrine\Common\Annotations\Reader;Reader は今回の改修でクラス本体から完全に消えていますが、use 文が残っています。Doctrine Annotations 依存を捨てる主旨の PR なので、この import も削除してください。
🟠 Major: 例外クラスが ExceptionInterface を実装していない
src/Exception/InvalidOnFailureMethod.php と src/Exception/InvalidFormPropertyException.php は、グローバルの \LogicException（use LogicException; 経由）を継承しており、パッケージ独自の ExceptionInterface を実装していません。RuntimeException / InvalidArgumentException は明示的に implements ExceptionInterface を付けているので、パターンが揃っていません。
修正案：use LogicException; を消してパッケージ内の Ray\WebFormModule\Exception\LogicException（既に ExceptionInterface を実装済み）を継承する。
namespace Ray\WebFormModule\Exception;
class InvalidOnFailureMethod extends LogicException
{
}（CodeRabbit の coderabbitai レビューの該当指摘と同じ内容です。）
🟡 Minor: AbstractForm::getFailureMessages() の @return が誤り
src/AbstractForm.php:159-167
/**
* Returns all failure messages for all fields.
*
* @return list<string>
*/
public function getFailureMessages(): array
{
return $this->filter->getFailures()->getMessages();
}Aura\Filter\SubjectFilter::getFailures()->getMessages() はフィールド名でキー付けされた連想配列（各値は文字列のリスト）を返します。したがって正しいのは：
@return array<string, list<string>>VndErrorHandler 内でも validation_messages として {"name": ["Name must be ..."]} の形で吐いており、フラットな list<string> ではないことが確認できます。
🟡 Minor: VndError::$href が required だが未使用
src/Annotation/VndError.php:25-31
public function __construct(
public string $message,
public array $href, // ← 必須だが Handler は読まない
public string|null $logref = null,
public string|null $path = null
) {
}VndErrorHandler::optionalAttribute() は message, path, logref しか使っていないので、$href は実質的にデッドコードです。デフォルト array $href = [] にしておくと利用側の記述が楽になります（互換性も維持されます）。
🟡 Minor: AbstractValidation 関連
src/Annotation/AbstractValidation.php
- クラス名が
Abstract...なのにabstract修飾子が付いていない（直接#[AbstractValidation]として書けてしまう）。 - 親クラスに
#[Attribute(Attribute::TARGET_METHOD)]が付いていますが、Attribute は継承されないので、サブクラスで個別に#[Attribute]が付いている以上、親側は不要です。Interceptor 側もReflectionAttribute::IS_INSTANCEOFで拾うので問題ありません。
修正案：
#[\Attribute(\Attribute::TARGET_METHOD)] // ← 削除
abstract class AbstractValidation // ← abstract を付与
{
public function __construct(public string $form = 'form') {}
}🟡 Minor: README のドキュメント不整合
README.md:183に@VndError annotationという古い表記が残っています（Sourcery は別の箇所だけ拾っていました）。README.JA.md:82-83に@FormValidation/onFailure/formのアノテーション表記が残っています。本文も Attributes 形式へ更新したほうが整合性が取れます。
参考: CodeRabbit の koriym/attributes 指摘について
CodeRabbit のレビュー（src/AuraInputModule.php）で「koriym/attributes を composer.json に追加すべき」という指摘がありますが、この PR では Koriym\Attributes\AttributeReader を使っていません（grep しても参照ゼロ）。実装はネイティブの ReflectionMethod::getAttributes() に統一されているので、この指摘は対応不要です。Sourcery のサマリの「Replace Doctrine AnnotationReader with Koriym AttributeReader」も実態と異なります。
検証手順（再現可能）
git fetch origin pull/24/head:pr-24 && git checkout pr-24
composer install
rm -rf tests/tmp/*.php
vendor/bin/phpunit --testdox
# => 11 errors + 1 failure, ray/di 2.20.0 / PHP 8.xtests/Fake/Fake*Controller*.php の #[Named(...)] をパラメータレベルに移すと全件パスします。
まとめ
方向性は良いので、上記の Critical（テスト失敗）を解消すれば mergeable になると思います。README のサンプルコードも合わせて修正すると、利用者が同じ落とし穴に嵌まりません。よろしくお願いします。
Generated by Claude Code
There was a problem hiding this comment.
Pull request overview
Modernizes Ray.WebFormModule for PHP 8: migrates Doctrine annotations to native PHP 8 attributes, raises the minimum PHP version to 8.0, bumps ray/di and ray/aop, upgrades the test suite to PHPUnit 9, and updates source files with declare(strict_types=1) and modern type hints.
Changes:
- Convert
@FormValidation,@InputValidation,@VndError,@Inject,@Named, and@PostConstructannotation usages to#[Attribute]syntax; reworkAbstractValidation/FormValidation/VndErroras constructor-promoted attribute classes; drop the DoctrineReaderdependency fromAuraInputInterceptor/VndErrorHandler/InputValidationInterceptorin favor of native reflection. - Raise PHP requirement to ^8.0, bump ray/di and ray/aop, upgrade PHPUnit to ^9.5, refresh phpunit.xml.dist coverage config, drop Travis, and trim Scrutinizer config.
- Modernize tests (PHPUnit 9
TestCase/expectException/setUp(): void), update Fake controllers to use attributes, and adjust expected VndError JSON in tests.
Reviewed changes
Copilot reviewed 48 out of 49 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Annotation/AbstractValidation.php | Converted to abstract class with constructor-promoted $form. |
| src/Annotation/FormValidation.php | Re-implemented as PHP 8 attribute with promoted properties. |
| src/Annotation/InputValidation.php | Re-implemented as PHP 8 attribute. |
| src/Annotation/VndError.php | Re-implemented as attribute; message/href now required. |
| src/AuraInputInterceptor.php | Drops Doctrine Reader; reads validation attribute via reflection. |
| src/AuraInputModule.php | Removes Reader binding. |
| src/InputValidationInterceptor.php | Uses #[Named] parameter attribute; removes Reader param. |
| src/VndErrorHandler.php | Reads VndError attribute via reflection; supports href. |
| src/FormVndErrorModule.php | strict_types and minor cleanup. |
| src/FailureHandlerInterface.php | strict_types added. |
| src/AbstractForm.php | strict_types, modern types, #[Inject]/#[PostConstruct]. |
| src/AntiCsrf.php | strict_types, typed props, public consts, strict equality. |
| src/FormFactory.php | Typed signature for newInstance. |
| src/FormInterface.php | Typed parameters/returns for input/error. |
| src/FormValidationError.php | strict_types and typed property. |
| src/OnFailureMethodHandler.php | strict_types and removed redundant check. |
| src/SetAntiCsrfTrait.php | Uses #[Inject] attribute and void return. |
| src/SubmitInterface.php | strict_types. |
| src/ToStringInterface.php | strict_types and minor cleanup. |
| src/Exception/*.php | strict_types added; some exceptions now extend project LogicException. |
| tests/*.php | PHPUnit 9 migration (TestCase, expectException, setUp(): void). |
| tests/Fake/*.php | Annotations replaced with PHP 8 attributes. |
| tests/AbstractFormTest.php | Removes AnnotationReader; uses ValidationException expectation. |
| tests/AuraInputInterceptorTest.php | Updates expected VndError JSON; leaves commented-out blocks. |
| tests/bootstrap.php | Removes AnnotationRegistry registration. |
| phpunit.xml.dist | Updated to PHPUnit 9 coverage/logging schema. |
| composer.json | PHP ^8.0, ray/di ^2.16, ray/aop ^2.14, phpunit ^9.5, allow-plugins. |
| .gitignore | Adds tests/tmp and phpunit cache entries. |
| .travis.yml | Removed. |
| .scrutinizer.yml | Trimmed to php-scrutinizer-run on PHP 8.2. |
| README.md / README.JA.md | Examples migrated to attribute syntax. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // public function invalidControllerProvider() | ||
| // { | ||
| // return [ | ||
| // [$this->injector->getInstance(FakeInvalidController1::class)], | ||
| // [$this->injector->getInstance(FakeInvalidController2::class)] | ||
| // ]; | ||
| // } |
There was a problem hiding this comment.
(Reposting in English to match the original review language.)
Agreed. Removed the stale commented-out blocks (lines 40–82 and 96–102) that referenced the AnnotationReader-based constructor.
Fix commit: claude/review-item-24-9tucR @ a1cfe37 — a1cfe37
Generated by Claude Code
| public function __construct( | ||
| public string $message, | ||
| public array $href, | ||
| public string|null $logref = null, | ||
| public string|null $path = null | ||
| ) { | ||
| } |
There was a problem hiding this comment.
(Reposting in English to match the original review language.)
Agreed — requiring $message and $href was a BC break against the previous annotation API. Restored backward compatibility by giving both parameters defaults: $message = '' and $href = [].
VndErrorHandler::optionalAttribute() already guards each field with a truthy check (if ($vndError->message) etc.), so an empty string / empty array is skipped naturally.
Fix commit: claude/review-item-24-9tucR @ a1cfe37 — a1cfe37
Generated by Claude Code
|
|
||
| use Doctrine\Common\Annotations\Reader; | ||
| use Ray\Aop\MethodInvocation; | ||
| use Ray\Aop\ReflectionMethod; |
There was a problem hiding this comment.
(Reposting in English to match the original review language.)
Agreed. Aligned VndErrorHandler::getVndErrorAttribute() with AuraInputInterceptor::getValidationAttribute() by switching to the built-in \ReflectionMethod. Since Ray\Aop\ReflectionMethod extends it, there is no runtime impact.
Fix commit: claude/review-item-24-9tucR @ a1cfe37 — a1cfe37
Generated by Claude Code
| namespace Ray\WebFormModule\Exception; | ||
|
|
||
| class RuntimeException extends \LogicException implements ExceptionInterface | ||
| class RuntimeException extends LogicException |
There was a problem hiding this comment.
(Reposting in English to match the original review language.)
Agreed — the original misnaming had been preserved. Corrected the hierarchy to class RuntimeException extends \RuntimeException implements ExceptionInterface so the semantics now match the class name.
Fix commit: claude/review-item-24-9tucR @ a1cfe37 — a1cfe37
Generated by Claude Code
- VndError: $message と $href にデフォルト値を付与し旧 annotation との BC を維持 - VndErrorHandler: Ray\Aop\ReflectionMethod を \ReflectionMethod に変更し AuraInputInterceptor と型を揃える - RuntimeException: 意味的に正しい \RuntimeException を継承するよう修正し ExceptionInterface を明示 - AuraInputInterceptorTest: AnnotationReader 時代のコメントアウト済みコード塊を削除
|
@coderabbitai approve |
✅ Actions performedComments resolved. Approval is disabled; enable |
- ValidationException, VndErrorHandler: implicit nullable parameter (Type $x = null) is deprecated in PHP 8.4. Use ?Type $x = null. - AbstractForm: type $errorMessages and $antiCsrf with nullable property types. $filter remains untyped because parent Aura\Input\Fieldset declares it untyped and PHP forbids adding a type in a subclass for such properties.
Summary by Sourcery
Migrate annotation-based validation and error handling to PHP 8 attributes while updating dependencies and tests for PHP 8 and modern PHPUnit.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Summary by CodeRabbit
Chores
Refactor
Tests
Documentation