(sorry, once again I don't remember which is the appropriate issue queue for this)
Some methods in Drupal have a parameter that controls whether an exception can be thrown, or null is returned on failure.
Example from Drupal\Component\Plugin\Discovery\DiscoveryInterface
:
/**
* Gets a specific plugin definition.
*
* @param string $plugin_id
* A plugin id.
* @param bool $exception_on_invalid
* (optional) If TRUE, an invalid plugin ID will throw an exception.
*
* @return mixed
* A plugin definition, or NULL if the plugin ID is invalid and
* $exception_on_invalid is FALSE.
*
* @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
* Thrown if $plugin_id is invalid and $exception_on_invalid is TRUE.
*/
public function getDefinition($plugin_id, $exception_on_invalid = TRUE);
Such methods confuse the IDE and static code analysis tools.
Because the IDE does not understand the meaning of the second parameter, one still needs to wrap the entire thing in try/catch to prevent inspection warnings.
I propose, as a policy, that whenever we introduce new methods, there should be distinct methods with and without exception.
E.g. if we were to rewrite Drupal\Component\Plugin\Discovery\DiscoveryInterface
(which I am not proposing at all):
/**
* Gets a specific plugin definition or NULL.
*
* @param string $plugin_id
* A plugin id.
*
* @return mixed
* A plugin definition, or NULL if the plugin ID is invalid.
*/
public function getDefinition($plugin_id);
/**
* Gets a specific plugin definition, or throws an exception.
*
* @param string $plugin_id
* A plugin id.
*
* @return mixed
* A plugin definition, if the $plugin_id is valid (exception otherwise).
*
* @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
* Thrown if $plugin_id is invalid.
*/
public function requireDefinition($plugin_id);
For my taste, we could name the first method getDefinitionOrNull()
instead. But I think to remember from previous discussions that people find this awkward, so I am fine with getDefinition()
.
The question whether this is a good idea or not applies beyond Drupal or PHP, so I started this stackexchange question: https://softwareengineering.stackexchange.com/questions/362900/parameter...
Comments
Comment #2
donquixote CreditAttribution: donquixote commentedThe stackexchange discussion turned out quite interesting.
Arseni Mourzenko:
Ewan:
Here we learn about a pattern in C# / .NET, which in PHP would be like using a by-reference parameter for the result, whereas the return value would be true/false for success or failure. I am not advocating for this here, it just looks interesting.
Polygnome:
Valuable lesson here..
Voo:
This is an interesting point.
Why would we want to provide the two cases? Why can we not make a decision which version we want to support?
I do have some thoughts on this on my own, which I posted over there.
supercat:
This is true.
Wrappers or decorators become more cluttered by having two distinct methods.
But zero upvotes, at the time I write this comment.
Conclusion
Some people question whether we should ever support two behaviors (exception vs null).
But a clear tendency seems to be that if we support two behaviors, it should be in separate methods, and not in the same method, for the sake of a more rigid signature.