Add validation constraints to configuration object #264
Conversation
There was a problem hiding this comment.
@daniruizcamacho thank you very much for your contribution (and I'm really sorry for not replying to you on the issue).
Could you please take a look on these comments? I think we can improve things a bit more 👍
They're more about delegating things to PHP so we don't need to worry to much on the library's side.
P.S.: your tests are great, I'm addressing the PHP 7.2 compatibility in #247.
src/Configuration.php
Outdated
| /** | ||
| * @var Constraint[]|null | ||
| */ | ||
| private $constraints; |
There was a problem hiding this comment.
We could configure the default value already here, so that the property would never be null.
There was a problem hiding this comment.
What do you think about renaming the property in order to align to the accessors' names (getter/setter) - rename to validationConstraints?
There was a problem hiding this comment.
Yes, totally agree. I will rename this var and the param in the setter function.
src/Configuration.php
Outdated
| return []; | ||
| } | ||
|
|
||
| return $this->constraints; |
There was a problem hiding this comment.
This would be simplified if we set the default value. Another way to simplify this method would be:
public function getValidationConstraints(): array
{
return $this->constraints ?? [];
}I think I'd have the former but am open to discuss possibilities here :)
There was a problem hiding this comment.
Yes, it's simpler. I use here the guard clauses pattern sometimes it's more clear. https://refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html
src/Configuration.php
Outdated
| return $this->constraints; | ||
| } | ||
|
|
||
| public function setValidationConstraints(array $constraints): void |
There was a problem hiding this comment.
I'd rather have a variadic function for this, which would leave this validation up to PHP. Something like:
public function setValidationConstraints(Constraint ...$constraints): void
{
$this->constraints = $constraints;
}There was a problem hiding this comment.
Very interesting. I never use variadic on this way. Thanks for the tip.
test/unit/ConfigurationTest.php
Outdated
| $validationConstraints = $config->getValidationConstraints(); | ||
|
|
||
| self::assertSame([], $validationConstraints); | ||
| self::assertNotSame($this->validationConstraints, $validationConstraints); |
There was a problem hiding this comment.
This assertion is not necessary since the one above already validates if the value is exactly [], so I'd remove it.
There was a problem hiding this comment.
Which would allow us to inline the variable $ValidationConstraints, since it has a single usage.
There was a problem hiding this comment.
Sorry, this is an error from my part.
test/unit/ConfigurationTest.php
Outdated
| * @uses \Lcobucci\JWT\Signer\None | ||
| * @uses \Lcobucci\JWT\Signer\Key | ||
| */ | ||
| public function getValidationConstraintsShouldReturnTheDefaultWhenItWasNotConfigured(): void |
There was a problem hiding this comment.
I'd suggest to rename this method to getValidationConstraintsShouldReturnAnEmptyArrayWhenItWasNotConfigured, since it makes our test intention a bit more explicit
test/unit/ConfigurationTest.php
Outdated
| */ | ||
| public function setValidationConstraintsShouldThrowExceptionWhenConstraintsArrayIsNotValid(): void | ||
| { | ||
| self::expectException(\InvalidArgumentException::class); |
There was a problem hiding this comment.
This would not be an InvalidArgumentException but rather a type error if we use the variadic form.
|
@lcobucci Don't worry about the issue, it's very difficult to have some time to answer all things. All comments are addressed. |
test/unit/ConfigurationTest.php
Outdated
| * @uses \Lcobucci\JWT\Signer\None | ||
| * @uses \Lcobucci\JWT\Signer\Key | ||
| */ | ||
| public function setValidationConstraintsShouldThrowExceptionWhenConstraintsArrayIsNotValid(): void |
There was a problem hiding this comment.
Since we're relying on PHP's validation I think that this test can be removed, the language will always enforce that requirement.
lcobucci
force-pushed
the
feature/addValidationConstraintsToConfigurationObject
branch
from
3311e2c to
627f436
Compare
This feature is to add validation constraints to configuration object. This feature is related to this opened issue. #204