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.5] change how request only works and remove intersect #18695

Merged
merged 6 commits into from Apr 7, 2017
Merged

[5.5] change how request only works and remove intersect #18695

merged 6 commits into from Apr 7, 2017

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Apr 6, 2017

This will make Request::only() work like Collection::only(). Currently these two methods are inconsistent with each other. This will bring them into harmony so that they behave the same.

Given the following payload:

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

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

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

Prior to this PR, age would have been included in the array returned by only.

@johnRivs
Copy link

johnRivs commented Apr 6, 2017

Don't have any relevant feedback (other than 'I love it') but I wanted to mention how much I love all those red lines in the commit.

😍😍😍

@sixlive
Copy link
Contributor

sixlive commented Apr 6, 2017

I think this makes total sense. Brings consistency and, what I feel, is what you would expect from the only method.

When I build an API that supports PATCH I almost always add a custom macro to add this exact behavior.

Awesome

@skyrpex
Copy link
Contributor

skyrpex commented Apr 6, 2017

I didn't know Request did that. I prefer the behavior that this PR brings to the framework.

@misenhower
Copy link
Contributor

The old behavior can be useful when dealing with checkboxes since an unchecked checkbox will be completely absent from submitted form data (rather than being present and set to false or null, etc.). The old behavior lets you specify all the fields you want to update and ensures they're updated even if they are missing from the form post data.

I like the consistency that this change brings, but it might make certain situations a little more awkward. Is there another method that replicates the old behavior?

@boydcl
Copy link

boydcl commented Apr 6, 2017

Yes please, this is definitely the most logical way the only method should work. The way it works now (before this PR) prevents me from using it especially in handling PATCH requests.

When you think about it it doesn't make sense that a method called 'only' will create new values, it should do no more than filter existing values.

@taylorotwell
Copy link
Member

@misenhower one suggestion I have mentioned internally is to have a pick method which replicates the old behavior to allow people to maintain the old behavior in their applications.

@misenhower
Copy link
Contributor

@taylorotwell That sounds good to me. Would be good to have that on the Collection class as well for consistency 👍

@themsaid
Copy link
Member Author

themsaid commented Apr 6, 2017

Following on @taylorotwell's comment, this will also make it easier for people while upgrading to just replace only() with pick() if they want to keep the old behaviour.


$request = Request::create('/', 'GET', ['developer' => ['name' => 'Taylor', 'age' => 25]]);
$this->assertEquals(['developer' => ['age' => 25]], $request->only('developer.age'));
Copy link
Member

Choose a reason for hiding this comment

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

This also introduces a change in behavior where only can't retrieve nested keys. We may want to still have that functionality.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, will look into that.

Copy link
Member Author

Choose a reason for hiding this comment

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

It now supported nested keys as well.

@laurencei
Copy link
Contributor

laurencei commented Apr 6, 2017

@misenhower - as a side point you can solve your issue another way:

<input type="hidden" name="the_checkbox" value="0" />
<input type="checkbox" name="the_checkbox" value="1" />

That means an unchecked checkbox will still come through your form request as 0 - even though they are not ticked. I use this in all my projects - works flawlessly.

http://stackoverflow.com/a/476581/1317935

Might mean you can refactor your code to use that, rather than refactor to use the new pick().

Ping me on slack if you want to discuss further (dont want to derail this PR)

@juukie
Copy link
Contributor

juukie commented Apr 6, 2017

@laurencei that is a way to solve it but it feels akward to me.

I'd prefer to have the pick() method to be added so I don't have to mess with frontend code. Replacing only() with pick() in the backend will be a lot easier.

@misenhower
Copy link
Contributor

@laurencei Yeah, that's a good option too, I've had to do that in a few situations as well. It works but feels a little hacky (and it adds duplicated fields to query strings if you're submitting via GET). Good to have both options available 😃

@Keoghan
Copy link

Keoghan commented Apr 6, 2017

👍

1 similar comment
@meyyappanv
Copy link

+1

@michaeldyrynda
Copy link
Contributor

Not only consistency within similarly named methods in the framework, but more coherent behaviour. I think it a bit odd that telling a request you want only certain keys, and getting a key that doesn't exist is a bit rough.

👍

@@ -149,8 +149,14 @@ public function only($keys)

$input = $this->all();

$placeholder = str_random();
Copy link
Member

Choose a reason for hiding this comment

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

Cool trick 👍

Incoming HTTP requests would have no way to trick this place-holder or even get lucky.
@taylorotwell
Copy link
Member

taylorotwell commented Apr 7, 2017

Updated the $placeholder to just be an empty anonymous class since HTTP requests have no way to pass in an object like that and we can do a strict equality check. Even though it would be very rare to guess a str_random just feels safer since its now totally impossible.

@JosephSilber
Copy link
Member

JosephSilber commented Apr 7, 2017

$placeholder = new stdClass; should also work, and would make StyleCI happy without another line break.

@pankitgami
Copy link

+1

@themsaid
Copy link
Member Author

themsaid commented Apr 7, 2017

Switched to using stdClass.

@RDelorier
Copy link
Contributor

One day this is going to happen 😌 #15758

@taylorotwell taylorotwell merged commit 542d347 into laravel:master Apr 7, 2017
@OwenMelbz
Copy link
Contributor

Only something to consider from a "social responsibility" point of view.

Recently theres been a surge of tweets/blog posts, video tutorials including ones from @adamwathan suggesting the benefits of "Intersect"

Now removing it will lead to killing lots of recently released blog posts/resources which could confuse newcomers.

@michaeldyrynda
Copy link
Contributor

michaeldyrynda commented Apr 25, 2017

Happens all the time, just means it's time for new blog content!

@ejunker
Copy link
Contributor

ejunker commented Jun 14, 2017

@themsaid are you going to add the pick() method discussed in this issue? It would make it easier for people to upgrade because they could replace their uses of only() with pick() and not have to worry about their code breaking.

@vinterskogen
Copy link
Contributor

@themsaid, are there any solutions of using intersect method with correct behavior in 5.4? In 5.4 intersect method doesn't work properly in come cases - check the issue #20315.

@kz
Copy link

kz commented Jul 31, 2017

@vinterskogen Given the recent changes with this topic, it's best to use a macro and wait for Laravel 5.5. You can find an example of a macro with a person's expected version of intersectOnly here: #15758 (comment)

@peter-dorn
Copy link

peter-dorn commented Aug 22, 2017

Not sure if I'm at the right place, but will this fix this problem as well:
When I pick rules from a FormRequest which contain a validation for an array as well, I get this strange wildcard as key in my output as well. Both for the only() and intersect() method.

$rules = [
            'title' => 'required|min:20',
            'channels' => 'min:1',
            'channels.*.account' => ‘required’
        ];

$request->all()

array:9 [▼
  "_method" => "PUT"
  "_token" => "BFcddbCGLshZtzB75XjjaM1LXAs4707zH49b4Li2"
  "title" => “Some Title“
  "channels" => array:2 [▼
    1 => array:1 [▼
      "account" => "1"
    ]
    2 => array:1 [▼
      "account" => "2"
    ]
  ]
]

$request->intersect(array_keys($rules));

array:5 [▼
  "title" => “Some Title“
  "channels" => array:3 [▼
    1 => array:1 [▼
      "account" => "1"
    ]
    2 => array:1 [▼
      "account" => "2"
    ]
    "*" => array:1 [▼
      "account" => array:2 [▶]
    ]
  ]
]

@arborrow
Copy link

I was cleaning up some code and had been using filled on a request to ensure that it was in fact filled.

    $donation->start_date = $request->filled('start_date') ? $request->input('start_date') : null;

In testing I noticed: Object of class DateTime could not be converted to string because the filled method expects a string.

In my case, I don't see any advantage to using filled. so I'm simply going to remove filled and go with:

    $donation->start_date = $request->input('start_date');

Nevertheless, I would have expected filled to continue to work; however, I suspect that casting start_date as a date in the Donation model explains the change in behavior (previously on the edit resource view I had renamed the form input name to start_date_only).

My question, for those with a greater understanding of how things ought to be, is would it be a helpful tweak or create an issue requesting to modify the filled method to properly handle a DateTime objects or would doing so encourage less than ideal programming? Alternatively, I wonder if an explanation on https://laravel.com/docs/8.x/requests to explain that the filled method will not work with a date would be helpful.

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