Admin Pool getAdminByAdminCode() should throw exceptions instead of returning false

7ochem commented 5 years ago

Throw exception vs. return false

Environment

sonata-project/admin-bundle        3.48.1     3.48.1

$ php -v
PHP 7.2.17-1+ubuntu16.04.1+deb.sury.org+3 (cli) (built: Apr 10 2019 10:50:19) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.2.17-1+ubuntu16.04.1+deb.sury.org+3, Copyright (c) 1999-2018, by Zend Technologies

Subject

I ran into this when I got the infamous PHP Warning:

PHP Warning:  Uncaught Error: Call to a member function setRequest() on boolean

On the line above I was trying to retrieve a Sonata admin from the Admin Pool:

$admin = $this->sonata->getAdminByAdminCode($code);
$admin->setRequest($request);

So calling \Sonata\AdminBundle\Admin\Pool::getAdminByAdminCode() could return \Sonata\AdminBundle\Admin\AdminInterface or boolean false. You would say that in my code I should add a check for a false value returned by getAdminByAdminCode(), but I think the method should be improved and not return false but throw exceptions, because:

I should not have to place false checks in my code when I use this method.
If I get exceptions, then I get much more feedback from the code than getting a vague PHP warning
I can have these exceptions written to a log for review later on, where when I have to check against false I have to write my own messages (to be written to a log) on all places where I use the method.
This makes the method having a consistent return type, also definable with a PHP 7 type declaration: public function getAdminByAdminCode(string $adminCode) : AdminInterface
The method returns false in two different places. Having exceptions will show you in which place there's something wrong

https://github.com/sonata-project/SonataAdminBundle/blob/b6bc0297d524fdcf361d77a5181508ba58bcf6a5/src/Admin/Pool.php#L236-L255

I'm not issuing this as a bug, but a feature request, because implementing throwing errors will break things.

greg0ire commented 5 years ago

If you want to contribute this, you can start by making a PR on the stable branch with this:

@trigger_error(sprintf(
     'Calling "%s" with an invalid admin code is deprecated since sonata-project/admin-bundle 3.x and will throw an exception in 4.0',
     __METHOD__
), E_USER_DEPRECATED);

Also, a new hasAdminByAdminCode() method should be introduced.

7ochem commented 5 years ago

If you want to contribute this [...]

I'd really like to. Just wanted to check if this is a good idea to you too.

Also, a new hasAdminByAdminCode() method should be introduced.

Good point. I'll add that too.

B.t.w. there's no 4.0 branch to contribute the actual code to, right?

OskarStark commented 5 years ago

This should go into 3.x branch 👍🏻

greg0ire commented 5 years ago

B.t.w. there's no 4.0 branch to contribute the actual code to, right?

It's called master :wink: , but you would have to do what I said on 3.x first, then wait for 3.x to be merged on master (happens nightly), and then make the PR on master.

7ochem commented 5 years ago

I will be waiting on #5543 before issuing a PR for the 3.x branch

phansys commented 5 years ago

@7ochem, #5543 is now merged :+1:

sonata-project / SonataAdminBundle