[WIP] Add bc layers for symfony 5 support

[acrobat]

Q	A
Branch?	"master"
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Fixed tickets
License	MIT
Doc PR	TODO

TODO:

• [ ] Update docs for generate name parameter deprecation
• [x] Check if the tests cover this code correctly and verify solution

@dbu this is a first try to work around the generate method signature in compatibility (+ some extra fixes for sf5 support). What do you think?

[acrobat]

Hmm @dbu I'm actually not sure what we should do when a route object is passed as the name parameter 😕

[acrobat]

@dbu thanks for the review! I've updated the patch to remove duplicated code and actually use the new _cmf_route parameter. Can you review again?

[dbu]

thanks, this is shaping up nicely!

what do you think about:

the dynamic router of the bundle further puts that from the route resolution into the parameters - unfortunately with a different key: https://github.com/symfony-cmf/routing-bundle/blob/master/src/Routing/DynamicRouter.php (i think back in the days we wanted to mimik the symfony behaviour of having underscore names in the defaults, but camelCase in the request attributes). i am a bit unsure if we should be looking for both keys, or only for the one from routing-bundle (which would require us to port the constant to the component so we can use it).

[acrobat]

thanks, this is shaping up nicely!

what do you think about:

the dynamic router of the bundle further puts that from the route resolution into the parameters - unfortunately with a different key: https://github.com/symfony-cmf/routing-bundle/blob/master/src/Routing/DynamicRouter.php (i think back in the days we wanted to mimik the symfony behaviour of having underscore names in the defaults, but camelCase in the request attributes). i am a bit unsure if we should be looking for both keys, or only for the one from routing-bundle (which would require us to port the constant to the component so we can use it).

But if I see it correctly the dynamic router of the bundle uses the constant we are using too, so it will actually be ok the way it is? Or am I missing something?

https://github.com/symfony-cmf/routing-bundle/blob/ec8cb976f50ddf7c792b6cf18d87259b914d1c50/src/Routing/DynamicRouter.php#L91-L94

[acrobat]

@dbu are the extra changes needed for your previous comment/question? Or any other remarks on the PR?

[dbu]

i was AFK for the weekend, hence the slow response.

But if I see it correctly the dynamic router of the bundle uses the constant we are using too, so it will actually be ok the way it is? Or am I missing something?

