Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Slamdunk
Copy link
Contributor

commented Jun 7, 2019

  • 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: Slamdunk/phpstan-zend-framework#5

@Slamdunk

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

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

@thomasvargiu

This comment has been minimized.

Copy link

commented Jun 11, 2019

@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

This comment has been minimized.

Copy link

commented Jun 11, 2019

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

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

This comment has been minimized.

Copy link
@thomasvargiu

thomasvargiu Jun 11, 2019

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

This comment has been minimized.

Copy link
@Slamdunk

Slamdunk Jun 11, 2019

Author Contributor
  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

This comment has been minimized.

Copy link
@thomasvargiu

thomasvargiu Jun 11, 2019

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.

This comment has been minimized.

Copy link
@Slamdunk

Slamdunk Jun 11, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@weierophinney

weierophinney Jun 11, 2019

Member

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.

This comment has been minimized.

Copy link
@Xerkus

Xerkus Jun 11, 2019

Member

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?

This comment has been minimized.

Copy link
@weierophinney

weierophinney Jun 11, 2019

Member

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

This comment has been minimized.

Copy link
@Slamdunk

Slamdunk Jun 12, 2019

Author Contributor

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 👍

This comment has been minimized.

Copy link
@Xerkus

Xerkus Jun 12, 2019

Member

@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

This comment has been minimized.

Copy link
@Slamdunk

Slamdunk Jun 12, 2019

Author Contributor

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

@Xerkus Xerkus self-assigned this Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.