Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

Updated AbstractActionController::indexAction return type to reflect real usage #312

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Updated AbstractActionController::indexAction return type to reflect real usage #312

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00112d7
Updated AbstractActionController::indexAction return type to reflect …
Slamdunk Jun 7, 2019
9bfe2bf
DefaultRenderingStrategy handles Zend\View\Model\ModelInterface
Slamdunk Jun 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Controller/AbstractActionController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

use Zend\Mvc\Exception;
use Zend\Mvc\MvcEvent;
use Zend\Stdlib\ResponseInterface as Response;
use Zend\View\Model\ViewModel;

/**
Expand All @@ -24,7 +25,7 @@ abstract class AbstractActionController extends AbstractController
/**
* Default action if none provided
*
* @return ViewModel
* @return null|array|ViewModel|Response
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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()).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*/
public function indexAction()
{
Expand Down