[12.x] feat: add native return types to helper functions

[calebdw]

Hello!

This PR adds native return types to the helper functions (where the return type would not be mixed).

Note

Functions cannot be extended so there should not be any breaks

Motivation

I've been using Rector to automatically upgrade / refactor my codebase---it has the ability to automatically add types, narrow too wide types, etc. However, in most cases it only uses native types to be completely safe during it's refactors---it skips over function calls like these that don't have native return types.

Adding actual return types like this will allow Rector (and other tools) to better understand the return type and be more safe.

Thanks!

[rodrigopedra]

Why did you change an unrelated test?

[calebdw]

It's not an "unrelated" test---the test was failing with a type error because the helper function was returning a mocked stdClass instead of a mocked ViewFactory like it should have to begin with

[rodrigopedra]

It is unrelated, as the PR title and description fail to mention it and only talk about fixing return types on helpers.

That should have been sent on a separate PR.

[calebdw]

I disagree, but what's done is done

[browner12]

could we also safely type the parameters here?

[calebdw]

could we also safely type the parameters here?

That's less safe because the user could be passing in bad data (e.g., could result in Argument #1 ($foo) must be of type string, null given type errors)

[chris-ware]

Interestingly, this seems to cause an issue with the csrf_token helper method. There appears to be some instances where it doesn't return a string, which now causes a fatal error due to it not being type matched.

[calebdw]

let me take a look

[shaedrich]

Functions cannot be extended so there should not be any breaks

Unfortunately, this creates a bug when you do, for example:

return response(null, Response::HTTP_INTERNAL_SERVER_ERROR);

which results in "Expected type 'Symfony\Component\HttpFoundation\Response'. Found 'Illuminate\Contracts\Routing\ResponseFactory'" which is not true, since the condition is not

framework/src/Illuminate/Foundation/helpers.php

Line 847 in 6e738d8

* @return ($content is null ? \Illuminate\Contracts\Routing\ResponseFactory : \Illuminate\Http\Response)

as hinted in the PHPStan conditional return types, but

framework/src/Illuminate/Foundation/helpers.php

Line 853 in 6e738d8

if (func_num_args() === 0) {

in runtime code, which is not the same when you pass null for the first parameter but something as the second.

[calebdw]

That is a phpstan limitation, not a runtime error, can just ignore

[shaedrich]

Yeah, it's not a runtime error, sure, but it's not PHPStan's fault. Granted, PHPStan can't do func_num_args() === 0 in conditional returns, but the conditional return that this PR introduced is factually wrong. It doesn't reflect the runtime code accurately.

[calebdw]

See phpstan/phpstan#11245 for reference, this is "good enough" to detect the correct return type when you call response(). You can just as call response([], Response::HTTP_INTERNAL_SERVER_ERROR) or even abort(500) instead to prevent the warning

[shaedrich]

If you insist on the PHPDoc, you should change the runtime code as it allows for something you would not.

[calebdw]

No, this PR did not touch the conditional return and the conditional return is good enough for most cases.

Larastan already has an extension for this helper. The fact that you're getting this warning indicates that you're not using it.