Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughA new ChangesArray Input Type Validation & Nullable Union Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 37 minutes and 56 seconds.Comment |
koriym
force-pushed
the
codex/native-array-input-validation
branch
4 times, most recently
from
94fc1fc to
a7ba47c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Exception/InvalidInputTypeException.php (1)
21-22: 💤 Low valueConsider providing a descriptive exception message.
The exception is constructed with an empty message string. While the structured properties (
paramName,expectedType,actualType,itemKey) are useful for programmatic handling, an emptygetMessage()can complicate debugging and logging. Consider generating a human-readable message:💡 Suggested improvement
) { - parent::__construct('', $code, $previous); + $message = $itemKey !== null + ? sprintf('Invalid type for item "%s" of parameter "%s": expected %s, got %s', $itemKey, $paramName, $expectedType, $actualType) + : sprintf('Invalid type for parameter "%s": expected %s, got %s', $paramName, $expectedType, $actualType); + parent::__construct($message, $code, $previous); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Exception/InvalidInputTypeException.php` around lines 21 - 22, The InvalidInputTypeException constructor currently calls parent::__construct with an empty message; update the constructor in class InvalidInputTypeException to build a descriptive human-readable message (including paramName, expectedType, actualType and itemKey when present) and pass that message into parent::__construct so getMessage() returns useful debugging info while preserving $code and $previous; ensure the message is concise and conditionally includes itemKey if not null.src/InputQuery.php (1)
572-613: ⚖️ Poor tradeoffConsider extracting shared logic between
getArrayParameterValueandgetNullableArrayParameterValue.Both methods share significant structural overlap: checking for
itemclass, FileUpload type validation, callingcreateArrayOfInputs, and validating array types. This duplication could be reduced by extracting common logic into a helper, though the null-handling differences may justify keeping them separate for readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InputQuery.php` around lines 572 - 613, Both getArrayParameterValue and getNullableArrayParameterValue duplicate logic for resolving an "item" class, checking FileUpload types, calling createArrayOfInputs, and validating array types; extract that shared logic into a single private helper (e.g., resolveArrayParameterItems) that accepts the ReflectionParameter $param, the parameter name, the raw $arrayData (or nullable), and the InputAttributes instance to: 1) detect and instantiate the input attribute, 2) assert and return the created array of inputs via createArrayOfInputs when an item class is present (or signal to the caller that null was explicit), 3) validate non-item arrays and throw InvalidInputTypeException, and 4) perform the FileUploadTypeChecker::isFileUploadType check and throw InvalidFileUploadAttributeException when needed; then simplify getArrayParameterValue and getNullableArrayParameterValue to call this helper and handle only the small differences (nullable return vs default value).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Exception/InvalidInputTypeException.php`:
- Around line 21-22: The InvalidInputTypeException constructor currently calls
parent::__construct with an empty message; update the constructor in class
InvalidInputTypeException to build a descriptive human-readable message
(including paramName, expectedType, actualType and itemKey when present) and
pass that message into parent::__construct so getMessage() returns useful
debugging info while preserving $code and $previous; ensure the message is
concise and conditionally includes itemKey if not null.
In `@src/InputQuery.php`:
- Around line 572-613: Both getArrayParameterValue and
getNullableArrayParameterValue duplicate logic for resolving an "item" class,
checking FileUpload types, calling createArrayOfInputs, and validating array
types; extract that shared logic into a single private helper (e.g.,
resolveArrayParameterItems) that accepts the ReflectionParameter $param, the
parameter name, the raw $arrayData (or nullable), and the InputAttributes
instance to: 1) detect and instantiate the input attribute, 2) assert and return
the created array of inputs via createArrayOfInputs when an item class is
present (or signal to the caller that null was explicit), 3) validate non-item
arrays and throw InvalidInputTypeException, and 4) perform the
FileUploadTypeChecker::isFileUploadType check and throw
InvalidFileUploadAttributeException when needed; then simplify
getArrayParameterValue and getNullableArrayParameterValue to call this helper
and handle only the small differences (nullable return vs default value).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c80585a6-a36f-497a-b71e-d2d318269fca
📒 Files selected for processing (3)
src/Exception/InvalidInputTypeException.phpsrc/InputQuery.phptests/ArrayInputTest.php
koriym
force-pushed
the
codex/native-array-input-validation
branch
from
a7ba47c to
8314b1e
Compare
|
@coderabbitai I reviewed both nitpick comments.
Validation after the change:
Please close the review comments if this addresses your feedback. |
|
Tip For best results, initiate chat on the files or code changes.
Resolving the two review comments now. [resolve] ✏️ Learnings added
|
1 participant
Summary
#[Input] arrayvalues before invoking methods or DTO constructors#[Input(item: ...)] arrayinputsarray|nullinput parameters consistently with the same container validationWhy
Native
arrayinput DTO fields should be safe to use for request fields such astagIds. Before this change, scalar values could reach the PHP call boundary and fail asTypeError, or item-array inputs could silently become an empty array.Validation
php vendor/bin/phpunitcomposer csvendor/bin/phpstan analyse -c phpstan.neon/opt/homebrew/opt/php@8.4/bin/php vendor/bin/psalm --show-info=falseNote:
composer saunder PHP 8.5 currently crashes inside Psalm due PHP 8.5SplObjectStoragedeprecations. Psalm passes with PHP 8.4.Follow-up
Summary by CodeRabbit
New Features
Bug Fixes
Tests