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 |
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.
![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.
src/AuraInputModule.php
Outdated
| 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
|
|
|
先日の対応（アノテーションからアトリビュートに移行）に加えて以下を対応
|
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) {
| /** | ||
| * Returns all failure messages for all fields. | ||
| * | ||
| * @return array | ||
| * @return list<string> | ||
| */ | ||
| public function getFailureMessages() | ||
| public function getFailureMessages(): array | ||
| { | ||
| return $this->filter->getFailures()->getMessages(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the AbstractForm.php file
find . -name "AbstractForm.php" -type fRepository: ray-di/Ray.WebFormModule
Length of output: 90
🏁 Script executed:
# Check if there's a composer.json to understand dependencies
cat composer.json 2>/dev/null | head -50Repository: ray-di/Ray.WebFormModule
Length of output: 1190
🏁 Script executed:
# Search for references to Aura Filter in the codebase
rg "Aura\\\\Filter" --type php -A 3Repository: ray-di/Ray.WebFormModule
Length of output: 732
🏁 Script executed:
# Let's examine the AbstractForm.php file, particularly around lines 159-167
cat -n ./src/AbstractForm.php | sed -n '150,175p'Repository: ray-di/Ray.WebFormModule
Length of output: 828
🏁 Script executed:
# Also, let's see the full context of the class to understand $this->filter
head -50 ./src/AbstractForm.phpRepository: ray-di/Ray.WebFormModule
Length of output: 1125
🏁 Script executed:
# Search for documentation or type hints related to SubjectFilter and getFailures()
rg "getFailures|getMessages" --type php -B 2 -A 5Repository: ray-di/Ray.WebFormModule
Length of output: 986
🏁 Script executed:
# Let's look for the complete context where errorMessages is used to understand the structure better
cat -n ./src/AbstractForm.php | sed -n '120,150p'Repository: ray-di/Ray.WebFormModule
Length of output: 1015
🏁 Script executed:
# Search for vendor/aura/filter to see if we can find the actual implementation
find . -path "*/vendor/aura/filter*" -type d 2>/dev/null | head -5Repository: ray-di/Ray.WebFormModule
Length of output: 50
🏁 Script executed:
# Let's check if there's any test file that shows the expected return type
rg "getFailureMessages|getMessages" --type php -B 5 -A 10 tests/Repository: ray-di/Ray.WebFormModule
Length of output: 50
🌐 Web query:
Aura Filter getMessages() return type documentation
💡 Result:
In Aura.Filter 2.x, SubjectFilter::getFailures() returns a FailureCollection (described as “essentially an ArrayObject keyed on the field names”). Each field entry contains an array of Failure objects, where each Failure has getMessage() (the failure message). The FailureCollection::getMessages() method is used to extract those into a messages array. [1]
So the effective return type of FailureCollection::getMessages() is:
array<string, array<int, string>>(aka “array keyed by field name, each value an array of message strings”). This is the same “shape” shown in the Aura docs’getMessages()examples (field => list of messages). [2]
Sources:
[1] Aura.Filter 2.x “Filtering Arrays and Objects” (FailureCollection + getMessages() usage) https://auraphp.com/packages/2.x/Filter/subject-filter.html
[2] Aura framework validation docs showing getMessages() output structure https://auraphp.com/framework/2.x/en/validation/
🌐 Web query:
Aura Filter FailureCollection getMessages() PHP
💡 Result:
In Aura.Filter 2.2+, SubjectFilter::getMessages() was removed and replaced by failures reporting via getFailures() (a FailureCollection). (php.libhunt.com)
Use:
$success = $filter->apply($subject);
if (! $success) {
$failures = $filter->getFailures(); // FailureCollection
$messages = $failures->getMessages(); // array of messages (keyed by field)
}This is the pattern shown in the Aura.Filter docs; getFailures() returns a FailureCollection keyed by field name, and getMessages() gives you the failure messages. (auraphp.com)
Citations:
- 1: https://php.libhunt.com/aura-filter-changelog?utm_source=openai
- 2: https://auraphp.com/packages/2.x/Filter/subject-filter.html?utm_source=openai
Incorrect @return docblock — getMessages() returns an associative array, not list<string>.
Aura\Filter\SubjectFilter::getFailures()->getMessages() returns an array keyed by field name with arrays of message strings. The list<string> annotation is incorrect.
Proposed fix
- * `@return` list<string>
+ * `@return` array<string, array<string>>📝 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.
| /** | |
| * Returns all failure messages for all fields. | |
| * | |
| * @return array | |
| * @return list<string> | |
| */ | |
| public function getFailureMessages() | |
| public function getFailureMessages(): array | |
| { | |
| return $this->filter->getFailures()->getMessages(); | |
| } | |
| /** | |
| * Returns all failure messages for all fields. | |
| * | |
| * `@return` array<string, array<string>> | |
| */ | |
| public function getFailureMessages(): array | |
| { | |
| return $this->filter->getFailures()->getMessages(); | |
| } |
🤖 Prompt for AI Agents
In `@src/AbstractForm.php` around lines 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.
| public function setField(Fieldset $fieldset): void | ||
| { | ||
| $fieldset->setField(self::TOKEN_KEY, 'hidden') | ||
| ->setAttribs(['value' => $this->getToken()]); | ||
| ->setAttribs(['value' => $this->getToken()]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for setAttribs usage
echo "=== Usage of setAttribs ==="
rg -n 'setAttribs' --type php -C 2
echo ""
echo "=== Usage of setValue ==="
rg -n '->setValue' --type php -C 2
echo ""
echo "=== Looking for FakeAntiCsrf ==="
find . -name "*FakeAntiCsrf*" -o -name "*fake*anti*" -type f | head -20
echo ""
echo "=== Checking composer/vendor structure ==="
ls -la vendor/ 2>/dev/null | head -20 || echo "vendor dir not available"Repository: ray-di/Ray.WebFormModule
Length of output: 3041
🏁 Script executed:
#!/bin/bash
# Read FakeAntiCsrf to see old implementation
echo "=== FakeAntiCsrf.php content ==="
cat -n tests/Fake/FakeAntiCsrf.php
echo ""
echo "=== Search for setValue (fixing the pattern) ==="
rg 'setValue' --type php -C 2
echo ""
echo "=== Check if there are any differences in src/AntiCsrf.php ==="
cat -n src/AntiCsrf.phpRepository: ray-di/Ray.WebFormModule
Length of output: 2867
🏁 Script executed:
#!/bin/bash
# Search for getValue calls
echo "=== getValue usage ==="
rg 'getValue' --type php -C 2
echo ""
echo "=== Tests related to CSRF ==="
find . -name "*test*" -o -name "*Test*" | grep -i csrf
echo ""
echo "=== Search for test files that test AntiCsrf ==="
rg 'AntiCsrf' tests/ --type php -l
echo ""
echo "=== Look for Aura.Input in composer.json or dependencies ==="
cat composer.json | grep -A 10 -B 10 "aura"Repository: ray-di/Ray.WebFormModule
Length of output: 2388
🏁 Script executed:
#!/bin/bash
# Read the full AntiCsrfTest.php
echo "=== AntiCsrfTest.php ==="
cat -n tests/AntiCsrfTest.php
echo ""
echo "=== Check AbstractFormTest for field value behavior ==="
cat -n tests/AbstractFormTest.php | head -100Repository: ray-di/Ray.WebFormModule
Length of output: 5592
🏁 Script executed:
#!/bin/bash
# Look for tests that might render or submit the CSRF field
echo "=== Search for tests that render forms or check HTML output ==="
rg 'render|html|output' tests/ --type php -i -l
echo ""
echo "=== Check AbstractAuraFormTest ==="
cat -n tests/AbstractAuraFormTest.php | head -100Repository: ray-di/Ray.WebFormModule
Length of output: 2930
🏁 Script executed:
#!/bin/bash
# Check FormFactory
echo "=== FormFactory.php ==="
cat -n tests/Fake/FormFactory.php
echo ""
echo "=== Check if there are any recent changes or comments about setAttribs vs setValue ==="
git log --all --oneline --grep="setAttribs\|setValue" 2>/dev/null | head -20 || echo "Git log not available"
echo ""
echo "=== Search for any documentation or comments about field value setting ==="
rg 'setAttribs|setValue' . -C 3 --type php | grep -i "comment\|todo\|fixme\|note" -C 3Repository: ray-di/Ray.WebFormModule
Length of output: 331
🏁 Script executed:
#!/bin/bash
# Find FormFactory
echo "=== Find FormFactory ==="
find . -name "FormFactory*" -type f
echo ""
echo "=== Find all test files ==="
ls -la tests/
echo ""
echo "=== Find all files in tests/Fake ==="
ls -la tests/Fake/Repository: ray-di/Ray.WebFormModule
Length of output: 2244
🏁 Script executed:
#!/bin/bash
# Look at the test that renders CSRF with FakeAntiCsrf
echo "=== Check how form rendering works with FakeForm ==="
cat -n tests/Fake/FakeForm.php
echo ""
echo "=== Check if AntiCsrf is tested with rendering ==="
rg 'AntiCsrf' tests/ --type php -A 5 | grep -E "(form|render|html|output)" -B 2 -A 2
echo ""
echo "=== Check if there's a way to verify setAttribs produces correct hidden field HTML ==="
rg 'hidden' tests/ --type php -B 2 -A 2Repository: ray-di/Ray.WebFormModule
Length of output: 3150
🏁 Script executed:
#!/bin/bash
# Let me verify if there's a test that specifically checks AntiCsrf rendering
echo "=== Check AntiCsrf constructor to see if test is calling it correctly ==="
cat -n src/AntiCsrf.php | head -40
echo ""
echo "=== Check the test again - AntiCsrfTest constructor call ==="
cat -n tests/AntiCsrfTest.php | grep -A 10 "setUp"
echo ""
echo "=== Let me search for what methods are actually available on Aura Input field ==="
rg 'class.*Field|function.*get|function.*set' --type php -A 1 | head -50Repository: ray-di/Ray.WebFormModule
Length of output: 3970
🏁 Script executed:
#!/bin/bash
# Let me verify if getValue() is ever called on CSRF field or if form submission works with setAttribs
echo "=== Check if getValue is called anywhere in form processing ==="
rg 'getValue' tests/ src/ --type php
echo ""
echo "=== Check AbstractForm to see how fields are accessed ==="
cat -n src/AbstractForm.php | head -80Repository: ray-di/Ray.WebFormModule
Length of output: 2882
🏁 Script executed:
#!/bin/bash
# Continue reading AbstractForm
echo "=== Continue AbstractForm ==="
cat -n src/AbstractForm.php | tail -100
echo ""
echo "=== Check what the test expects - does it test AntiCsrf with actual rendering? ==="
grep -n "AntiCsrf" tests/AbstractAuraFormTest.php -A 10 -B 2Repository: ray-di/Ray.WebFormModule
Length of output: 3969
Add test coverage to verify AntiCsrf.setField() renders the CSRF token field correctly.
The migration from setValue() (in FakeAntiCsrf) to setAttribs(['value' => ...]) is a real API difference. While form submission will capture the HTML attribute value regardless of the internal field value tracking, the existing test testAntiCsrfForm() only validates the old FakeAntiCsrf implementation with setValue(). The production AntiCsrf class uses setAttribs() but has no corresponding render test — testSetField() only verifies the method returns null, not the actual field state. Since all other form fields in the codebase use setAttribs() for values, this pattern is consistent, but explicit test coverage confirming the rendered HTML output matches expectations (as testAntiCsrfForm() does for FakeAntiCsrf) would validate the change.
🤖 Prompt for AI Agents
In `@src/AntiCsrf.php` around lines 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.
| use LogicException; | ||
|
|
||
| class InvalidOnFailureMethod extends LogicException |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Current package's LogicException ==="
fd 'LogicException.php' src/ -x cat {}
echo ""
echo "=== ExceptionInterface usage throughout codebase ==="
rg 'ExceptionInterface' --type=php -n
echo ""
echo "=== All exception classes and their base classes ==="
rg 'class.*Exception.*extends' --type=php -n
echo ""
echo "=== Check for catch statements catching exceptions ==="
rg 'catch\s*\(' --type=php -A2Repository: ray-di/Ray.WebFormModule
Length of output: 2683
🏁 Script executed:
#!/bin/bash
echo "=== Current InvalidOnFailureMethod.php content ==="
cat -n src/Exception/InvalidOnFailureMethod.php
echo ""
echo "=== Check git history for this file ==="
git log --oneline -n 10 -- src/Exception/InvalidOnFailureMethod.php
echo ""
echo "=== Show git diff for recent changes to this file ==="
git log -p -n 2 -- src/Exception/InvalidOnFailureMethod.php | head -100Repository: ray-di/Ray.WebFormModule
Length of output: 686
🏁 Script executed:
#!/bin/bash
echo "=== Full git show for the refactor commit ==="
git show dfafacb -- src/Exception/InvalidOnFailureMethod.php
echo ""
echo "=== Check if other exception files in same namespace use similar pattern ==="
cat -n src/Exception/InvalidFormPropertyException.phpRepository: ray-di/Ray.WebFormModule
Length of output: 1301
Missing ExceptionInterface implementation: extends \LogicException directly without implementing the contract.
InvalidOnFailureMethod uses use LogicException; which imports PHP's built-in \LogicException (not the package's LogicException), and extends it without implementing ExceptionInterface. This is inconsistent with the package's exception pattern: RuntimeException and InvalidArgumentException both explicitly implement ExceptionInterface when extending PHP's built-in exception classes.
Either remove the use LogicException; statement to extend the package-local LogicException (which implements ExceptionInterface transitively), or add implements ExceptionInterface to this class.
Note: InvalidFormPropertyException has the same issue.
🤖 Prompt for AI Agents
In `@src/Exception/InvalidOnFailureMethod.php` around lines 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.
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
Refactor
Tests
Documentation