Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Routing] Fixed unexpected 404 NoConfigurationException#31207
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| if ('/' ===$pathinfo && !$this->allow) { | ||
| if ('/' ===$pathinfo && !$this->allow && !$this->allowSchemes) { |
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.
According to what we have inPhpMatcherTrait:
symfony/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherTrait.php
Lines 176 to 178 ine479b69
| if ('/' ===$pathinfo && !$allow && !$allowSchemes) { | |
| thrownewNoConfigurationException(); | |
| } |
| /** | ||
| * @expectedException \Symfony\Component\Routing\Exception\ResourceNotFoundException | ||
| * @expectedExceptionMessage No routes found for "/". |
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.
Making sure it is aResourceNotFoundException and not aNoConfigurationException which has no message.
| if ($hasRequiredScheme &&$requiredMethods && !\in_array($method,$requiredMethods)) { | ||
| $this->allow =array_merge($this->allow,$requiredMethods); | ||
| continue; |
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.
thiscontinue outside theif ($hasRequiredScheme) was preventing to collect the$this->allowSchemes bellow
yceruto commentedApr 23, 2019
(Travis failures are unrelated) |
0693bb9 to64579e8Compare
nicolas-grekas left a comment
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.
LGTM for 4.2 thanks.
For 3.4, what's your advice?
yceruto commentedMay 6, 2019
For 3.4 we need to collect |
yceruto commentedMay 8, 2019
@nicolas-grekas I just rebased to make tests pass, but I lost your changes, I'm sorry :( please, feel free to push them again, thanks! |
nicolas-grekas commentedMay 9, 2019
Or we can forget about it - 4.2 has new features that allow making the difference - 3.4 doesn't. Makes sense? I added back my changes. |
nicolas-grekas commentedMay 9, 2019
Thank you@yceruto. |
…ceruto)This PR was merged into the 4.2 branch.Discussion----------[Routing] Fixed unexpected 404 NoConfigurationException| Q | A| ------------- | ---| Branch? | 4.2| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#31199| License | MITThis is the patch for 4.2+We need a different patch for 3.4 that is more complex, I think.Commits-------aa71a42 [Routing] Fixed unexpected 404 NoConfigurationException
yceruto commentedMay 9, 2019
It's fine to me, thanks for merging. |
This is the patch for 4.2+
We need a different patch for 3.4 that is more complex, I think.