Skip to content

[Routing] Fix using a custom matcher & generator dumper class #35261

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

Merged
merged 1 commit into from
Jan 8, 2020

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Jan 8, 2020

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 [Router] routing cache crash when using generator_class #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.

@fancyweb
Copy link
Contributor Author

fancyweb commented Jan 8, 2020

I applied the same patch logic on the matcher btw because looking at the code, I guess it suffers the same problem.

@yceruto yceruto added this to the 4.3 milestone Jan 8, 2020
@nicolas-grekas
Copy link
Member

Thank you @fancyweb.

nicolas-grekas added a commit that referenced this pull request Jan 8, 2020
…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
@nicolas-grekas nicolas-grekas merged commit 3a840a9 into symfony:4.3 Jan 8, 2020
@fancyweb fancyweb deleted the routing-compiled-fix branch January 8, 2020 17:24
This was referenced Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants