Updated AbstractActionController::indexAction return type to reflect real usage

[Slamdunk]

• Is this related to quality assurance?

When running static analysis against a simple userland controller like:

class MyController extends AbstractActionController
{
    public function indexAction()
    {
        return ['posts' => ['foo', 'bar']];
    }
}

That do NOT provide the return type, neither with PHP 7.1 return type nor via DocBlock, tools like PHPstan use inherited docs and report:

Method MyController::indexAction() should return Zend\View\Model\ViewModel but returns array.

Of course users should add return types, but it would result in requiring a massive code update without real benefits, since the default behavior of Zend\Mvc framework is to allow:

1. null from \Zend\Mvc\View\Http\CreateViewModelListener::createViewModelFromNull listener
2. array from \Zend\Mvc\View\Http\CreateViewModelListener::createViewModelFromArray listener
3. ViewModel from \Zend\Mvc\View\Http\DefaultRenderingStrategy::render listener
4. ResponseInterface from \Zend\Mvc\SendResponseListener::sendResponse listener

This change only applies to indexAction since I guess its usage is widespread: notFoundAction is untouched as users that override it are, IMHO, already advanced ones.

Ping @Ocramius you type lover ❤️

Reference: https://github.com/Slamdunk/phpstan-zend-framework/issues/5

[Slamdunk]

As it seems you've worked on ZF with PHPStan quite a lot, what do you think about this @thomasvargiu?

[thomasvargiu]

@Slamdunk I have the same problem in my projects and right now I’m ignoring that kind of errors.
But yes, even if I don’t like methods that can return 5 or 6 different types, we should fix the phodoc return type.

[thomasvargiu]

But probably it's better to use only ResponseInterface and ModelInterface, starting deprecating array, null, and void.

[thomasvargiu]

If we want to support every handled type it should be:
null|void|array|ModelInterface|ResponseInterface adding void and using the ModelInterface.

[Slamdunk]

1. You are correct about ModelInterface, I was deceived by the alias in DefaultRenderingStrategy
2. But why void? In the years I use ZF 2/3, I've always returned something in the actions, if needed the users shall use explicit return null; statements

[thomasvargiu]

as the same if I say:

• we can remove array, if need the users shall use explicit return new ViewModel()
• we can remove null, if need the users shall use explicit return new ViewModel()

This is related to my comment in the discussion.

[Slamdunk]

I understand: I would like to be a bit more pragmatic with this PR

• Requiring users that run static analysis to change their return; to return null; is ok IMHO, this specific type awareness is important and easy to implement, and the occurrences would be very few as far as I can tell
• On the other way, requiring to move from null|array to ViewModel the indexAction would for sure mean updating all the codebases that use zend-mvc, a bit harsh move, wouldn't it?

[weierophinney]

Honestly, eventually we should have it be just void.

PSR-14 (event dispatcher) ignores return values from listeners. As such, if/when we adopt it, we will need controllers to update the event instead of returning a value.

For now, mixed is probably the best possible solution for a return type hint, as you can return basically anything; some things are just more of interest to the event dispatcher than others currently.

[Xerkus]

Also, what return type is allowed is defined by the listeners. By default they allow null, array, view model and response. mixed should be on the list, technically, but in practice i never seen anything else returned from controllers.

Implementers can declare overriding return type if they intentionally return anything else. phpstan and co won't complain about that, will they?

[weierophinney]

@Xerkus I'm well aware of how actions work! My point is that it would likely make far more sense for actions to have void returns, and be passed the event (and, potentially, the request and/or response instance), and call methods on it, instead of returning a value.

Right now, what happens is this:

• onDispatch() takes the return value, passes it to MvcEvent::setResult(), and returns it.
• DispatchListener checks to see if the return value is an associative array, and, if so, casts it to an ArrayObject and pushes it back into the MvcEvent::setResult() method, and returns the value.
• Application::run() checks to see if the last result returned is a response; if so, it returns it, but otherwise it ignores the return value.

The only reason for pushing a return value other than a response to MvcEvent::setResult() is to allow later listeners on the event to act on the results of previous listeners. Currently, this occurs with one of the following:

• associative array/ArrayObject instances and null return values trigger the CreateViewModelListener, which will cast the value to a ViewModel instance and pass it back to MvcEvent::setResult().

• ViewModel instances trigger the InjectViewModelListener to inject the result into the existing view model composed in the event, and the InjectTemplateListener to push a template name into the view model if none was specified.

As such, for the shipped functionality, we'd have: void|null|array|ViewModel|Response. Why void? Because a controller might operate on the event or its children directly, without returning a Response to indicate processing is complete. Forcing a return null does not mimic real-world usage; furthermore, a null return forces injection of an empty ViewModel, which may or may not be desired.

In terms of what is allowed, MvcEvent::setResult() allows mixed, which allows later listeners to act on whatever value is present, regardless of type. If we want to be explicit about what IndexAction can return and what the default shipped functionality will work with, it should include mixed, and, in its entirety read void|null|array|ViewModel|Response|mixed.

---

Tangent time.

Since the only return value that is of interest to the MVC workflow is a response, it would make far more sense for controllers to not return a value unless it is a response at this time. Even better: have a new method on the MvcEvent, finalizeResponse(ResponseInterface $response), which would stop propagation; Application::run() would check for a finalized response and return it.

If a controller wants to set a view model in the event, do it explicitly. Listeners can check to see if view models have templates set, and, if not, set them. Listeners can also set default view models if none are present. If the controller wants to update the response, but not mark it as final, it can do just that (by omitting a call to finalizeResponse()).

[Slamdunk]

Implementers can declare overriding return type if they intentionally return anything else. phpstan and co won't complain about that, will they?

@Xerkus overriding return types is always preferred and boosted, and static analyzer won't complain, instead will thank for more accurate documentation about typing.
This entirely PR, as written, solely targets indexActions without any return type documented, neither native nor docblock ones, which I assume are a lot, since static analyzers came out after ZF spreaded.

In terms of what is allowed, MvcEvent::setResult() allows mixed

@weierophinney this is correct, but I'd point out that in a default zend-mvc installation there are no listener handling anything but void|null|array|ViewModel|Response; if a user has custom listener, return types shall be customized and not inherited

At this point I'm not updating this PR code anymore: feel free to push the changes you consider appropriate 👍

[Xerkus]

@Slamdunk well, if we do not define mixed in astract but then add bool as a return type in controller implementation we will be violating return type covariance. I can easily see static analysis tools complaining about that

[Slamdunk]

Yes, it would be correct for static analyzers to complain about a bool: no default zend-mvc listener handle booleans as a return type from controller actions

[weierophinney]

This repository has been closed and moved to laminas/laminas-mvc; a new issue has been opened at laminas/laminas-mvc#5.

[weierophinney]

This repository has been moved to laminas/laminas-mvc. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

• Squash all commits in your branch (git rebase -i origin/{branch})
• Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
• Run the laminas/laminas-migration tool on the code.
• Clone laminas/laminas-mvc to another directory.
• Copy the files from the second bullet point to the clone of laminas/laminas-mvc.
• In your clone of laminas/laminas-mvc, commit the files, push to your fork, and open the new PR.
  We will be providing tooling via laminas/laminas-migration soon to help automate the process.