update tests, and classes according to the interfaces of aura/filter-interface#80
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughModernize codebase to PHP 8.4 strict typing, replace Yoast PHPUnit polyfills with PHPUnit 11, change Filter::apply() to return a FilterResult, introduce immutable Filter\Failure and richer FailureCollection with dot-notation keys, and update Collection/Fieldset and tests to use FailuresInterface and typed signatures. ChangesStrict typing and failure interface refactoring
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Filter.php (1)
101-110:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix
Filter::apply()to handle array subjects without object property access
apply(array|object $values)calls$closure($values->$field, $values)on line 107; if$valuesis an array this will throw at runtime. Use array indexing for array inputs instead of->$field—and preserve reference behavior for sanitize closures that takefunction (&$value)so mutations apply to the returnedgetValues()result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Filter.php` around lines 101 - 110, Filter::apply currently uses object property access ($values->$field) which breaks when $values is an array and also prevents sanitize closures declared as function (&$value) from mutating the stored value; update Filter::apply to detect whether $values is an array or object, obtain the target field value by reference (e.g., $fieldValue =& $values[$field] for arrays or $fieldValue =& $values->$field for objects) before invoking the rule closure so $closure($fieldValue, $values) works for both types and preserves in-place mutations that will be visible to getValues(); keep existing failure handling via $this->failures and $this->proto_failures and only change the value retrieval/invocation logic around iterating $this->rules and calling each $closure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@composer.json`:
- Around line 19-22: composer.json declares "php": "^8.4" but the CI workflow
(continuous-integration.yml) still tests PHP 7.2–8.3 causing install failures;
update the GitHub Actions PHP matrix to only include PHP versions matching
composer.json (e.g., 8.4 and 8.5), removing any entries below 8.4, and ensure
any matrix variable name like matrix.php or the job step that runs composer
install reflects the new set so CI runs use versions compatible with the "php":
"^8.4" requirement.
- Line 21: The composer.json currently pins the dependency
"aura/filter-interface" to the unstable branch "dev-sub-filter"; update that
requirement to a stable tagged constraint such as "^3.0" or "3.0.0" in the
composer.json dependency list (replace "aura/filter-interface": "dev-sub-filter"
with the chosen stable constraint), then run composer update to lock the stable
version and run tests to verify compatibility with any code using
Aura\Filter_Interface.
In `@src/Fieldset.php`:
- Around line 75-77: The protected typed property $failures on class Fieldset is
uninitialized and causes a runtime error if getFailures() is called before
filter(); initialize it in the constructor to an empty FailureCollection (or
appropriate implementation of FailuresInterface) so $failures is always valid;
update the Fieldset::__construct to set $this->failures = new
FailureCollection() (or factory) and ensure any references in getFailures() and
filter() use that property safely.
In `@src/Filter.php`:
- Around line 56-60: The constructor currently accepts
?FailureCollectionInterface but silently discards any implementation that is not
FailureCollection; change Filter::__construct to either type-hint
?FailureCollection (narrow the parameter) or add a guard that throws a
TypeError/InvalidArgumentException if $failures is provided and not an instance
of FailureCollection to preserve caller state/behavior (reference
FailureCollectionInterface, FailureCollection, and Filter::__construct). For
apply, fix the mismatched array|object handling in Filter::apply by either
restricting the parameter to object only or implementing proper array access:
detect is_array($values) and use array_key_exists($field, $values) and
$values[$field] when appropriate (reference Filter::apply, the $values->$field
usage and $field) and update the signature/docblock accordingly so array access
no longer breaks.
---
Outside diff comments:
In `@src/Filter.php`:
- Around line 101-110: Filter::apply currently uses object property access
($values->$field) which breaks when $values is an array and also prevents
sanitize closures declared as function (&$value) from mutating the stored value;
update Filter::apply to detect whether $values is an array or object, obtain the
target field value by reference (e.g., $fieldValue =& $values[$field] for arrays
or $fieldValue =& $values->$field for objects) before invoking the rule closure
so $closure($fieldValue, $values) works for both types and preserves in-place
mutations that will be visible to getValues(); keep existing failure handling
via $this->failures and $this->proto_failures and only change the value
retrieval/invocation logic around iterating $this->rules and calling each
$closure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 427c84a1-bd2e-45bc-bf96-13c00063aa8e
📒 Files selected for processing (22)
composer.jsonsrc/Collection.phpsrc/CollectionIterator.phpsrc/Fieldset.phpsrc/Filter.phpsrc/Filter/Failure.phpsrc/Filter/FailureCollection.phptests/CollectionTest.phptests/ConfigurableFieldset.phptests/ConfigurableForm.phptests/Example/AddressFieldset.phptests/Example/ContactFilter.phptests/Example/ContactForm.phptests/Example/ExampleTest.phptests/Example/PhoneFieldset.phptests/Example/PhoneFilter.phptests/FieldTest.phptests/FieldsetTest.phptests/FilterTest.phptests/FormFactoryTest.phptests/FormTest.phptests/MockFieldset.php
| "php": "^7.2 || ^8.0", | ||
| "aura/filter-interface":"4.x-dev" | ||
| "php": "^8.4", | ||
| "aura/filter-interface": "dev-sub-filter" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check available versions/tags of aura/filter-interface
# Using GitHub API to check repository tags
gh api repos/auraphp/Aura.Filter_Interface/tags --jq '.[].name' | head -20Repository: auraphp/Aura.Input
Length of output: 81
Avoid the dev-sub-filter branch for aura/filter-interface
composer.json pins "aura/filter-interface": "dev-sub-filter", but auraphp/Aura.Filter_Interface has a stable tagged release (3.0.0). Switch to a stable constraint (e.g., ^3.0 / 3.0.0) unless there’s a documented reason the dev branch is required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@composer.json` at line 21, The composer.json currently pins the dependency
"aura/filter-interface" to the unstable branch "dev-sub-filter"; update that
requirement to a stable tagged constraint such as "^3.0" or "3.0.0" in the
composer.json dependency list (replace "aura/filter-interface": "dev-sub-filter"
with the chosen stable constraint), then run composer update to lock the stable
version and run tests to verify compatibility with any code using
Aura\Filter_Interface.
| * Failures aggregated after the last filter() call. | ||
| */ | ||
| protected $failures; | ||
| protected FailuresInterface $failures; |
There was a problem hiding this comment.
Uninitialized typed property will cause error if getFailures() is called before filter().
The $failures property is declared with type FailuresInterface but has no default value. It is only initialized in filter(). Calling getFailures() before filter() throws:
Typed property Aura\Input\Fieldset::$failures must not be accessed before initialization
Initialize the property to an empty FailureCollection to prevent this runtime error.
Proposed fix in constructor
public function __construct(
BuilderInterface $builder,
FilterInterface $filter,
$options = null
) {
$this->builder = $builder;
$this->filter = $filter;
$this->options = $options;
+ $this->failures = new FailureCollection();
$this->init();
}Also applies to: 392-395
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Fieldset.php` around lines 75 - 77, The protected typed property
$failures on class Fieldset is uninitialized and causes a runtime error if
getFailures() is called before filter(); initialize it in the constructor to an
empty FailureCollection (or appropriate implementation of FailuresInterface) so
$failures is always valid; update the Fieldset::__construct to set
$this->failures = new FailureCollection() (or factory) and ensure any references
in getFailures() and filter() use that property safely.
| public function __construct(?FailureCollectionInterface $failures = null) | ||
| { | ||
| if ($failures === null) { | ||
| $failures = new FailureCollection(); | ||
| } | ||
| $this->proto_failures = $failures; | ||
| $proto = ($failures instanceof FailureCollection) | ||
| ? $failures | ||
| : new FailureCollection(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and show the relevant constructor/apply code in src/Filter.php
if [ -f src/Filter.php ]; then
echo "=== src/Filter.php (around lines 40-120) ==="
nl -ba src/Filter.php | sed -n '40,130p'
else
echo "src/Filter.php not found"
exit 1
fi
# Find the FailureCollectionInterface definition
echo "=== Finding FailureCollectionInterface ==="
rg -n "interface FailureCollectionInterface" -S . || true
rg -n "FailureCollectionInterface" -S src tests . || true
# Find all concrete FailureCollection classes implementing/used
echo "=== Finding class FailureCollection ==="
rg -n "class FailureCollection" -S . || true
# Enumerate any other implementations of FailureCollectionInterface
echo "=== Searching for 'implements FailureCollectionInterface' ==="
rg -n "implements\s+FailureCollectionInterface" -S . || trueRepository: auraphp/Aura.Input
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ -f src/Filter.php ]; then
echo "=== src/Filter.php (around lines 40-130) ==="
# Print with line numbers using awk (nl not available)
awk 'NR>=40 && NR<=130 {printf "%5d:%s\n", NR, $0}' src/Filter.php
else
echo "src/Filter.php not found"
exit 1
fi
echo "=== Finding FailureCollectionInterface ==="
rg -n "interface FailureCollectionInterface" -S src tests . || true
rg -n "FailureCollectionInterface" -S src tests . || true
echo "=== Finding class FailureCollection ==="
rg -n "class FailureCollection\b" -S src tests . || true
echo "=== Searching for implementations ==="
rg -n "implements\s+FailureCollectionInterface" -S src tests . || trueRepository: auraphp/Aura.Input
Length of output: 6723
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check where FailureCollectionInterface is type-hinted/constructed
echo "=== Searching constructors/usages of FailureCollection/FailureCollectionInterface ==="
rg -n "new\s+FailureCollection\s*\(" -S src tests . || true
rg -n "\bFailureCollection\s*\(" -S src tests . || true
rg -n ":\s*\?FailureCollectionInterface\b|\b\?FailureCollectionInterface\b" -S src tests . || trueRepository: auraphp/Aura.Input
Length of output: 1102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check apply signature and array/object handling in Filter
echo "=== Searching Filter::apply implementation details ==="
rg -n "function\s+apply\s*\(" -S src/Filter.php src tests . || true
rg -n "\$values->" -S src/Filter.php || true
rg -n "\$values\[" -S src/Filter.php || true
# Also scan for any mention of $values being array vs object handling elsewhere
rg -n "is_array\(\$values\)|is_object\(\$values\)" -S src tests . || trueRepository: auraphp/Aura.Input
Length of output: 389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for calls to apply() ==="
rg -n "->apply\s*\(" -S src tests . || true
echo "=== Searching for Filter::apply usage by name ==="
rg -n "Filter::apply\s*\(" -S src tests . || true
echo "=== Searching for 'apply(' in context of Filter objects ==="
rg -n "function\s+filter|filter\(" -S src tests . || trueRepository: auraphp/Aura.Input
Length of output: 5016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for Filter::apply() call sites ==="
rg -n '\->apply\s*\(' -S src tests . || true
echo "=== Searching for ->apply( without requiring '->apply' tokenization issues ==="
rg -n 'apply\s*\(' -S src tests . || true
echo "=== Locate where Fieldset/Collection call Filter::apply ==="
rg -n 'getFilter\(\)|->getFilter\(\)|filter\(\)|Filter::apply|->apply' -S src/Fieldset.php src/Collection.php src/Form* src . || trueRepository: auraphp/Aura.Input
Length of output: 7121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== tests/FilterTest.php around apply($values) usages ==="
awk 'NR>=1 && NR<=240 {printf "%5d:%s\n", NR, $0}' tests/FilterTest.php | sed -n '40,220p'
echo "=== src/Fieldset.php around where apply($this) is called ==="
awk 'NR>=330 && NR<=420 {printf "%5d:%s\n", NR, $0}' src/Fieldset.php
echo "=== src/Filter.php around apply() implementation details ==="
awk 'NR>=80 && NR<=150 {printf "%5d:%s\n", NR, $0}' src/Filter.phpRepository: auraphp/Aura.Input
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== tests/FilterTest.php excerpt (1-220) ==="
awk 'NR>=1 && NR<=220 {print NR ":" $0}' tests/FilterTest.php
echo "=== src/Fieldset.php excerpt (330-410) ==="
awk 'NR>=330 && NR<=410 {print NR ":" $0}' src/Fieldset.php
echo "=== src/Filter.php excerpt (45-130) ==="
awk 'NR>=45 && NR<=130 {print NR ":" $0}' src/Filter.phpRepository: auraphp/Aura.Input
Length of output: 11622
🌐 Web query:
Aura\Filter_Interface\FailureCollectionInterface constructor expectation / intended usage in aura/filter-interface
💡 Result:
No constructor expectations apply because FailureCollectionInterface is an interface; it should not declare (and callers should not expect) any construct signature. In Aura’s design, the “failure collection” is produced via an implementation that implements FailureCollectionInterface, and user code should only rely on the interface’s methods. In PHP, interfaces describe behavior/contracts; they cannot be instantiated and therefore can’t express a constructor contract. As a general language/design rule, this is why you can’t specify a constructor on an interface and consumers should not assume anything about how an implementing class is constructed [1]. Within the Aura.Filter ecosystem, FailureCollectionInterface lives in the separate interface package (aura/filter-interface), which is meant to provide contracts for filter/failure collection behavior, not concrete instantiation details [2]. Practical intended usage: 1) Type-hint against Aura\Filter_Interface\FailureCollectionInterface (behavior contract). 2) Let Aura.Filter (or your DI container/factory wiring) instantiate the concrete implementation that implements that interface. 3) Do not call new FailureCollectionInterface(...) or rely on any specific constructor parameters; those belong to the concrete implementation class, not the interface [1].
Citations:
- 1: https://stackoverflow.com/questions/689474/why-are-we-not-allowed-to-specify-a-constructor-in-an-interface
- 2: https://github.com/auraphp/Aura.Filter_Interface
Don’t ignore non-FailureCollection implementations and don’t advertise array support in Filter::apply.
src/Filter.php(lines 56-64):__construct(?FailureCollectionInterface $failures)silently replaces any non-Aura\Input\Filter\FailureCollectionimplementation withnew FailureCollection(), discarding caller-provided behavior/state. Narrow the type to?FailureCollectionor fail fast when an unsupported implementation is passed.src/Filter.php(lines 101-112):apply(array|object $values)uses$values->$field(object property access), which will break for plain arrays despite thearray|objectsignature. Restrict the parameter type or implement correct array handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Filter.php` around lines 56 - 60, The constructor currently accepts
?FailureCollectionInterface but silently discards any implementation that is not
FailureCollection; change Filter::__construct to either type-hint
?FailureCollection (narrow the parameter) or add a guard that throws a
TypeError/InvalidArgumentException if $failures is provided and not an instance
of FailureCollection to preserve caller state/behavior (reference
FailureCollectionInterface, FailureCollection, and Filter::__construct). For
apply, fix the mismatched array|object handling in Filter::apply by either
restricting the parameter to object only or implementing proper array access:
detect is_array($values) and use array_key_exists($field, $values) and
$values[$field] when appropriate (reference Filter::apply, the $values->$field
usage and $field) and update the signature/docblock accordingly so array access
no longer breaks.
1 participant
Summary by CodeRabbit
Chores
New Features
Bug Fixes
Refactor
Documentation