8000 [Validator] Access container array in Expression/Context by Engerim · Pull Request #23134 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Validator] Access container array in Expression/Context #23134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 11 commits into from
Closed

[Validator] Access container array in Expression/Context #23134

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2aa3e7c
Fix #12315
Jun 11, 2017
c6d83d5
Fix #12315
Jun 11, 2017
9e0882b
* fix cs
Jun 11, 2017
c964fcb
* fix issue with test lowest
Jun 11, 2017
53016a5
Fix #12315
Jun 19, 2017
1907d8b
Fix #12315
Jun 20, 2017
209c386
Merge branch '3.4' into fix-issue-12315
Engerim Aug 18, 2017
8000 afd2406
Fix #12315
Oct 9, 2017
26f4505
Fix #12315
Oct 9, 2017
bd3abc9
Fix #12315
Oct 9, 2017
5c849fe
Fix #12315
Oct 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Symfony/Component/Validator/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ CHANGELOG
the `Url::CHECK_DNS_TYPE_*` constants values and will throw an exception in Symfony 4.0
* added min/max amount of pixels check to `Image` constraint via `minPixels` and `maxPixels`
* added a new "propertyPath" option to comparison constraints in order to get the value to compare from an array or object
* added a `dataPath` option to the `Expression` constraint to allow an other way to get "this" from the context
Copy link
Member

Choose a reason for hiding this comment

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

another

Copy link
Member

Choose a reason for hiding this comment

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

"this" should be enclosed with backticks


3.3.0
-----
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/Validator/Constraints/Expression.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class Expression extends Constraint

public $message = 'This value is not valid.';
public $expression;
public $dataPath;

/**
* {@inheritdoc}
Expand Down
14 changes: 14 additions & 0 deletions src/Symfony/Component/Validator/Constraints/ExpressionValidator.php
View file
Open in desktop
6D40
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\Validator\Constraints;

use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Component\Validator\Constraint;
use Symfony\Component\Validator\ConstraintValidator;
use Symfony\Component\Validator\Exception\RuntimeException;
Expand Down Expand Up @@ -46,6 +47,10 @@ public function validate($value, Constraint $constraint)
$variables['value'] = $value;
$variables['this'] = $this->context->getObject();
Copy link
Member

Choose a reason for hiding this comment

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

to improve readability I would make this line an else branch of the if below


if (null !== $constraint->dataPath && '' !== $constraint->dataPath) {
Copy link
Member

Choose a reason for hiding this comment

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

if ($constraint->dataPath) { should be better

$variables['this'] = $this->getPropertyAccessor()->getValue($this->context, $constraint->dataPath);
Copy link
Member

Choose a reason for hiding this comment

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

if $variables['this'] may be overridden I suggest move $variables['this'] = $this->context->getObject(); to else statement?

Copy link
Member

Choose a reason for hiding this comment

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

In the other hand, if Symfony deciders decides to make it configurable I think we could start the path from the root i.e.:

$this->getPropertyAccessor()->getValue($this->context->getRoot(), $constraint->dataPath);

else, I suggest pass the root value to this directly if it's null i.e::

if (null === $variables['this'] = $this->context->getObject()) {
    $variables['this'] = $this->context->getRoot();
}

thus, you don't have to worry about the "dataPath", you know the data linked to the root, therefore you just have to access them:

new Expression(array('expression' => 'value <= this["data"]["dateEnd"]'));

wdyt?

< 8000 details-menu class="dropdown-menu dropdown-menu-sw show-more-popover color-fg-default" style="width:185px" src="" preload > Copy link
Contributor Author
@Engerim Engerim Jun 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I like the idea, to pass the root value directly to this if it's null. No dataPath and so easier to use. I will wait to change it because I would like to know how the deciders think about this.

}

if (!$this->getExpressionLanguage()->evaluate($constraint->expression, $variables)) {
$this->context->buildViolation($constraint->message)
->setParameter('{{ value }}', $this->formatValue($value))
Expand All @@ -65,4 +70,13 @@ private function getExpressionLanguage()

return $this->expressionLanguage;
}

private function getPropertyAccessor()
Copy link
Member

Choose a reason for hiding this comment

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

IMO we don't need this method anymore and can inline the createPropertyAccessor() call instead.

{
if (!class_exists('Symfony\Component\PropertyAccess\PropertyAccess')) {
Copy link
Member

Choose a reason for hiding this comment

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

we should use the class constant instead

throw new RuntimeException('Unable to use expressions with data path as the Symfony PropertyAccess component is not installed.');
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should not throw this exception in the constraint to allow to detect this mistake earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course we could throw it there. But than we need to check there if dataPath is set because only then we check if the PropertyAccess is avaibale

Copy link
Member

Choose a reason for hiding this comment

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

I think that would be fine.

}

return PropertyAccess::createPropertyAccessor();
Copy link
Member

Choose a reason for hiding this comment

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

the exception in src/Symfony/Component/Validator/Constraints/Expression.php should be moved here IMHO:
since the property is public, the check in the constructor is very fragile, only here would be robust

Copy link
Member

Choose a reason for hiding this comment

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

I would rather make the property private then. Catching these mistakes when the constraint is created allows to spot issues earlier which improves DX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the property private and added a magic __get like it was done in the File Constraint.

}
}
13 changes: 13 additions & 0 deletions src/Symfony/Component/Validator/Tests/Constraints/ExpressionValidatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,4 +235,17 @@ public function testExpressionLanguageUsage()

$this->assertTrue($used, 'Failed asserting that custom ExpressionLanguage instance is used.');
}

public function testExpressionWithCustomDataPath()
{
$constraint = new Expression(array('expression' => 'value <= this["dateEnd"]', 'dataPath' => 'root[data]'));

$this->setRoot(array('data' => array('dateEnd' => '2011-06-07', 'dateStart' => '2011-06-05')));
$this->setPropertyPath('');
$this->setProperty(null, 'property');

$this->validator->validate('2011-06-05', $constraint);

$this->assertNoViolation();
}
}
0