Skip to content
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.

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

[Validator] Add constraint on unique elements collection(Assert\Unique) #26555

Merged
merged 2 commits into from Mar 25, 2019

Conversation

@zzenmate
Copy link

commented Mar 15, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets #26535
License MIT
Doc PR symfony/symfony-docs#...
@zzenmate

This comment has been minimized.

Copy link
Author

commented Mar 15, 2018

Please, review feature.
If realization constraint is ok I will write tests and doc for this feature.

@zzenmate zzenmate changed the title [Validator] Add UniqueCollection constraint and validator [Validator] Add constraint on unique elements collection(Assert\UniqueCollection) Mar 15, 2018

return;
}
if (!is_array($value) && !($value instanceof \Traversable && $value instanceof \ArrayAccess)) {

This comment has been minimized.

Copy link
@ostrolucky

ostrolucky Mar 16, 2018

Contributor

Scrap ($value instanceof \Traversable && $value instanceof \ArrayAccess) and replace with !$value instanceof \IteratorAggregate. It's apparently interface of Traversable highest up in a chain which guarantees iterator is rewindable

$collectionElements = [];
foreach ($value as $element) {
if (in_array($element, $collectionElements)) {

This comment has been minimized.

Copy link
@ostrolucky

ostrolucky Mar 16, 2018

Contributor

third argument to true is important here to avoid obscure bugs https://3v4l.org/9QGNo

public function validate($value, Constraint $constraint)
{
if (!$constraint instanceof UniqueCollection) {
throw new UnexpectedTypeException($constraint, __NAMESPACE__.'\UniqueCollection');

This comment has been minimized.

Copy link
@ostrolucky

ostrolucky Mar 16, 2018

Contributor

We can use ::class now

*
* @author Yevgeniy Zholkevskiy <zhenya.zholkevskiy@gmail.com>
*/
class UniqueCollection extends Constraint

This comment has been minimized.

Copy link
@ostrolucky

ostrolucky Mar 16, 2018

Contributor

Symfony constraints don't tend to add such suffix. See e.g. constraint "All". Should be just Unique. Would be in line with #9888 as well.

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Mar 16, 2018

@zzenmate zzenmate changed the title [Validator] Add constraint on unique elements collection(Assert\UniqueCollection) [Validator] Add constraint on unique elements collection(Assert\Unique) Mar 16, 2018

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2018

Would wait for a symfony team member to confirm, but I think this is pretty straight forward and feature should be accepted.

status: needs review

return;
}
if (!$value instanceof \IteratorAggregate) {

This comment has been minimized.

Copy link
@ostrolucky

ostrolucky Mar 17, 2018

Contributor

is_array part must stay, I wanted to change second part of condition only. This won't work with arrays now.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

@zzenmate please check fabbot's failure

@ogizanagi
Copy link
Member

left a comment

Thanks for your contribution!

foreach ($value as $element) {
if (in_array($element, $collectionElements, true)) {
$this->context->buildViolation($constraint->message)
->setParameter('{{ value }}', $this->formatValue($value))

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Mar 19, 2018

Member

Additionnally to fabbot reported issues, there is too much indentation here:

                $this->context->buildViolation($constraint->message)
-                              ->setParameter('{{ value }}', $this->formatValue($value))
-                              ->setCode(Unique::IS_NOT_UNIQUE)
-                   ->addViolation();
+                   ->setParameter('{{ value }}', $this->formatValue($value))
+                   ->setCode(Unique::IS_NOT_UNIQUE)
+                   ->addViolation();
return;
}
if (!is_array($value) && !$value instanceof \IteratorAggregate) {

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Mar 19, 2018

Member

I've seen @ostrolucky 's #26555 (comment), but actually, we don't need ArrayAccess here, right?
So !is_array($value) && !$value instanceof \Traversable would be enough.

This comment has been minimized.

Copy link
@ostrolucky

ostrolucky Mar 19, 2018

Contributor

I thought so too until recently. Apparently this is considered too unsafe, see #24577 (comment) and #25506

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi Mar 19, 2018

Member

So, IIUC that's an issue we already have with AllValidator then?

Was it already raised somewhere and do we need to add a deprecation to warn about this issue if $value isn't implementing IteratorAggregate?

This comment has been minimized.

Copy link
@ostrolucky

ostrolucky Mar 21, 2018

Contributor

Dunno, what do you say @nicolas-grekas? I guess there should be an RFC to decide how to move forward in consistent way and let others to speak their mind about this.

This comment has been minimized.

Copy link
@xabbuh

xabbuh Sep 24, 2018

Member

I'd keep it as it is.

$collectionElements[] = $element;
}
}
}

This comment has been minimized.

Copy link
@yceruto

yceruto Mar 19, 2018

Contributor

missing newline at end of file

@zzenmate

This comment has been minimized.

Copy link
Author

commented Mar 20, 2018

status: needs review

@ostrolucky
Copy link
Contributor

left a comment

This test suite is IMO too noisy. Check e.g. RegexValidatorTest to learn how to utilize dataProviders to shrink this.

@zzenmate

This comment has been minimized.

Copy link
Author

commented Mar 22, 2018

@ostrolucky Thank you for your advice! I have done it.
status: needs review

}
if (!is_array($value) && !$value instanceof \IteratorAggregate) {
throw new UnexpectedTypeException($value, 'IteratorAggregate');

This comment has been minimized.

Copy link
@ostrolucky

ostrolucky Apr 1, 2018

Contributor

Sorry but I think we should not throw here, to avoid issues like this one #26463

@nicolas-grekas nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018

@xabbuh

xabbuh approved these changes Sep 24, 2018

Copy link
Member

left a comment

with a minor comment

*/
class Unique extends Constraint
{
const IS_NOT_UNIQUE = '7911c98d-b845-4da0-94b7-a8dac36bc55a';

This comment has been minimized.

Copy link
@xabbuh

xabbuh Sep 24, 2018

Member

public const

return;
}
if (!is_array($value) && !$value instanceof \IteratorAggregate) {

This comment has been minimized.

Copy link
@xabbuh

xabbuh Sep 24, 2018

Member

I'd keep it as it is.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

oh, please also mention the new constraint in the changelog of the component

@fabpot

fabpot approved these changes Oct 10, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

Friendly ping @zzenmate, could you please add an entry in the changelog file of the component?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

And create a doc issue/PR also please?

@nicolas-grekas nicolas-grekas force-pushed the zzenmate:unique-collection-constraint branch from ad8e7d4 to bfa3278 Mar 13, 2019

@nicolas-grekas nicolas-grekas force-pushed the zzenmate:unique-collection-constraint branch from bfa3278 to d0eb13e Mar 13, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Rebased, ready.

@fabpot

fabpot approved these changes Mar 25, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

Thank you @zzenmate.

@fabpot fabpot merged commit d0eb13e into symfony:master Mar 25, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 25, 2019

feature #26555 [Validator] Add constraint on unique elements collecti…
…on(Assert\Unique) (zenmate, nicolas-grekas)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Validator] Add constraint on unique elements collection(Assert\Unique)

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #26535
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Commits
-------

d0eb13e Rebase and update to latest CS
fc66683 Add UniqueCollection constraint and validator
@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

Translation added in e863621. Typo in the error message fixed in e3d42a9.

}
if (!\is_array($value) && !$value instanceof \IteratorAggregate) {
throw new UnexpectedValueException($value, 'array|IteratorAggregate');

This comment has been minimized.

Copy link
@ro0NL

ro0NL Mar 29, 2019

Contributor

why not iterable?

This comment has been minimized.

Copy link
@ostrolucky

ostrolucky Mar 29, 2019

Contributor

see review chain

This comment has been minimized.

Copy link
@ro0NL

ro0NL Mar 29, 2019

Contributor

I thought so too until recently. Apparently this is considered too unsafe, see #24577 (comment) and #25506

?

was it a blocker for this PR?

This comment has been minimized.

Copy link
@ro0NL

ro0NL Mar 29, 2019

Contributor

i can pass a broken/unrewindbable iterator as aggregate just as easy. But i cant pass a working iterator/iterable directly. That's poor DX IMHO.

This comment has been minimized.

Copy link
@ostrolucky

ostrolucky Mar 29, 2019

Contributor

Consequencies of this validator being applied on non-rewindable iterators are much bigger than in #25506 so yes (generally, after validator validates the data, data is processed)

*
* @author Yevgeniy Zholkevskiy <zhenya.zholkevskiy@gmail.com>
*/
class Unique extends Constraint

This comment has been minimized.

Copy link
@ro0NL

ro0NL Mar 29, 2019

Contributor

UniqueCollection?

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.