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

AbstractRestfulController::patch returns array, not mixed #114

Closed
gregtyler opened this issue Apr 21, 2022 · 3 comments · Fixed by #115
Closed

AbstractRestfulController::patch returns array, not mixed #114

gregtyler opened this issue Apr 21, 2022 · 3 comments · Fixed by #115
Labels
Bug Something isn't working
Milestone

Comments

@gregtyler
Copy link
Contributor

Bug Report

Q A
Version(s) 3.3.3

Summary

AbstractRestfulController::patch has a typehint of @return array rather than @return mixed (which the other actions have).

Current behavior

This issue is highlighted by static analysis tools like PHPStan when you extend the class. If you try to set a return type of e.g. JsonModel or ResponseInterface, PHPStan (rightly) complains that the return type isn't compatible.

How to reproduce

N/A, it's just documentation

Expected behavior

I think it should return mixed, like the other methods. I'm happy to create a PR for this, but wanted to check I hadn't missed something first.

@gregtyler gregtyler added the Bug Something isn't working label Apr 21, 2022
@Ocramius
Copy link
Member

Changing the return type to mixed may break BC here, as it would mean that callers can expect anything, whereas they previously would expect array 🤔

@Xerkus
Copy link
Member

Xerkus commented Apr 21, 2022

Following intended use of the return values in AbstractController and its descendants, array was never a valid return type as it is expected that ViewModel would be returned, with array just a convenience shortcut for ViewModel, while Response object as a return value would short circuit controller's dispatch event and would prevent render event entirely.

With that in mind, this is not a BC break.

@gregtyler
Copy link
Contributor Author

Thanks both, with that in mind I've created #115 :)

@Xerkus Xerkus modified the milestones: 3.3.4, 3.4.0 Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants