Issue with `object|void` style returns

[mbabker]

/**
 * Add a command to the application.
 *
 * @param   CommandInterface  $command  The command to add
 *
 * @return  CommandInterface|void  The registered command or null if the command is not enabled
 *
 * @since   __DEPLOY_VERSION__
 */
public function addCommand(CommandInterface $command)
{
    if (!$command->isEnabled())
    {
        return;
    }

    return $command;
}

Results in...

----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 128 | ERROR | Function return type is not void, but function is
     |       | returning void here
     |       | (Joomla.Commenting.FunctionComment.InvalidReturnNotVoid)
----------------------------------------------------------------------

[Paladin]

I'm afraid I'm on the side of phpcs in this one. The whole "return something or nothing" concept squicks me. I think phpcs is expecting the return of a null object in this case, i.e., something that supports CommandInterface but does nothing and is nothing. It's a different approach to the design of the code, but it pays off in simplicity down the line as the code can be less defensive (no more "test value before calling" cluttering up the code flow).

references: http://www.virtuouscode.com/introduction-to-much-ado-about-naught/ (ruby focused) https://www.sitepoint.com/the-null-object-pattern-polymorphism-in-domain-models/ (PHP, but less complete) https://gist.github.com/eric1234/7991763

[mbabker]

Changing my API implementation is easy (especially as this is new code). But, we do have cases throughout the project where we do have a mixed returns like this and IMO it could start to be problematic if we force code refactoring (with behavior changes) on existing APIs to appease a code style rule.

[photodude]

I believe the code should handle mixed returns, Possibly need to review the source file for differences as there were some big changes in 2.8 which I think was released after the last time I reviewed our custom sniffs for differences. There are 4 changes to that file since I last reviewed it. One of the changes might be related to the issue here.