from how i read that section, the router moves the attribute from "our" key to its own key, meaning that when you generate, the attribute would be at the key the bundle specifies. we could make it stop doing that (copying rather than moving, for BC), or we could make this PR look at both keys. i feel that looking at both keys is less likely to cause trouble for existing systems, but it is the less elegant solution. maybe we should do both: no longer unset the $default and announce that the next major of the bundle will operate on the RouteObjectInterface::ROUTE_OBJECT and stop using self::ROUTE_KEY, while for now have this component look at both keys, prefering ROUTE_KEY when it is found. (as people working with the bundle might be updating that key to achieve some effect. people using only the component won't set the ROUTE_KEY but work on ROUTE_OBJECT exclusively.)

[pableu]

What's needed for this to go forward? We'd love to be able to upgrade our project using symfony-cmf/Routing to Symfony 5. It this the only piece of the puzzle needed to solve #240 and symfony-cmf/routing-bundle#447 too?

[dbu]

@pableu if you don't use the bundle, then this MR should be all you need. if you use the bundle, the bundle MR will be needed too, but i expect that one to be relatively simple once this thing is solved.

if you have inputs or opinions on how we should solve the parameter naming here, i would be glad to hear them. the problem is having both a reasonable thing for the future but not break BC if possible.

[webhdx]

It's a pity there is hardly any traction in this PR. There are lots of projects upgrading to Symfony 5 and this package is a huge blocker. At least for us at ezsystems/ezplatform.

@dbu I think this is a good approach but I'd prefer to have a proper route name indicating it will use an object from parameters instead of an empty string.

Is this the last issue before you could tag next major with Symfony 5 support? I could help if @acrobat is no longer interested in finishing it.

[acrobat]

I think we’re almost finished here, except for @dbu’s last comment. I will try to find some time to finish this pr.

[dbu]

I think this is a good approach but I'd prefer to have a proper route name indicating it will use an object from parameters instead of an empty string.

that is an interesting idea. that would also allow us to have a BC behaviour and the new behaviour only if that name is used. the name should be fairly specific to avoid accidental clashes with other routers or even actual route names. something like cmf_routing_object maybe?

[alexander-schranz]

@webhdx idea sounds great 👍 . @dbu cmf_routing_object sounds good for me.

[webhdx]

cmf_routing_object is fine for me as well :+1:

[acrobat]

So how to continue with this, replace the current usages of RouteObjectInterface::ROUTE_OBJECT to a new key?

[acrobat]

I've pushed an update with the specific routename

[acrobat]

@dbu I think this is ready for a final review!

[acrobat]

@dbu Thanks for the review, I've addressed your comments except for the comment about the doGenerate call.

[alexander-schranz]

I'm looking forward to this. @dbu is anything missing here?

[gisostallenberg]

I'm looking forward to this. @dbu is anything missing here?

Same here, @acrobat and @dbu can I do something to help?

[dbu]

Same here, @acrobat and @dbu can I do something to help?

each time i look over the diff, i find something new that would lead to a BC break. its really quite tricky here, to not break existing applications.

things that would help

• review the code and try to think if existing code might break from the behaviour change if it is not changed, or if it is changed to only use route names and the parameters for the route object. that includes the event listeners and everything. i don't think it will be possible to solve everything here, particularly custom route generators that rely on the object being in $name.
• try this branch in your application for a sanity check

one option would be to instead of this make a hard break with a new major version to keep things a bit simpler - without BC breaks, there are just so many hackeries we need to do to not break any existing code.

[dbu]

each time i look over the diff, i find something new that would lead to a BC break.

@acrobat that was not a slant at you, btw. i appreciate the effort you put into this, and the patience with my feedback. its just that i am afraid that we attempt something impossible with keeping BC with such a fundamental change to the basic assumption of this codebase.

[gisostallenberg]

one option would be to instead of this make a hard break with a new major version to keep things a bit simpler - without BC breaks, there are just so many hackeries we need to do to not break any existing code.

What would the upgrade path look like if you did this? Would it just be 'check your code for objects passed as names, choose a name and pass the object in the parameters'? In that case I'd suggest avoid adding all this complex BC layers which also needs to be maintained. But that is just an opinion :wink:. I think you and @acrobat are doing an incredible job in trying to move forward and it is much appreciated, but maybe it is just to hard to keep BC and it would be easy to upgrade a library using your package...

[dbu]

yep, basically the upgrade path will be to move route objects into the parameters, if something is calling route generation explicitly. the tricky bits will be listeners and event handlers and other places where this change is rather implicit.

if we don't do this whole forward/backward compatible thing, we can't really add the deprecations though - deprecating $name be an object when there is no other way to generate the route would be weird.

if we would aim to support the new and the old way but without any magic to work with symfony 5, we would probably still need at least 50% of the changes in here. which is still quite some effort, with the risk of breaking things...

i tend to say lets just make a BC breaking new major version that only works with symfony 5 and a rather hard break for it.

[gisostallenberg]

Although it is a pity all hard work seems te have been for nothing then, I agree with @dbu. What do you think @acrobat ?

[acrobat]

For me it's either solution. If we want to keep trying I'm up for it or if we choose to do the hard BC-break also good for me! Ofcourse it would be nice is we could do it without bc breaks, but I understand the choice if we move forward without.

Maybe we should still try to add some deprecation notices about the change for object route names?

[dbu]

Maybe we should still try to add some deprecation notices about the change for object route names?

if we don't support the new way, we should not do deprecations messages, as there would be no way to fix them.

[acrobat]

Good point! So what would be the next step here? Create a new PR and strip out all the routename as object support code and create a pr for 2.x with a small upgrade guide?

What will this mean for the RoutingBundle? Also a new major version there?

[dbu]

yes. sorry acrobat for not realizing earlier :-( your effort is not wasted though, we can use it to build the symfony 5 compatible version, and the rest was the proof that we can't reasonably support 4 and 5 at the same time...

master is 2.2. i suggest you start a new branch against master where you bump branch-alias to 3.x and to the BC breaks as needed to only have the new way. i can then create the 2.2 branch right before merging.

[acrobat]

No worries @dbu! It was a search for all of us.

I will start on a new pr!

[acrobat]

I've made a start for the new approach in #248

[dbu]

finished in #250