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 support for request helper #145

Merged
merged 1 commit into from
Sep 25, 2018
Merged

Conversation

j3j5
Copy link
Contributor

@j3j5 j3j5 commented Sep 25, 2018

Add support for the request() helper.

This is my first significant contribution to phpstan/larastan so all feedback is welcome.

@szepeviktor
Copy link
Collaborator

Looks very nice. Does it work?
BTW Larastan's code is checked by PHPStan ;-)

@j3j5
Copy link
Contributor Author

j3j5 commented Sep 25, 2018

I've tested it on one of my projects where I was getting all those errors Cannot call method input() on array|Illuminate\Http\Request|string when using request()->input('key') and it works, but it never hurts to have more eyeballs look at it :)

@szepeviktor szepeviktor merged commit 6e8513b into larastan:master Sep 25, 2018
@szepeviktor
Copy link
Collaborator

Thank you for your contribution!

@j3j5 j3j5 deleted the request_helper branch September 25, 2018 20:52
Copy link
Collaborator

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

@j3j5 Thank you so much for this contribution, do you mind of address my feedback?

}

if (isset($functionCall->args[0]->value) && $functionCall->args[0]->value instanceof Array_) {
return new ArrayType(new MixedType(), new MixedType());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be only return new ArrayType() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the constructor I didn't think it was possible, but I just tested it and it works. What are the values of Type $keyType and Type $itemType when you construct the object w/o arguments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

When $key is an array the request helper returns the result of the method only:

        if (is_array($key)) {
            return app('request')->only($key);
        }

And the only method returns an array:

    /**
     * Get a subset containing the provided keys with values from the input data.
     *
     * @param  array|mixed  $keys
     * @return array
     */
    public function only($keys)

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, that I understand, that's why I returned a PHPStan\Type\ArrayType in the first place. But looking at the source code of that class, I interpreted (wrongly it seems) that the constructor needed 2 arguments.

public function __construct(Type $keyType, Type $itemType)
{
	if ($keyType->describe(VerbosityLevel::value()) === '(int|string)') {
		$keyType = new MixedType();
	}
	$this->keyType = $keyType;
	$this->itemType = $itemType;
}

I looked at PHPStan's source code and I copypasted that one, thinking it made sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/ReturnTypes/Helpers/RequestExtension.php Show resolved Hide resolved
@j3j5
Copy link
Contributor Author

j3j5 commented Sep 25, 2018

I've pushed to the original branch, not sure how GH will handle now that the PR is already merged. Shall I submit another one?

@nunomaduro
Copy link
Collaborator

@j3j5 I think you can submit a new PR.

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.

None yet

3 participants