Skip to content

Extract PropertyResolver strategy#1799

Open
henriquemoody wants to merge 2 commits into
Respect:mainfrom
henriquemoody:attributes
Open

Extract PropertyResolver strategy#1799
henriquemoody wants to merge 2 commits into
Respect:mainfrom
henriquemoody:attributes

Conversation

@henriquemoody

@henriquemoody henriquemoody commented Jun 25, 2026

Copy link
Copy Markdown
Member

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.

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

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

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.

@henriquemoody henriquemoody force-pushed the attributes branch 3 times, most recently from c856a35 to 37123a7 Compare June 25, 2026 15:11
@henriquemoody henriquemoody changed the title Add @var docblock-based array validation to Attributes Extract PropertyResolver strategy Jun 25, 2026
@henriquemoody henriquemoody force-pushed the attributes branch 2 times, most recently from eb82319 to b3734c1 Compare June 25, 2026 15:16
@henriquemoody henriquemoody requested a review from Copilot June 25, 2026 15:19
@henriquemoody henriquemoody marked this pull request as ready for review June 25, 2026 15:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/Validators/Attributes.php
Comment thread src/Validators/Attributes/PropertyResolver.php
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.
@henriquemoody henriquemoody requested a review from alganet June 25, 2026 15:38
@henriquemoody henriquemoody force-pushed the attributes branch 2 times, most recently from 191f36d to 88dfdfb Compare June 25, 2026 16:13
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants