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

[9.x] Added has and missing methods to ValidatedInput #42184

Merged
merged 7 commits into from
May 3, 2022
Merged

[9.x] Added has and missing methods to ValidatedInput #42184

merged 7 commits into from
May 3, 2022

Conversation

Sammyjo20
Copy link
Contributor

@Sammyjo20 Sammyjo20 commented Apr 29, 2022

This PR introduces two new methods to the Illuminate\Support\ValidatedInput class.

  • has($keys)
  • missing($keys)

These two methods are used to check the existence of validated input easily. For example, now with the $request->safe() method, I could now do:

$validatedName = $request->safe()->has('name'); // True
$validatedAge = $request->safe()->has('age') // False

$validatedName = $request->safe()->missing('name'); // False
$validatedAge = $request->safe()->missing('age') // True

You can also provide an array, and it will validate each key...

$validatedName = $request->safe()->has(['name', 'age']);

@Sammyjo20
Copy link
Contributor Author

Is the updated correct, Dries?

@Sammyjo20
Copy link
Contributor Author

I've also updated the other instances of array|mixed in the file for you 👍🏼

Copy link
Contributor Author

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

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

Self review complete

@driesvints driesvints changed the title [9.x] Added has and doesntHave methods to ValidatedInput. [9.x] Added has and doesntHave methods to ValidatedInput Apr 29, 2022
@Sammyjo20
Copy link
Contributor Author

Sammyjo20 commented Apr 29, 2022

I also wanted to add a method to the FormRequest class, like $request->hasValidated($keys) or $request->validated($keys) but I wasn't sure if you guys would like that, what do you think?

* @param mixed $keys
* @return bool
*/
public function doesntHave($keys)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like missing is more Laravel-ish

Copy link
Contributor

Choose a reason for hiding this comment

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

I had the same thought, missing() is used as the method name when interacting with a plain request() instance.

https://github.com/laravel/framework/blob/9.x/src/Illuminate/Http/Concerns/InteractsWithInput.php#L220

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll get that updated 👍🏼

Copy link
Contributor Author

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

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

I have changed the doesntHave to missing as I do agree it sounds more like Laravel's language, so I will update the description of the PR.

@Sammyjo20 Sammyjo20 changed the title [9.x] Added has and doesntHave methods to ValidatedInput [9.x] Added has and missing methods to ValidatedInput May 1, 2022
@Sammyjo20
Copy link
Contributor Author

Sammyjo20 commented May 1, 2022

Hey guys, I'm just thinking - does this PR add value to the framework? I could have sworn I recently had a use-case where I wanted to check if something was validated. Surely if the data was provided, it has to be validated. This does double check that it was provided AND valid, but not sure if that's really helping.

Could this be sufficient, $request->has('input') / $request->filled('input')? Again, if the input has been provided it must have been validated.

I think it may have been because I was conditionally applying the rule in my rules() array. In that instance, it would be useful.

Edit; Also come to think of it, if people are using the validator outside of the FormRequest class, this would be very useful for people.

@taylorotwell
Copy link
Member

@Sammyjo20 one thing I notice is the there could be a slight behavior difference between request->has and validatedInput->has... you use array_key_exists while request->has uses Arr::has under the hood. Thoughts?

@taylorotwell taylorotwell marked this pull request as draft May 2, 2022 14:27
@taylorotwell
Copy link
Member

Please mark as ready for review after responding. Thanks.

@Sammyjo20
Copy link
Contributor Author

Sammyjo20 commented May 2, 2022

Ah yes, good spot. I have switched it to use Arr::has() like the InteractsWithInput trait has.

From InteractsWithInput:

public function has($key)
{
    $keys = is_array($key) ? $key : func_get_args();

    $input = $this->all();

    foreach ($keys as $value) {
        if (! Arr::has($input, $value)) {
            return false;
        }
    }

    return true;
}

@Sammyjo20 Sammyjo20 marked this pull request as ready for review May 2, 2022 20:57
@taylorotwell taylorotwell merged commit 4768e94 into laravel:9.x May 3, 2022
@matiaslauriti
Copy link
Contributor

matiaslauriti commented Jul 7, 2022

May I ask why is Laravel not requiring a type-hinted return?

For example, the has and missing could easily have : bool at the end:

public function has($key): bool;

public function missing($key): bool;

Why I am saying so? I see other tests and author's tests are using assertEquals for booleans, which means that if has or missing starts returning something that is not true or false, the tests would not fail nor the method itself would throw a PHP Fatal (allowing the Laravel team to fix this on a test).

public function has($keys)
{
    return null; // Example
}

$this->assertEquals(false, $inputA->missing('name')); // This passes because false == null

The fix would be to start using assertSame (valueA === valueB) and type-hint the return value (because we can in this case and we are allowing PHP 8.0+ on composer for Laravel 9.x).

public function has($keys): bool;

$this->assertSame(false, $inputA->missing('name'));

So, is there a why we are not doing so? I am asking so when I contribute in the near future I am already aware of this 👍

@matiaslauriti
Copy link
Contributor

@driesvints

@Sammyjo20
Copy link
Contributor Author

@matiaslauriti I was just keeping it consistent with the rest of the test. I assume it was written a while ago. I’d rather keep it consistent than make lots more changes to have to be reviewed. I agree though I prefer “assetTrue” and useful methods like that.

@driesvints
Copy link
Member

driesvints commented Aug 1, 2022

We don't use return types right now in the codebase. We have no plans right now to add any.

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.

6 participants