Skip to content
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

[Router] routing cache crash when using generator_class #31964

Open
wants to merge 1 commit into
base: 4.3
from

Conversation

Projects
None yet
3 participants
@dFayet
Copy link
Contributor

commented Jun 9, 2019

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.

@dFayet dFayet force-pushed the dFayet:hotfix/issue-31807 branch from 5be8299 to 3247fbf Jun 9, 2019

@dFayet dFayet force-pushed the dFayet:hotfix/issue-31807 branch from 3247fbf to adee1bd Jun 9, 2019

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Jun 9, 2019

// 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';

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 10, 2019

Member

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.)

This comment has been minimized.

Copy link
@dFayet

dFayet Jun 10, 2019

Author Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.