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

Add typehints in favour of PHPDocs for TransformsRequest middleware #28373

Closed
wants to merge 1 commit into from

Conversation

DannyvdSluijs
Copy link

As a minor improvement (and since the project is only targeting >= PHP 7.1) I've updated the TransformsRequest middleware to use the type hints where possible and removing the (redundant) @params and @return.
This PR doesn't touch the cleanValue method as it is overloaded and contains support for mixed types.

Feel free to reply if this PR needs rework (Since this is my 1st PR on this project)

@DannyvdSluijs DannyvdSluijs changed the title Add typehints in favour of PHPDocs Add typehints in favour of PHPDocs for TransformsRequest middleware Apr 30, 2019
@devcircus
Copy link
Contributor

The framework doesn't use return typehints at this time. Doc blocks are preferred. While this may change in the future, this file should be consistent with the rest of the framework.

@driesvints
Copy link
Member

Heya, thanks for sending in your first PR. Please see the comment above. At the moment we don't implement these type-hints in the framework.

@driesvints driesvints closed this Apr 30, 2019
@DannyvdSluijs
Copy link
Author

Thnx for the feedback. However Im trying to wrap my head around why not to add these basic typehints (Especially the object type hints have been there since PHP 5, July 2004). Also reading laravel/ideas#1409 where the majority seems to be on the pro side. But there seems to be little progress on a feature which I consider very positive for strengthening the framework and assist the programmer. As currently the responsibility lies with the programmer using the framework. I would expect the framework to help.

Btw there seems to be some typehints in the framework: https://github.com/laravel/framework/blob/5.8/src/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.php#L55

@mfn
Copy link
Contributor

mfn commented Apr 30, 2019

why not

I think these two things are relevant:

Basically Taylor so far hasn't gone forward merging such PRs which shows his stance is (not yet?) "pro" them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants