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

[11.x] Adds Dumpable concern #47122

Merged
merged 26 commits into from
May 18, 2023
Merged

[11.x] Adds Dumpable concern #47122

merged 26 commits into from
May 18, 2023

Conversation

nunomaduro
Copy link
Member

@nunomaduro nunomaduro commented May 17, 2023

This pull request is currently under development and introduces a fresh Dumpable concern (or trait), which replaces the existing dd and dump methods in a majority of our classes. Furthermore, users and package authors have the option to include this concern in their own code easily, thereby including these debugging methods in their classes:

<?php

namespace App\ValueObjects;

use Illuminate\Support\Traits\Conditionable;
use Illuminate\Support\Traits\Dumpable;

class Address
{
    use Conditionable, Dumpable;

    // ...
}
$address = new Address;

-$address->setThis()->setThat();
+$address->setThis()->dd()->setThat();

Please take note that this pull request is specifically aimed at Laravel 11.x. As it involves modifications to the signature of certain methods. To ensure consistency across the entire code base, the addition of ...$args to these debugging methods was added. For the sake of precaution, I would prefer to target the 11.x version and explicitly mention this in the upgrade guide.

@nunomaduro nunomaduro changed the base branch from 10.x to master May 17, 2023 20:03
@nunomaduro nunomaduro changed the title [10.x] Adds Dumpable concern [11.x] Adds Dumpable concern May 17, 2023
@nunomaduro nunomaduro marked this pull request as ready for review May 17, 2023 21:15
@felixdorn
Copy link
Contributor

felixdorn commented May 17, 2023

edited: What about a way to customize what gets dumped without having to override the dump method?

-public function dump(...$args) {
-     dump($this->something, ...$args);
-
-    return $this;
-}
+public function toDump() {
+     return $this->something;
+}

@valorin
Copy link
Contributor

valorin commented May 18, 2023

What about a way to customize what gets dumped ? Often, you don't want to dump the object itself but a property or the result of a function (because dd(), dump() are very useful when using some sort of builder pattern that usually store what they're building in an array).

This is my biggest annoyance when dumping models with dump() and dd(). The output is full of so much boilerplate that you want to ignore 99% of the time, with the actual model contents buried on a second level. I'm forever doing dump($model->toArray()). So I'd love a way to customize the dump output. Ideally have models always use a simpler layout that exposes the actual attributes over the metadata properties.

As for naming, what about toDump(), to match toString() and toArray(), etc?

@felixdorn
Copy link
Contributor

This is my biggest annoyance when dumping models with dump() and dd(). The output is full of so much boilerplate that you want to ignore 99% of the time, with the actual model contents buried on a second level.

FYI, this package does just that: https://github.com/glhd/laravel-dumper (I don't know how well it works / is annoying, never used it)

Comment on lines +13 to +18
public function dd(...$args)
{
$this->dump(...$args);

dd();
}

Choose a reason for hiding this comment

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

Suggested change
public function dd(...$args)
{
$this->dump(...$args);
dd();
}
public function dd(...$args)
{
dd(...$args);
}

@taylorotwell taylorotwell merged commit 4326fc3 into master May 18, 2023
@taylorotwell taylorotwell deleted the feat/dumpable branch May 18, 2023 13:52
@BafS
Copy link
Contributor

BafS commented May 21, 2023

What is... the point? dd($foo) is not only shorter but it doesn't introduce yet another trait to the class. Now we have dd and dump to bloat the public API even more (you know, composition over inheritance and KISS), dump should not be used in prod.

@BafS
Copy link
Contributor

BafS commented May 30, 2023

@taylorotwell, @nunomaduro looking at how much upvotes the last comment has, it would be great to reconsider it or to have more feedback

@adevade
Copy link
Contributor

adevade commented Feb 5, 2024

@BafS Well, the methods already existed on all these classes. The PR only cleans up the code base by moving them to a single file instead of having to redefine them on each class.

@BafS
Copy link
Contributor

BafS commented Feb 5, 2024

That's true but having a trait means opening the door to have many classes implementing it, plugins or libraries using the illuminate "support" (OP wrote "Furthermore, users and package authors have the option to include this concern in their own code easily"). It doesn't bring anything and it's painful to see a famous framework bloating classes with traits, even more when it's debug only.

@GitHub30
Copy link

@BafS わかりみが深い

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.

9 participants