[7.x] Re-use Router::newRoute() inside CompiledRouteCollection

[klimov-paul]

Fixes #32415

Re-use Router::newRoute() inside CompiledRouteCollection, allow to use single place for the custom route definition.

[driesvints]

I don't immediately see anything wrong with this so I guess it's fine. Can we remove the ->setRouter($this->router) and ->setContainer($this->container) calls as well in the old method?

[klimov-paul]

Redundant invocations of setRouter() and setContainer() have been removed.

[pgrenaud]

@klimov-paul Your bug report said that current behavior was breaking custom Route class, but your fix just broke any custom Router class that was overriding the newRoute() method. That broke our custom Router and we now have to maintain a separate branch for when laravel/framework >= 7.7.0.

@driesvints A minor update should remain backward compatible and changing a method signature is definitely not. Is it possible to revert this and postponed this change for the next Laravel release? Thanks.

[driesvints]

@pgrenaud given that this has been done since a few releases already I don't think it's wise to revert and do another breaking change for anyone who already adapted their code.

[pgrenaud]

@driesvints I just notice that I got the version wrong and it's actually since 7.7.0. I wish I had caught that sooner.

[driesvints]

@pgrenaud what's your use case for overwriting the method?

[pgrenaud]

@driesvints We have a custom router that provides localized routes. We added a custom method Route::groupLocales() to prefix URL with locales. For instance:

http://example.com/en/home

Will get routed to the home route with the locale en.

You can find out more here: https://github.com/eXolnet/laravel-translation

[devcircus]

Definitely agree that method signature changes like this should never slip through. However it could be too late now as it may compound the problem. 🤦🏼‍♂️

[klimov-paul]

we now have to maintain a separate branch for when laravel/framework >= 7.7.0.

It is sad to hear someone suffers from this update.
If it will give you some comfort, you do not need a separated branch.
PHP allows publishing of the protected method inherited from parent class. For example:

class A
{
    protected function foo() {}
}

class B extends A
{
    public function foo() {} // no error! 
}

If you release a patch which newRoute() overridden as public, it will work both for 7.x and 6.x.

[driesvints]

@klimov-paul the problem is that in this case it's the other way around. The upstream is now public but all downstreams who were overriding it are protected. And that does trigger an error.

@pgrenaud sorry you got caught by this. We try to pay attention to these things but sometimes things slip through. It's a little late to revert now unfortunately. We'll try to take care of this better in the future.

[pgrenaud]

@klimov-paul TIL! Thanks for letting me know. I'll look into it.