Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
dmaicher commentedApr 5, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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(newInvalidArgumentException(sprintf('Cache key "%s" contains reserved characters {}()/\@:',$testedValue))); | ||
| $this->expectException(NoSuchMetadataException::class); |
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.
if we target 2.7 then we need to support php 5.3 here
| $cache | ||
| ->method('read') | ||
| ->with($testedValue) | ||
| ->willThrowException(newInvalidArgumentException(sprintf('Cache key "%s" contains reserved characters {}()/\@:',$testedValue))); |
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.
just assert that we never actually call$cache->read(...)?
| $this->expectException(NoSuchMetadataException::class); | ||
| try { | ||
| $factory->getMetadataFor($testedValue); | ||
| }catch (InvalidArgumentException$exception) { |
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.
no need for this then. We expectNoSuchMetadataException::class and if not the test just fails
pmontoya commentedApr 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... 😄 |
| return$this->loadedClasses[$class]; | ||
| } | ||
| if (!class_exists($class) && !interface_exists($class)) { |
nicolas-grekasApr 7, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
just one change please:false should be passed as 2nd arg tointerface_exists() (no need to trigger autoloader twice)
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.
done
…ssname if tested values isn't an existing class
nicolas-grekas commentedApr 9, 2018
Thank you@pmontoya. |
…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 | MITIf@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 checkcd91420 [Validator] Fix LazyLoadingMetadataFactory with PSR6Cache for non classname if tested values isn't an existing class
Uh oh!
There was an error while loading.Please reload this page.
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.