Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AdminValueResolver don't check type properly #7846

Closed
mpoiriert opened this issue Jun 15, 2022 · 2 comments · Fixed by #7847
Closed

AdminValueResolver don't check type properly #7846

mpoiriert opened this issue Jun 15, 2022 · 2 comments · Fixed by #7847

Comments

@mpoiriert
Copy link
Contributor

mpoiriert commented Jun 15, 2022

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
Copy link
Contributor Author

@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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants