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

[8.x] Add dd() and dump() to the request object #35384

Merged
merged 1 commit into from
Nov 27, 2020

Conversation

nakov0301
Copy link
Contributor

This PR adds dump() and dd() to the Request class.

Usage

// use Illuminate\Http\Request;
public function index(Request $request)
{
    // instead of
    dd($request->all());

    // we can use
    $request->dd();

    $request->dd(['name', 'age']); // print only the keys from the array

    $request->dd('name', 'age'); // pass them as separate arguments
}

dump() can be used the same way, and it will be useful for debugging AJAX requests.

Real world example

// use Illuminate\Http\Request;
public function index(Request $request)
{
    $request->dd()->validate([
        'name' => 'required'
    ]);
}

Quickly inspect the request params before passing them to the validator.

When using dd() or dump() without params the all() method is used, and when there are params passed in, the only() method is used.

@driesvints driesvints changed the title Add dd() and dump() to the request object [8.x] Add dd() and dump() to the request object Nov 27, 2020
@taylorotwell taylorotwell merged commit 858ff9b into laravel:8.x Nov 27, 2020
@danabrey
Copy link

danabrey commented Dec 4, 2020

I realise it's a bit late to share opinion on this, but FWIW I do think this is an unnecessary and confusing addition.

Firstly, the it shouldn't be the Request's responsibility to dump itself.

Secondly, and more importantly, magically dumping $request->all() instead of $request could and probably will cause more confusion than just making people use dd($request->all()). I wouldn't expect dd($request) and $request->dd() to dump different things, all for the sake of saving 5 characters and losing a whole lot of clarity.

Debugging is a situation where you want to be really sure what you're looking at, and not a place to throw in magic for little return.

@driesvints
Copy link
Member

@danabrey you don't need to use this if you don't want to.

@danabrey
Copy link

danabrey commented Dec 4, 2020

I realise that. I'm just pointing out the possible confusion for users.

@nakov0301
Copy link
Contributor Author

nakov0301 commented Dec 5, 2020

@danabrey I completely agree with your comment above. But this is just a helper function given my "real world" example above that will save you couple of keystrokes.

And I doubt that it will bring confusion as this is not a documented function, having said that in contrary I think that it will help a lot of inexperienced developers to debug their request data easier.

I've been active on SO and Laracasts for the last couple of years, and I can't count the times I've asked developers to debug their request for what it contains, which usually ends in dd($request->all());.
And talking for myself, I've never used dd($request), but always reach for any of the methods on the $request object.

@davisngl
Copy link

davisngl commented Dec 5, 2020

I realise it's a bit late to share opinion on this, but FWIW I do think this is an unnecessary and confusing addition.

Firstly, the it shouldn't be the Request's responsibility to dump itself.

Secondly, and more importantly, magically dumping $request->all() instead of $request could and probably will cause more confusion than just making people use dd($request->all()). I wouldn't expect dd($request) and $request->dd() to dump different things, all for the sake of saving 5 characters and losing a whole lot of clarity.

Debugging is a situation where you want to be really sure what you're looking at, and not a place to throw in magic for little return.

I am pretty sure that someone who is used to coding in Laravel, would always understand what does dd even refer to. It is not like - we only have one way to dump contents of request. We can do it in multiple ways if we forget one way to do it.
Up to you - to use it or not.

@danabrey
Copy link

Up to you - to use it or not.

Is this the slogan of Laravel now?

I doubt that it will bring confusion as this is not a documented function

What's less confusing than undocumented functions that do things people don't expect? :)

@davisngl
Copy link

Up to you - to use it or not.

Is this the slogan of Laravel now?
I think the slogan was "Batteries included but replaceable". It surely implies that it is up to to use it or not. The same about request validation - you use special request validation class or do it "inline" in the method. That's the beauty of this framework.

No one will get hurt if this gets introduced.

@danabrey
Copy link

I'm not going to keep making the same point. Adding end-user confusion is a negative that should be considered - "you don't have to use it" isn't some magic sentence that removes any debate.

Peace out.

@browner12
Copy link
Contributor

I would tend to agree with @danabrey here, we all understand we have the choice to use or not use these methods, we're trying to have a debate about the value of an addition. Let's take the argument ad absurdum for a second. What if I got the following code merged into the Request class:

public function nuke()
{
    DB::statement("DROP TABLE `users`");
}

I can imagine someone commenting that this doesn't belong on the Request object, and is super dangerous because of what it does. Can I just respond, "Don't use it if you don't want to"? No, we should debate the cost/benefit of the addition. This change obviously has a very high cost, and a very low benefit.

In regards to this PR, the proposed benefit is apparently that it saves you a couple of keystrokes. From a purely character count aspect, it looks like we're saving about 5 characters. And with any type of modern IDE and autocompletion, saved characters don't necessarily translate directly to saved keystrokes. Some people may still view this as a benefit, but I'm hard pressed to see it that way.

As to the cost of this addition, we now have an additional 36 lines of code to maintain in the codebase, a little more visual debt on the class, and (possibly) slightly more memory when the object is used. The weight you assign to these costs is obviously subjective. I personally probably assign a little higher cost than others.

Every line of code is always a trade off, with a cost and a benefit. We need to be willing to look at both when having a discussion about PRs.

This is not a hill I have any interest in dying on. At the end of the day, yes, this PR is relatively low impact, and people can just avoid using it if they don't like it. But I don't think we should shut the conversation down immediately. We're all working towards the same goal, and those of us who raise concerns are just doing so to make sure we acknowledge the short term and long term effects of these changes.

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.

7 participants