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

AdminValueResolver don't check type properly #7846

Closed mpoiriert closed 2 years ago

mpoiriert commented 2 years ago

Subject

When typing argument with the admin interface instead of the exact admin type the admin is not found.

Code example

class Admin implements \Sonata\AdminBundle\Admin\AdminInterface
{

}

class Action
{
  public function failingAction(\Sonata\AdminBundle\Admin\AdminInterface $admin)
  {
     // Will not found the admin parameter
  }

  public function workingAction(Admin $admin)
  {
    // Everything is ok
  }
}

How to fix

This is because the is_subclass_of function use in AdminValueResolver does check if the class is the same as requested. Also the is_a function have a different behaviour base on a class string or an object, probably a bug in PHP, so it cannot be use of the first check.

class Admin implements \Sonata\AdminBundle\Admin\AdminInterface
{

}

class Action
{
  public function failingAction(\Sonata\AdminBundle\Admin\AdminInterface $admin)
  {
  }

  public function workingAction(Admin $admin)
  {
  }
}

var_dump(is_a(ClassX::class, InterfaceY::class)); // false

var_dump(is_a(ClassX::class, ClassX::class)); // false

var_dump(is_a(new ClassX(), InterfaceY::class)); // true

var_dump(is_a(new ClassX(), ClassX::class)); // true

var_dump(is_subclass_of(ClassX::class, ClassX::class)); // false

var_dump(is_subclass_of(ClassX::class, InterfaceY::class)); // true

Recommendation would be the check if $type === AdminInterface::class or is_subclass_of.

mpoiriert commented 2 years ago

@VincentLanglet You should check this one as well before releasing. It's easy to fix I just want to make sure it will be accepted.

Behaviour of the Admin Fetcher is not base on the specific class so calling it manually it a controller would work that why I was expecting the AdminValueResolver to work the same way.

VincentLanglet commented 2 years ago

Sure, feel free to do a PR (with a test will be great) and I'll approve