アノテーションを 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
|
📝 WalkthroughWalkthroughReplaces Doctrine-style docblock annotations with PHP 8 attributes across source, tests, and README; swaps AnnotationReader for AttributeReader; updates composer/phpunit requirements and CI config; modernizes tests and simplifies test bootstrap; adds .gitignore entries. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom Pre-merge Checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
apple-x-co
changed the title
apple-x-co
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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.
| #[FormValidation(form: "contactForm", onFailure: "badRequestAction")] | ||
| public function createAction() | ||
| { | ||
| // validation success |
There was a problem hiding this comment.
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.
| // validation success | |
| // validation success | |
| More detail for `vnd.error+json` can be added with `#[VndError]`. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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).
| 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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for VndError usages
rg -n "VndError" --type php -B 2 -A 2Repository: ray-di/Ray.WebFormModule
Length of output: 6306
🏁 Script executed:
#!/bin/bash
# Read the VndError class definition
cat -n src/Annotation/VndError.phpRepository: ray-di/Ray.WebFormModule
Length of output: 938
🏁 Script executed:
#!/bin/bash
# Read the VndErrorHandler methods that work with VndError
cat -n src/VndErrorHandler.phpRepository: ray-di/Ray.WebFormModule
Length of output: 2350
🏁 Script executed:
#!/bin/bash
# Search for direct VndError instantiations
rg "new VndError" --type phpRepository: ray-di/Ray.WebFormModule
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Check git history for VndError changes
git log --oneline -- src/Annotation/VndError.php | head -20Repository: ray-di/Ray.WebFormModule
Length of output: 139
The $href parameter should have a default value since it's not used in the handler.
The $href parameter is required but completely unused: VndErrorHandler::optionalAttribute() only processes message, path, and logref, and never returns href in the response body. Since the parameter provides no functional value, making it optional with an empty array default would reduce unnecessary coupling:
Add default for $href
public function __construct(
public string $message,
- public array $href,
+ public array $href = [],
public string|null $logref = null,
public string|null $path = null
) {
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function __construct( | |
| public string $message, | |
| public array $href, | |
| public string|null $logref = null, | |
| public string|null $path = null | |
| ) { | |
| } | |
| public function __construct( | |
| public string $message, | |
| public array $href = [], | |
| public string|null $logref = null, | |
| public string|null $path = null | |
| ) { | |
| } |
🤖 Prompt for AI Agents
In @src/Annotation/VndError.php around lines 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.
| use Aura\Input\FilterInterface; | ||
| use Doctrine\Common\Annotations\AnnotationReader; | ||
| use Doctrine\Common\Annotations\Reader; | ||
| use Koriym\Attributes\AttributeReader; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the file structure
git ls-files | grep -E "(AuraInputModule|composer)" | head -20Repository: ray-di/Ray.WebFormModule
Length of output: 135
🏁 Script executed:
#!/bin/bash
# Examine the AuraInputModule.php file
cat -n src/AuraInputModule.phpRepository: ray-di/Ray.WebFormModule
Length of output: 2236
🏁 Script executed:
#!/bin/bash
# Check composer.json for dependencies
cat composer.json | jq '.require, ."require-dev"' 2>/dev/null || cat composer.jsonRepository: ray-di/Ray.WebFormModule
Length of output: 282
🏁 Script executed:
#!/bin/bash
# Check if composer.lock exists and search for koriym/attributes
if [ -f composer.lock ]; then
jq '.packages[] | select(.name == "koriym/attributes") | {name, version}' composer.lock 2>/dev/null || grep -A5 "koriym/attributes" composer.lock
fiRepository: ray-di/Ray.WebFormModule
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check the actual ray/di package to see if it depends on koriym/attributes
# First, let's see if composer.lock exists and check ray/di entry
if [ -f composer.lock ]; then
echo "=== ray/di package info from composer.lock ==="
jq '.packages[] | select(.name == "ray/di") | {name, version, require}' composer.lock 2>/dev/null
echo ""
echo "=== All packages in lock file ==="
jq '.packages[].name' composer.lock 2>/dev/null | grep -i koriym
else
echo "composer.lock not found"
fiRepository: ray-di/Ray.WebFormModule
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for any reference to koriym/attributes in the entire repo
rg "koriym/attributes" --type php 2>/dev/null || echo "No references found"Repository: ray-di/Ray.WebFormModule
Length of output: 87
Add koriym/attributes as an explicit dependency in composer.json.
While the code changes correctly implement the PHP 8 attribute migration by binding Reader::class to AttributeReader::class, the koriym/attributes package is not declared as a direct or dev dependency in composer.json. Without this explicit dependency, the code will fail at runtime when attempting to load the AttributeReader class.
Add the following to the require section of composer.json:
"koriym/attributes": "*"
Or verify that ray/di ^2.16 explicitly includes koriym/attributes as a transitive dependency by checking its composer.json.
🤖 Prompt for AI Agents
In @src/AuraInputModule.php around 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.
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
2 participants
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
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.