Skip to content

[Validator] Fix LazyLoadingMetadataFactory with PSR6Cache for non classname if tested values isn't existing class #26823

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

Merged
merged 2 commits into from
Apr 9, 2018

Conversation

pmontoya
Copy link

@pmontoya pmontoya commented Apr 5, 2018

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #26313
License MIT

If @Assert\Valid is applied to a string value, the value is searched in metadata cache and some characters aren't allowed in this cache. This create an unexpected exception.

Class existence is now tested before cache read.

@dmaicher
Copy link
Contributor

dmaicher commented Apr 5, 2018

Symfony 3.1 is not maintained anymore 😉 I guess the lowest maintained affected branch is 3.4? Before that there is no PSR-6 Cache.

But I think its fine to target even 2.7 for consistency? I would wait for a symfony member to confirm 😉

->method('read')
->with($testedValue)
->willThrowException(new InvalidArgumentException(sprintf('Cache key "%s" contains reserved characters {}()/\@:', $testedValue)));
$this->expectException(NoSuchMetadataException::class);
Copy link
Contributor

Choose a reason for hiding this comment

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

if we target 2.7 then we need to support php 5.3 here

$cache
->method('read')
->with($testedValue)
->willThrowException(new InvalidArgumentException(sprintf('Cache key "%s" contains reserved characters {}()/\@:', $testedValue)));
Copy link
Contributor

@dmaicher dmaicher Apr 5, 2018

Choose a reason for hiding this comment

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

just assert that we never actually call $cache->read(...)?

$this->expectException(NoSuchMetadataException::class);
try {
$factory->getMetadataFor($testedValue);
} catch (InvalidArgumentException $exception) {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for this then. We expect NoSuchMetadataException::class and if not the test just fails

@pmontoya pmontoya changed the base branch from 3.1 to 2.7 April 6, 2018 05:52
@pmontoya
Copy link
Author

pmontoya commented Apr 6, 2018

@dmaicher : Sorry. Too worried to not forget a comment, to find the right way to create patchs and I forgot the most important : the bug... 😄
Thanks for your comments

@nicolas-grekas nicolas-grekas changed the title [Validator] Fix LazyLoadingMetadataFactory with PSR6Cache for non cla… [Validator] Fix LazyLoadingMetadataFactory with PSR6Cache for non classname if tested values isn't existing class Apr 6, 2018
@@ -90,17 +90,17 @@ public function getMetadataFor($value)
return $this->loadedClasses[$class];
}

if (!class_exists($class) && !interface_exists($class)) {
Copy link
Member

@nicolas-grekas nicolas-grekas Apr 7, 2018

Choose a reason for hiding this comment

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

just one change please: false should be passed as 2nd arg to interface_exists() (no need to trigger autoloader twice)

Copy link
Author

Choose a reason for hiding this comment

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

done

Pascal Montoya and others added 2 commits April 9, 2018 10:32
@nicolas-grekas
Copy link
Member

Thank you @pmontoya.

@nicolas-grekas nicolas-grekas merged commit 5198f43 into symfony:2.7 Apr 9, 2018
nicolas-grekas added a commit that referenced this pull request Apr 9, 2018
…for non classname if tested values isn't existing class (Pascal Montoya, pmontoya)

This PR was merged into the 2.7 branch.

Discussion
----------

[Validator] Fix LazyLoadingMetadataFactory with PSR6Cache for non classname if tested values isn't existing class

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #26313
| License       | MIT

If @Assert\Valid is applied to a string value, the value is searched in metadata cache and some characters aren't allowed in this cache. This create an unexpected exception.

Class existence is now tested before cache read.

Commits
-------

5198f43 Disable autoloader call on interface_exists check
cd91420 [Validator] Fix LazyLoadingMetadataFactory with PSR6Cache for non classname if tested values isn't an existing class
@pmontoya pmontoya deleted the bug-26313 branch April 15, 2018 17:22
This was referenced Apr 27, 2018
This was referenced Apr 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants