Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Router] routing cache crash when using generator_class#31964
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
3247fbf toadee1bdCompare| // Fallback to PhpGeneratorDumper if the UrlGenerator and UrlGeneratorDumper are not consistent with each other | ||
| if (is_a($this->options['generator_class'], CompiledUrlGenerator::class,true) !== | ||
| is_a($this->options['generator_dumper_class'], CompiledUrlGeneratorDumper::class,true)) { | ||
| $this->options['generator_dumper_class'] ='Symfony\\Component\\Routing\\Generator\\Dumper\\PhpGeneratorDumper'; |
nicolas-grekasJun 10, 2019 • 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.
I'm not super comfortable with this:PhpGeneratorDumper is deprecated, so that this line of code will have to change in v5. But to what? Let's say we remove it: this means we broke BC without any deprecation notice in v4. To the minimum, this might miss one.
Maybe the added logic is in the wrong method: shouldn't it be ingetGenerator() instead?
Note also that this overrides any values set by the user. I'd suggest replacing theis_a by a direct string comparison instead (that still wouldn't allow detecting when the option is explicitly set vs loaded from the defaults.)
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.
It must be removed when thePhpGeneratorDumper will be removed. You're right it misses a notice.
It does not remove all the values set by the user, only the "invalid" ones.
The only cases where values are overriden is when the user use a compiled generator|matcher with a "non-compiled" generator|matcher dumper (inverse is also true)
I think it should be more ingetGeneratorDumperInstance orgetMatcherDumperInstance, thangetGenerator andgetMatcher. But the changed values won't be visible if you callgetOption, your thoughts?
I don't think string comparision is right here, as the given classes will in most cases be children of the generator|matcher classes.
dFayet commentedJun 22, 2019 • 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.
@nicolas-grekas In#28865 the default router generator and matcher were changed to the new Rather than "force changing" the classes used by the router in case of mismatch, would it be better to revert the changes made on |
adee1bd toa5b46e5Comparenicolas-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.
(I pushed here the implementation you talked about - no need for additional notices as one will already be triggered when loading the class)
nicolas-grekas commentedSep 6, 2019
Thank you@dFayet. |
…Fayet)This PR was merged into the 4.3 branch.Discussion----------[Router] routing cache crash when using generator_class| Q | A| ------------- | ---| Branch? | 4.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#31807| License | MITSince#28865 the Router use, by default, new generator, matcher, and dumpers.This leads to crash when the Router use a custom generator, or matcher based on the old ones.Commits-------a5b46e5 Fix routing cache broken when using generator_class
…ass (fancyweb)This PR was merged into the 4.3 branch.Discussion----------[Routing] Fix using a custom matcher & generator dumper class| Q | A| ------------- | ---| Branch? | 4.3| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -This PR fixes a BC break I encountered while upgrading an existing project from 4.2 to 4.4. In this project I use a custom `generator_dumper_class` that is not a `CompiledUrlGeneratorDumper` (it didn't exist yet). I faced 2 problems:- The generator is considered "compiled" while it is not. This is because we don't check if the `generator_dumper_class` is effectively a `CompiledUrlGeneratorDumper` to compute the `$compiled` variable. That result in a `\TypeError: Return value of Symfony\Component\Routing\Router::getCompiledRoutes() must be of the type array, int returned`- My custom dumper is not used at all. This is because of#31964. I altered the condition to fall back only in one way and not the other. The original issue is still fixed (if one uses a classic `UrlGenerator` + a `CompiledUrlGeneratorDumper`, it fall backs on `PhpGeneratorDumper`). However, if one uses a `CompiledUrlGenerator` + a classic `PhpGeneratorDumper` (my case), the classic dumper is still returned. Since `$compiled` is now correctly computed, this case works fine. The Router won't try to get the compiled routes and will use the "old" way.Commits-------3a840a9 [Routing] Fix using a custom matcher & generator dumper class
Uh oh!
There was an error while loading.Please reload this page.
Since#28865 the Router use, by default, new generator, matcher, and dumpers.
This leads to crash when the Router use a custom generator, or matcher based on the old ones.