Skip to content

Conversation

gseidel
Copy link
Contributor

@gseidel gseidel commented May 27, 2020

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
License MIT

If the route is fetched over a relation from the database, it could be, that the route is still a proxy object (For lazy loading). If a url is generated with a proxy route object, then symfony will generate a string like /path?_route_object%5B__isInitialized__%5D=1, because http_build_query also accept objects and generate a value if the object has a public property.

I think the parameter RouteObjectInterface::ROUTE_OBJECT is not necessary to pass it doGenerate. So it should be unset if exists.

Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a lot, good point!

is this the only point where we need to do this, or are there other places in the code? i guess whenever we read the route from the ROUTE_OBJECT parameter potentially has the same issue.


// the parameter must be unset to avoid unexpected generation behaviour
if (array_key_exists(RouteObjectInterface::ROUTE_OBJECT, $parameters)) {
unset($parameters[RouteObjectInterface::ROUTE_OBJECT]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to avoid any possible side effects, i would move this into the elseif right after line 67.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I updated the code and use it inside the elseif

Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

i double checked and only found ContentAwareGenerator where we use the ROUTE_OBJECT as well, and that calls the parent generate method which is what you are fixing here. so we are good 👍

@dbu dbu merged commit b0a9609 into symfony-cmf:master May 27, 2020
@dbu
Copy link
Member

dbu commented May 27, 2020

i updated the changelog and tagged 2.3.2 with this.

@gseidel
Copy link
Contributor Author

gseidel commented May 27, 2020

Thanks a lot @dbu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants