search

symfony-cmf / Routing

Routing component building on the Symfony Routing component
Other
288 stars 69 forks source link

Symfony 5 support #248

Closed acrobat closed 4 years ago

acrobat commented 4 years ago
Q A
Branch? "master"
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Fixed tickets
License MIT
Doc PR

As discussed in #245 this PR adds symfony support without the required bc layer for objects as routeName. I've bumped the php version to 7.2 to allow us to keep the current generator signature for now, which allows for an easier upgrade.

@dbu should I open a PR for the dev-kit to bump the php version tested in travis?

dbu commented 4 years ago

should I open a PR for the dev-kit to bump the php version tested in travis?

once we are ready to merge, we should adjust the dev-kit. before that, editing the file by hand is the way to go.

acrobat commented 4 years ago

@dbu php 7.2 has type widening implemented (https://wiki.php.net/rfc/parameter-no-type-variance). This allows to drop the typehint on implenting classes, so indeed this could help us in v2.x but would it solve the whole BC breaking issue we try to avoid?

acrobat commented 4 years ago

should I open a PR for the dev-kit to bump the php version tested in travis?

once we are ready to merge, we should adjust the dev-kit. before that, editing the file by hand is the way to go.

Ok, will do!

dbu commented 4 years ago

@dbu php 7.2 has type widening implemented (https://wiki.php.net/rfc/parameter-no-type-variance). This allows to drop the typehint on implenting classes, so indeed this could help us in v2.x but would it solve the whole BC breaking issue we try to avoid?

i think it would. version 2.3 would simply implement looking in the parameters but falling back to $name being the object if the parameter is not there, but raise a deprecation warning if the $name is not a string. i would let each bit check that separately and not have e.g. the chain router convert from old to new - the chain router, the event and the generator would each need to support the new mode while falling back to the old mode.

acrobat commented 4 years ago

That would indeed be possible! Let me see what I can come up with, I will tag you if you can review the new solution!

acrobat commented 4 years ago

I've pushed an update where I've added the deprecation notice and the replacement to pass objects into the generate method.

Next up is added some tests for the new object parameter, I think this will also highlight some area's we still need to update

alexander-schranz commented 4 years ago

Looks like travis did just don't get any feedback but the tests are green. StyleCI seems to have some issues which need to be fixed.

acrobat commented 4 years ago

@dbu I think I fixed most of the comments, so I guess this PR is ready for another review round!

dbu commented 4 years ago

looks good, i am doing a couple of tweaks now...