Extract PropertyResolver strategy by henriquemoody · Pull Request #1799 · Respect/Validation

henriquemoody · 2026-06-25T05:45:35Z

Previously, the resolution of validators from property types and explicit new resolution strategies required editing the class, and a class-typed property annotated with #[Attributes] was validated twice — once by the explicit-attribute branch and again by the declared-type branch — which tripped the circular-reference guard on the second pass.

This commit moves that responsibility to a dedicated PropertyResolver interface with composable implementations, so each strategy can be developed and composed independently. The composite collapses duplicate Attributes entries, ensuring each property is validated exactly once even when multiple resolution paths produce the same instance. The Attributes class is reduced to its single responsibility, and resolver chains become pluggable through the constructor.

codecov · 2026-06-25T05:46:19Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.23%. Comparing base (42f08d7) to head (b2f7e6f).

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1799      +/-   ##
============================================
- Coverage     99.27%   99.23%   -0.04%     
- Complexity     1048     1058      +10     
============================================
  Files           197      200       +3     
  Lines          2466     2602     +136     
============================================
+ Hits           2448     2582     +134     
- Misses           18       20       +2

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR refactors Respect\Validation\Validators\Attributes by extracting “which validators apply to a property” into a dedicated PropertyResolver strategy interface with composable implementations, aiming to make resolution extensible and to avoid duplicate nested validation (notably around #[Attributes] + declared class types).

Changes:

Introduces PropertyResolver plus DeclaredTypePropertyResolver, ExplicitAttributePropertyResolver, and CompositePropertyResolver.
Updates Attributes to accept an injectable resolver chain and to delegate property-validator discovery to it.
Adds unit tests and new test stubs to cover the new resolution strategies and composite de-duplication behavior; updates docs and the dev lint command exclusions.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/unit/Validators/Attributes/ExplicitAttributePropertyResolverTest.php	Adds unit coverage for resolving explicit validator attributes from properties.
tests/unit/Validators/Attributes/DeclaredTypePropertyResolverTest.php	Adds unit coverage for resolving validators from declared property types (named/union/intersection).
tests/unit/Validators/Attributes/CompositePropertyResolverTest.php	Adds unit coverage for composing resolver strategies and collapsing duplicate `Attributes` instances.
tests/src/Stubs/WithUntypedProperty.php	Adds stub for “no declared type” property resolver behavior.
tests/src/Stubs/WithNoValidatorAttributes.php	Adds stub for properties with no validator attributes.
tests/src/Stubs/WithMixedProperty.php	Adds stub used by composite resolver tests.
tests/src/Stubs/WithExplicitStringTypeProperty.php	Adds stub to test explicit validator attribute resolution (`#[StringType]`).
tests/src/Stubs/WithExplicitAttributesAttributeProperty.php	Adds stub to test explicit `#[Attributes]` resolution returning the passed `Attributes` instance.
tests/src/Stubs/WithClassTypedProperty.php	Adds stub for declared class-typed property resolution.
tests/src/Stubs/StubPropertyResolver.php	Adds a test double implementing `PropertyResolver` for composite resolver tests.
src/Validators/Attributes/PropertyResolver.php	Introduces the new resolver interface used by `Attributes`.
src/Validators/Attributes/ExplicitAttributePropertyResolver.php	Implements resolution of explicit attribute-based validators on a property.
src/Validators/Attributes/DeclaredTypePropertyResolver.php	Implements resolution from declared property type (including union/intersection cases).
src/Validators/Attributes/CompositePropertyResolver.php	Composes multiple resolvers and collapses duplicate `$attributes` entries.
src/Validators/Attributes.php	Injects the resolver chain into `Attributes` and removes inline property-resolution logic.
src-dev/Commands/LintMixinCommand.php	Excludes the new `PropertyResolver` type from mixin lint generation.
docs/validators/Attributes.md	Documents the new constructor signature accepting a `PropertyResolver`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

alganet

I see the namespace as a blocker (a better choice outside of the Validators/ pool would make more sense).

The PropertyResolver interface is more a design question/refinement. If there's a deeper reason for it that can't be cleaned up, then it's fine.

alganet · 2026-06-25T21:25:51Z

+
+declare(strict_types=1);
+
+namespace Respect\Validation\Validators\Attributes;


I'm not sure about the namespacing.

Until now, we kept the Validators namespace containing exclusively Validator instances.

The Fluent family even builds on this convention, scanning folders for classes that it assumes are all the same type within a namespace (it has an interface check, but still).

The way I see, the resolvers are not validators and should instead be somewhere else.

I'm fine to move it somewhere else, but they're not generic. They're very Attributes specific.

I would have been happier if that wasn't the case, but dealing with the cycle here was the path I found. Maybe we could pass the WeakMap and a $depth here, but I struggle to visualize this working now (without testing it first)

Am I missing something? Does the namespace declaration has any role in the recursion detection?

I'm discussing that namespace Respect\Validation\Validators\Attributes; is a resolve-mechanism inside the bucket that previously only held Validators. Maybe namespace Respect\Validation\Attributes; would fit better.

alganet · 2026-06-25T21:44:42Z

+interface PropertyResolver
+{
+    /** @return array<Validator> */
+    public function resolve(ReflectionProperty $property, Attributes $attributes): array;


I'm not sure about Attributes being here.

Feels like juggling around the instance so you can return it back.

From the perspective of the interface itself, a cleaner contract would be "just return me validators for this property", and Attributes is not one of them.

Maybe I missed something though.

Clarifying further, maybe:

public function resolve(ReflectionProperty $property, string $type): array;

Would be the best interface, passing Validator::class as a class-string to $type.

In the future, I see this as possibly a component reuse for Respect\Data (entity attribute resolution). This is, however, for a future generalization and out of scope. But hopefully the direction clarifies why I'm picking on the interface!

This is precisely how we prevent the circular cycle since we have Attributes being called recursively. That's why it's in the Attributes namespace, it's really specific to this validator.

That is an important implementation detail! Makes sense. Would it be possible to make that without declaring it in the resolve signature though?

"Get me these validators, but don't recurse on this one specifically" feels like a weird abstraction that gives away information about the underlying implementation, and instead better handled somewhere else.

Maybe you already tried. If that's the case, I can take a deeper look with fresh eyes to see if I can come up with something.

You might be getting into something... We could pass a key => value array with the string-class => Validator and replace them dynamically. It might make it even more robust actually

Previously, the resolution of validators from property types and explicit new resolution strategies required editing the class, and a class-typed property annotated with #[Attributes] was validated twice — once by the explicit-attribute branch and again by the declared-type branch — which tripped the circular-reference guard on the second pass. This commit moves that responsibility to a dedicated PropertyResolver interface with composable implementations, so each strategy can be developed and composed independently. The composite collapses duplicate Attributes entries, ensuring each property is validated exactly once even when multiple resolution paths produce the same instance. The Attributes class is reduced to its single responsibility, and resolver chains become pluggable through the constructor.

The circular-reference guard keyed visited objects by spl_object_id(), which is recycled once an object is garbage-collected. On a reused validator instance this caused two problems: unbounded growth of the visited map across calls, and false circular-reference failures when a fresh object inherited a freed ID. WeakMap is keyed by object identity, so distinct objects can never collide, and entries are reclaimed automatically when the key object is garbage-collected, no manual reset is required. Since WeakMap is not serializable, __sleep excludes the field and __wakeup reinitializes it, which also avoids carrying stale IDs across unserialize.

henriquemoody · 2026-06-26T14:43:49Z

+    private readonly PropertyResolver $propertyResolver;
+
+    public function __construct(
+        PropertyResolver|null $propertyResolver = null,


@alganet because of this new injection, I would like to only release version 3.2 after this has been merged. However, there are a few things that are not optimal with this.

For starters, I think we should have a resolver that works on the class level rather than the property level because that will allow us to also use doc blocks in the future. It would then make sense that once of those class resolver take care of the properties by itself, so we don't need to call any PropertyResolver here.

Are you worried about breaking BC on the parameter signature?

For me, Attributes is parameter-less. We could document that the parameter is internal and subject to change, and only say it's public when it's done.

We need to do more of that (what is internal vs what is public) so we can have more freedom. Not all mechanisms in validation are public API surface.

The resolver you mention is a subsystem of Validation that is starting to couple cache ideas, docblock scanning and so on. It has now become more than a validator.

I think this subsystem deserves a careful design, and it's likely to take a lot of time, so I'm concerned about blocking release of other features while we do it. Perhaps there's a way we can deliver recursive attributes (lower tier improvement) before going full-feature.

henriquemoody · 2026-06-26T14:44:28Z

+use ReflectionFunctionAbstract;
+use Respect\Parameter\Resolver;
+
+final class BypassResolver implements Resolver


I created this one here quickly, just to signal the use-case, but this should be in Respect\Parameter

henriquemoody · 2026-06-26T14:50:04Z

+final class ExplicitAttributePropertyResolver implements PropertyResolver
+{
+    public function __construct(
+        private readonly Resolver $resolver,


@alganet I'm wondering if we should pass the factory here instead of the resolver.

Part of me thinks it would be a big overhead, but if we want to do something like I mentioned in Parameter we will need somewhere to mediate between the Resolver and the creation of each validator.

Another part of me want to keep only the Resolver here, to make things more simpler at the cost of making things inflexible.

I still don't understand the real use case for that. I mean, I get the Animal -> Goat, Cat analogy but I don't get why we need that in Validation.

henriquemoody marked this pull request as draft June 25, 2026 05:46

henriquemoody force-pushed the attributes branch from db23275 to 18a3c72 Compare June 25, 2026 07:23

henriquemoody mentioned this pull request Jun 25, 2026

Rename Attributes to something else #1800

Open

henriquemoody force-pushed the attributes branch 3 times, most recently from c856a35 to 37123a7 Compare June 25, 2026 15:11

henriquemoody changed the title ~~Add @var docblock-based array validation to Attributes~~ Extract PropertyResolver strategy Jun 25, 2026

henriquemoody force-pushed the attributes branch 2 times, most recently from eb82319 to b3734c1 Compare June 25, 2026 15:16

henriquemoody requested a review from Copilot June 25, 2026 15:19

Copilot started reviewing on behalf of henriquemoody June 25, 2026 15:19 View session

henriquemoody marked this pull request as ready for review June 25, 2026 15:21

Copilot AI reviewed Jun 25, 2026

View reviewed changes

Comment thread src/Validators/Attributes.php

Comment thread src/Validators/Attributes/PropertyResolver.php

henriquemoody force-pushed the attributes branch 2 times, most recently from 1ecb9b5 to f2ab556 Compare June 25, 2026 15:29

henriquemoody requested a review from alganet June 25, 2026 15:38

henriquemoody force-pushed the attributes branch 3 times, most recently from 88dfdfb to b2f7e6f Compare June 25, 2026 16:14

alganet requested changes Jun 25, 2026

View reviewed changes

henriquemoody added 2 commits June 26, 2026 16:38

henriquemoody force-pushed the attributes branch from b2f7e6f to 2365fb8 Compare June 26, 2026 14:39

henriquemoody commented Jun 26, 2026

View reviewed changes


		declare(strict_types=1);

		namespace Respect\Validation\Validators\Attributes;

Uh oh!

Conversation

henriquemoody commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

alganet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alganet Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

henriquemoody commented Jun 25, 2026 •

edited

Loading

codecov Bot commented Jun 25, 2026 •

edited

Loading

alganet Jun 25, 2026 •

edited

Loading