sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

Import RouteCollectionInterface from `4.x` into `3.x` #6698

Closed franmomu closed 3 years ago

franmomu commented 3 years ago

Feature Request

In order to ease the upgrade to 4.0 I think we should import RouteCollectionInterface into 3.x making it compatible with RouteCollection, so people can prepare their code before upgrading, otherwise you'll get errors like:

Warning: Declaration of App\Admin\FooAdmin::configureRoutes(Sonata\AdminBundle\
!!    Route\RouteCollection $collection): void should be compatible with Sonata\Admin
!!    Bundle\Admin\AbstractAdmin::configureRoutes(Sonata\AdminBundle\Route\Route
!!    CollectionInterface $collection): void 
franmomu commented 3 years ago

Apparently it does not work in php 7.3: https://3v4l.org/glBKO 😞 but I think it could be useful for people using 7.4.

VincentLanglet commented 3 years ago

@franmomu I'm not sure we can do something about this.

When migrating from 3.x to 4.x, all the typehint should be updated. For RouteCollection, but also for all the methods were typehint was missing in 3.x ; I see this as a general rule.

But talking about Collection, I feel weird that we have a RouteCollectionInterface but we dont have an interface for https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Admin/FieldDescriptionCollection.php Shouldn't FieldDescriptionCollection and RouteCollection be in a similar situation (having an interface or not) ?

franmomu commented 3 years ago

When migrating from 3.x to 4.x, all the typehint should be updated.

That's the point, if users are able to update their code before upgrade to work with 3.x, then the upgrade is much easier.

But talking about Collection, I feel weird that we have a RouteCollectionInterface but we dont have an interface for https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Admin/FieldDescriptionCollection.php Shouldn't FieldDescriptionCollection and RouteCollection be in a similar situation (having an interface or not) ?

In this case I was talking about AbstractAdmin::configureRoutes() method. I think this method is usually overriden to add/remove routes, so if I can update my admins with:

protected function configureRoutes(RouteCollectionInterface $collection): void
{
// ...
}

Then it would work in 3.x and there is no need to modify when upgrading to 4.0.

VincentLanglet commented 3 years ago

When migrating from 3.x to 4.x, all the typehint should be updated.

That's the point, if users are able to update their code before upgrade to work with 3.x, then the upgrade is much easier.

It will help php 7.4 users indeed.

franmomu commented 3 years ago

Well we could also

kirya-dev commented 3 years ago

Apparently it does not work in php 7.3: https://3v4l.org/glBKO but I think it could be useful for people using 7.4.

Sub class cannot narrow down declaration. Only equal or extend. Its OOP

franmomu commented 3 years ago

Apparently it does not work in php 7.3: https://3v4l.org/glBKO but I think it could be useful for people using 7.4.

Sub class cannot narrow down declaration. Only equal or extend. Its OOP

😕 It depends on what declaration, when overriding a method, a subclass must return the same type or a more specific one (covariance), but as argument it accepts the same type or a more general one (contravariance) and support for this was fully added in PHP 7.4: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters