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

[5.4] Add Request::pick() #17842

Closed
wants to merge 2 commits into from
Closed

[5.4] Add Request::pick() #17842

wants to merge 2 commits into from

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Feb 9, 2017

The method works like intersect(), however the problem with intersect is that it excludes null, empty, and zero values.

So for example a payload like:

{
    "name": "",
    "category": null,
    "level": 0
}

Output of request()->intersect('name', 'category', 'level', 'age')' will be an empty array.

Output of request()->only('name', 'category', 'level', 'age')' will be:

[
    "name"=> "",
    "category"=> null,
    "level"=> 0,
    "age" => null
]

Output of request()->pick('name', 'category', 'level', 'age')' will be:

[
    "name"=> "",
    "category"=> null,
    "level"=> 0
]

@tillkruss
Copy link
Collaborator

What's the difference between pick() and only()?

@themsaid
Copy link
Member Author

themsaid commented Feb 9, 2017

Output of request()->only('name', 'category', 'level', 'age')' will be:

[
    "description"=> "",
    "category"=> null,
    "level"=> 0,
    "age" => null
]

@JosephSilber
Copy link
Member

Does it really make sense to have 3 different methods that each work slightly differently?


return collect($this->all())->filter(function ($value, $key) use ($keys) {
return in_array($key, $keys);
})->toArray();
Copy link
Member

Choose a reason for hiding this comment

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

How about this?

return array_intersect_key($this->all(), array_flip($keys));

@themsaid
Copy link
Member Author

@JosephSilber I personally think only() should be the only existing method and for it to work like the new pick(), but with the introduction of intersect it looks like every method is best fit for different scenarios.

@JosephSilber
Copy link
Member

@themsaid I agree that we should change the way only works.

@taylorotwell
Copy link
Member

If only is changed what method would be given to provide the old only behavior?

@JosephSilber
Copy link
Member

JosephSilber commented Feb 20, 2017

I personally never cared for the current only method, and struggle to see a use-case at all. Do people really want to reset all of the columns to null if they haven't been passed in???

I do however see many people on SO constantly being frustrated that it works the way it currently does. It's counter-intuitive and in stark contrast with array_only.


If I'm correct that needing the old functionality is an edge case, we don't have to provide a separate method for that. If someone really needs it, they can get similar functionality with this monstrosity:

$keys = ['some', 'keys'];

$data = array_merge(array_fill_keys($keys, null), request()->only($keys));

And since the request is now macro-able, if people need this throughout their project they can create their own method that encapsulates it.


To summarize, I believe:

  • We should only have the only method.
  • Make it work the way pick works in this PR.
  • Get rid of the intersect method.
  • If people need anything else, they should add their own macros.

@srmklive
Copy link
Contributor

I personally think the pick method should be the new only. Remove intersect & old implementation of only.

@taylorotwell
Copy link
Member

So, now we have a new debate over what should be pick, what should be only, and what should be intersect?

@themsaid
Copy link
Member Author

@taylorotwell I think the three of us think that only should act like pick, and we remove intersect. Of course that's in 5.5

@yadakhov
Copy link

yadakhov commented Feb 22, 2017

I like the pick method but I don't think it should be the new only. Doing so will create a lot of backward compatibility issues.

A lot of our code, we safely assume the array index exists after the only is called.

$input = $request->only('username', 'email', 'only');

Now we can safely use these index without worrying about Undefined index error.

$input['username']; 
$input['email']; 
$input['only'];

@JosephSilber
Copy link
Member

@yadakhov you can always add that functionality in a macro, and replace your existing onlys with your new method.

@huglester
Copy link
Contributor

I agree with @yadakhov, why change only() method, when everywhere in the examples we see only() as an example, and now it gets changed?

we often need empty values also, since user can remove data, so if the field value is '' it does not mean anything, and database should be updated.

@JosephSilber
Copy link
Member

@huglester the new only method will include empty strings.

@huglester
Copy link
Contributor

Then I am happy user :)

@amochohan
Copy link
Contributor

This will be very helpful in effectively implementing patch requests.

@taylorotwell
Copy link
Member

So what is our best course here? Should we make request->only behave like request->pick does here for Laravel 5.5? We would maybe need to add a method that works like current request->only() (perhaps that could be request->pick()?) for backwards compatibility.

I'm going to close this on 5.4 as I don't think we'll do anything on the 5.4 release but want to talk about what we should do for 5.5.

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.

8 participants