[Router] routing cache crash when using generator_class #31964
Conversation
3247fbf to
adee1bd
Compare
| // 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'; |
There was a problem hiding this comment.
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 in getGenerator() instead?
Note also that this overrides any values set by the user. I'd suggest replacing the is_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.
It must be removed when the PhpGeneratorDumper 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 in getGeneratorDumperInstance or getMatcherDumperInstance, than getGenerator and getMatcher. But the changed values won't be visible if you call getOption, 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.
|
@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 to
a5b46e5
Compare
|
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 | MIT 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. 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
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.