Extract PropertyResolver strategy #1799
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| <?php | ||
|
|
||
| namespace Respect\Validation\Validators\Attributes; | ||
|
|
||
| use ReflectionFunctionAbstract; | ||
| use Respect\Parameter\Resolver; | ||
|
|
||
| final class BypassResolver implements Resolver | ||
|
Member
Author
There was a problem hiding this comment. I created this one here quickly, just to signal the use-case, but this should be in |
||
| { | ||
| public function resolve(ReflectionFunctionAbstract $reflection, array $arguments): array | ||
| { | ||
| return $arguments; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * SPDX-License-Identifier: MIT | ||
| * SPDX-FileCopyrightText: (c) Respect Project Contributors | ||
| * SPDX-FileContributor: Henrique Moody <henriquemoody@gmail.com> | ||
| */ | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Respect\Validation\Validators\Attributes; | ||
|
|
||
| use ReflectionProperty; | ||
| use Respect\Validation\Validator; | ||
| use Respect\Validation\Validators\Attributes; | ||
|
|
||
| use function array_values; | ||
| use function in_array; | ||
|
|
||
| final class CompositePropertyResolver implements PropertyResolver | ||
| { | ||
| /** @var list<PropertyResolver> */ | ||
| private readonly array $resolvers; | ||
|
|
||
| public function __construct(PropertyResolver ...$resolvers) | ||
| { | ||
| $this->resolvers = array_values($resolvers); | ||
| } | ||
|
|
||
| /** @return array<Validator> */ | ||
| public function resolve(ReflectionProperty $property, Attributes $attributes): array | ||
| { | ||
| $accumulated = []; | ||
|
|
||
| foreach ($this->resolvers as $resolver) { | ||
| $validators = $resolver->resolve($property, $attributes); | ||
| if ($validators === []) { | ||
| continue; | ||
| } | ||
|
|
||
| // When more than one resolver recognizes the same property (e.g. a | ||
| // class-typed property annotated with #[Attributes], which both | ||
| // DeclaredTypePropertyResolver and ExplicitAttributePropertyResolver | ||
| // emit `$attributes` for), collapse duplicate `$attributes` entries | ||
| // so the nested Attributes validator runs once instead of tripping | ||
| // its circular-reference guard on the second pass. | ||
| foreach ($validators as $validator) { | ||
| if ($validator === $attributes && in_array($validator, $accumulated, true)) { | ||
| continue; | ||
| } | ||
|
|
||
| $accumulated[] = $validator; | ||
| } | ||
| } | ||
|
|
||
| return $accumulated; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * SPDX-License-Identifier: MIT | ||
| * SPDX-FileCopyrightText: (c) Respect Project Contributors | ||
| * SPDX-FileContributor: Henrique Moody <henriquemoody@gmail.com> | ||
| */ | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Respect\Validation\Validators\Attributes; | ||
|
Member
There was a problem hiding this comment. I'm not sure about the namespacing. Until now, we kept the The The way I see, the resolvers are not validators and should instead be somewhere else.
Member
Author
There was a problem hiding this comment. I'm fine to move it somewhere else, but they're not generic. They're very 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
Member
There was a problem hiding this comment. Am I missing something? Does the namespace declaration has any role in the recursion detection? I'm discussing that |
||
|
|
||
| use ReflectionIntersectionType; | ||
| use ReflectionNamedType; | ||
| use ReflectionProperty; | ||
| use ReflectionUnionType; | ||
| use Respect\Validation\Validator; | ||
| use Respect\Validation\Validators\Attributes; | ||
| use Respect\Validation\Validators\Given; | ||
| use Respect\Validation\Validators\Instance; | ||
|
|
||
| final class DeclaredTypePropertyResolver implements PropertyResolver | ||
| { | ||
| /** @return array<Validator> */ | ||
| public function resolve(ReflectionProperty $property, Attributes $attributes): array | ||
| { | ||
| $type = $property->getType(); | ||
|
|
||
| if ($type instanceof ReflectionNamedType) { | ||
| if ($type->isBuiltin()) { | ||
| return []; | ||
| } | ||
|
|
||
| return [$attributes]; | ||
| } | ||
|
|
||
| if ($type instanceof ReflectionIntersectionType) { | ||
| return [$attributes]; | ||
| } | ||
|
|
||
| if ($type instanceof ReflectionUnionType) { | ||
| $validators = []; | ||
| foreach ($type->getTypes() as $innerType) { | ||
| if (!$innerType instanceof ReflectionNamedType || $innerType->isBuiltin()) { | ||
| continue; | ||
| } | ||
|
|
||
| /** @var class-string $class */ | ||
| $class = $innerType->getName(); | ||
| $validators[] = new Given(new Instance($class), $attributes); | ||
| } | ||
|
|
||
| return $validators; | ||
| } | ||
|
|
||
| return []; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * SPDX-License-Identifier: MIT | ||
| * SPDX-FileCopyrightText: (c) Respect Project Contributors | ||
| * SPDX-FileContributor: Henrique Moody <henriquemoody@gmail.com> | ||
| */ | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Respect\Validation\Validators\Attributes; | ||
|
|
||
| use ReflectionAttribute; | ||
| use ReflectionClass; | ||
| use ReflectionProperty; | ||
| use Respect\Parameter\Resolver; | ||
| use Respect\Validation\Validator; | ||
| use Respect\Validation\Validators\Attributes; | ||
|
|
||
| final class ExplicitAttributePropertyResolver implements PropertyResolver | ||
| { | ||
| public function __construct( | ||
| private readonly Resolver $resolver, | ||
|
Member
Author
There was a problem hiding this comment. @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.
Member
There was a problem hiding this comment. I still don't understand the real use case for that. I mean, I get the |
||
| ) { | ||
| } | ||
|
|
||
| /** @return array<Validator> */ | ||
| public function resolve(ReflectionProperty $property, Attributes $attributes): array | ||
| { | ||
| $validators = []; | ||
| foreach ($property->getAttributes(Validator::class, ReflectionAttribute::IS_INSTANCEOF) as $attribute) { | ||
| $propertyValidator = $this->toValidator($attribute, $attributes); | ||
| $validators[] = $propertyValidator; | ||
| } | ||
|
|
||
| return $validators; | ||
| } | ||
|
|
||
| private function toValidator(ReflectionAttribute $attribute, Attributes $attributes): Validator | ||
| { | ||
| /** @var class-string<Validator> $name */ | ||
| $name = $attribute->getName(); | ||
| if ($name === Attributes::class) { | ||
| return $attributes; | ||
| } | ||
|
|
||
| $reflection = new ReflectionClass($name); | ||
| $constructor = $reflection->getConstructor(); | ||
| if ($constructor === null) { | ||
| return $attribute->newInstance(); | ||
| } | ||
|
|
||
| return $reflection->newInstanceArgs($this->resolver->resolve($constructor, $attribute->getArguments())); | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you worried about breaking BC on the parameter signature?
For me,
Attributesis 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.