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

[VarDumper] Make `dump()` a little bit more easier to use #24280

Closed
wants to merge 3 commits into
base: master
from

Conversation

@freekmurze
Contributor

freekmurze commented Sep 21, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes

Imagine you have this code:

$object->method();

If you want to dump the object, this is currently the way to do that:

dump($object);

$object->method();

This PR makes adding (and removing) the dump function easier. When using one argument is used, that argument is returned.

dump($object)->method();

When dumping multiple things, they all get returned. So you can to this:

$object->someMethod(...dump($arg1, $arg2, $arg3));
Make `dump()` a slightly bit more developer friendly
Imagine you have this code:

```php
$object->method();
```

If you want to dump the object, this is currently the way to do that:

```php
dump($object);

$object->method();
```

This PR makes adding (and removing) the `dump` function easier.

```php
dump($object)->method();
```

@freekmurze freekmurze changed the title from Make `dump()` a little bit more easier to use to [VarDumper] Make `dump()` a little bit more easier to use Sep 21, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 21, 2017

Member

I wanted that yesterday, so definitely yes :)
Should target 3.4 when merging.

Member

nicolas-grekas commented Sep 21, 2017

I wanted that yesterday, so definitely yes :)
Should target 3.4 when merging.

@javiereguiluz

Nice change! @freekmurze thanks (and congrats!) for your first contribution to Symfony!! 🎉

@Taluu

This comment has been minimized.

Show comment
Hide comment
@Taluu

Taluu Sep 21, 2017

Contributor

This is wrong, because if I do a dump($object, $foo, $bar), I know we will have $object but from an external point of view, this is not so obvious...

Contributor

Taluu commented Sep 21, 2017

This is wrong, because if I do a dump($object, $foo, $bar), I know we will have $object but from an external point of view, this is not so obvious...

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 21, 2017

Contributor

@Taluu in that case the last argument is the var that is returned ;)

dump($string, $float, $object)->method();
Contributor

yceruto commented Sep 21, 2017

@Taluu in that case the last argument is the var that is returned ;)

dump($string, $float, $object)->method();
@ogizanagi

Nice 👍

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire Sep 21, 2017

Contributor

@Taluu in that case the last argument is the var that is returned ;)

Which means it is even less obvious than you think it was... maybe return func_get_args() or null in this case, so that it can't be misused?

Contributor

greg0ire commented Sep 21, 2017

@Taluu in that case the last argument is the var that is returned ;)

Which means it is even less obvious than you think it was... maybe return func_get_args() or null in this case, so that it can't be misused?

@Taluu

This comment has been minimized.

Show comment
Hide comment
@Taluu

Taluu Sep 21, 2017

Contributor

That's the same thing (I read too fast), but still. If I am using echo, var_dump, error_log and so on, I am not expecting those to "return" the (last ?) argument. And returning all the args is not really intuitive too IMO.

This is making something fluent just for the sake of making it fluent : https://ocramius.github.io/blog/fluent-interfaces-are-evil/

Contributor

Taluu commented Sep 21, 2017

That's the same thing (I read too fast), but still. If I am using echo, var_dump, error_log and so on, I am not expecting those to "return" the (last ?) argument. And returning all the args is not really intuitive too IMO.

This is making something fluent just for the sake of making it fluent : https://ocramius.github.io/blog/fluent-interfaces-are-evil/

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 21, 2017

Member

This is just about easing inspecting some vars when developing. It's not just for the sake of making it fluent. Thanks to this, you just have to wrap a variable or an expression instead of writing a new line and possibly executing the same piece of code twice.

This is wrong, because if I do a dump($object, $foo, $bar), I know we will have $object but from an external point of view, this is not so obvious...

maybe return func_get_args() or null in this case, so that it can't be misused?

Returning func_get_args() would defeat the purpose of this, as you won't be able to chain.
What it returns in case of multiple arguments "does not have to be particularly obvious when reading", because you're not likely to read that code anyway: you generally are the one writing this code for debugging purpose so it won't remains. Either you know about the feature, how it behaves and use it, either you don't and keep dumping around the old way.

Member

ogizanagi commented Sep 21, 2017

This is just about easing inspecting some vars when developing. It's not just for the sake of making it fluent. Thanks to this, you just have to wrap a variable or an expression instead of writing a new line and possibly executing the same piece of code twice.

This is wrong, because if I do a dump($object, $foo, $bar), I know we will have $object but from an external point of view, this is not so obvious...

maybe return func_get_args() or null in this case, so that it can't be misused?

Returning func_get_args() would defeat the purpose of this, as you won't be able to chain.
What it returns in case of multiple arguments "does not have to be particularly obvious when reading", because you're not likely to read that code anyway: you generally are the one writing this code for debugging purpose so it won't remains. Either you know about the feature, how it behaves and use it, either you don't and keep dumping around the old way.

@Taluu

This comment has been minimized.

Show comment
Hide comment
@Taluu

Taluu Sep 21, 2017

Contributor

For the func_get_args() I agree, but on the other thing (returning something), I disagree.

Something that would be more convenient IMO would be to yield each value on multiple args, rather than arbitrarily returning the last arg

Contributor

Taluu commented Sep 21, 2017

For the func_get_args() I agree, but on the other thing (returning something), I disagree.

Something that would be more convenient IMO would be to yield each value on multiple args, rather than arbitrarily returning the last arg

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 21, 2017

Member

But what would be the usefulness of this in practice, appart from the debatable less arbitrary nature of the returned value? How would it be convenient? It'll return a generator and won't allow chaining, so again defeats the purpose of this PR.

Member

ogizanagi commented Sep 21, 2017

But what would be the usefulness of this in practice, appart from the debatable less arbitrary nature of the returned value? How would it be convenient? It'll return a generator and won't allow chaining, so again defeats the purpose of this PR.

@franzliedke

This comment has been minimized.

Show comment
Hide comment
@franzliedke

franzliedke Sep 22, 2017

Contributor

When you dump multiple values, you're probably not using them inline anyways, so the benefit of simply adding and removing dump() around the variable would not be existent anyways...

Contributor

franzliedke commented Sep 22, 2017

When you dump multiple values, you're probably not using them inline anyways, so the benefit of simply adding and removing dump() around the variable would not be existent anyways...

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire Sep 22, 2017

Contributor

Returning func_get_args() would defeat the purpose of this, as you won't be able to chain.

I wrote "in that case", meaning when you pass multiple vars.

Contributor

greg0ire commented Sep 22, 2017

Returning func_get_args() would defeat the purpose of this, as you won't be able to chain.

I wrote "in that case", meaning when you pass multiple vars.

@JulienTant

This comment has been minimized.

Show comment
Hide comment
@JulienTant

JulienTant Sep 22, 2017

Don't forget that the goal of this PR is it so make dump($object)->method(); works.

We can accept that this PR is a first step, and create new PRs later to improve the behavior of what's returned in the case of multiple arguments ?

JulienTant commented Sep 22, 2017

Don't forget that the goal of this PR is it so make dump($object)->method(); works.

We can accept that this PR is a first step, and create new PRs later to improve the behavior of what's returned in the case of multiple arguments ?

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 22, 2017

Member

@greg0ire you cannot make a function become a generator conditionally, as this is determined at compile time (based on the presence of a yield in it).

Member

stof commented Sep 22, 2017

@greg0ire you cannot make a function become a generator conditionally, as this is determined at compile time (based on the presence of a yield in it).

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire Sep 22, 2017

Contributor

TIL 💡 , but I think you meant to answer to @Taluu

Contributor

greg0ire commented Sep 22, 2017

TIL 💡 , but I think you meant to answer to @Taluu

@greg0ire

Very useful for people who hate one-time variables like I do

@hannesvdvreken

This comment has been minimized.

Show comment
Hide comment
@hannesvdvreken

hannesvdvreken Sep 22, 2017

We can have it return the exact same list of things of course..

if ($moreVars) {
    return func_get_args();
}

return $var;

and then we could do both

dump($stuff)->someMethod();

and:

$object->someMethod(...dump($arg1, $arg2, $arg3));

imho: it would be a bit awkward to do

dump($foo, $bar, $object)->methodOnObject();

The goal is to dump some variables and make your application still work, kind of, without having to do too many keystrokes.

hannesvdvreken commented Sep 22, 2017

We can have it return the exact same list of things of course..

if ($moreVars) {
    return func_get_args();
}

return $var;

and then we could do both

dump($stuff)->someMethod();

and:

$object->someMethod(...dump($arg1, $arg2, $arg3));

imho: it would be a bit awkward to do

dump($foo, $bar, $object)->methodOnObject();

The goal is to dump some variables and make your application still work, kind of, without having to do too many keystrokes.

@freekmurze

This comment has been minimized.

Show comment
Hide comment
@freekmurze

freekmurze Sep 22, 2017

Contributor

I really like what @hannesvdvreken proposed. I'll update this PR.

Contributor

freekmurze commented Sep 22, 2017

I really like what @hannesvdvreken proposed. I'll update this PR.

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire Sep 22, 2017

Contributor

That's exactly what I proposed BTW :P Although I didn't think about the unpacking at the time, good thinking @hannesvdvreken !

Contributor

greg0ire commented Sep 22, 2017

That's exactly what I proposed BTW :P Although I didn't think about the unpacking at the time, good thinking @hannesvdvreken !

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas
Member

nicolas-grekas commented Sep 23, 2017

Thank you @freekmurze.

nicolas-grekas added a commit that referenced this pull request Sep 23, 2017

feature #24280 [VarDumper] Make `dump()` a little bit more easier to …
…use (freekmurze)

This PR was submitted for the master branch but it was squashed and merged into the 3.4 branch instead (closes #24280).

Discussion
----------

[VarDumper] Make `dump()` a little bit more easier to use

| Q             | A
| ------------- | ---
| Branch?       |  3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes

Imagine you have this code:

```php
$object->method();
```

If you want to dump the object, this is currently the way to do that:

```php
dump($object);

$object->method();
```

This PR makes adding (and removing) the `dump` function easier. When using one argument is used, that argument is returned.

```php
dump($object)->method();
```

When dumping multiple things, they all get returned. So you can to this:

```php
$object->someMethod(...dump($arg1, $arg2, $arg3));
```

Commits
-------

42f0984 [VarDumper] Make `dump()` a little bit more easier to use

This was referenced Oct 18, 2017

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Dec 20, 2017

Member

Hello.

I think this is a BC break. Now we can not die(dump()) Anymore.
Exemple:

>[/tmp/toto] cat index.php
<?php

require __DIR__.'/vendor/autoload.php';

die(dump(new datetime()));
>[/tmp/toto] php -n index.php 
DateTime @1513766992 {#3
  date: 2017-12-20 11:49:52.615055 Europe/Berlin (+01:00)
}

Recoverable fatal error: Object of class DateTime could not be converted to string in /tmp/toto/index.php on line 5
Member

lyrixx commented Dec 20, 2017

Hello.

I think this is a BC break. Now we can not die(dump()) Anymore.
Exemple:

>[/tmp/toto] cat index.php
<?php

require __DIR__.'/vendor/autoload.php';

die(dump(new datetime()));
>[/tmp/toto] php -n index.php 
DateTime @1513766992 {#3
  date: 2017-12-20 11:49:52.615055 Europe/Berlin (+01:00)
}

Recoverable fatal error: Object of class DateTime could not be converted to string in /tmp/toto/index.php on line 5
@hannesvdvreken

This comment has been minimized.

Show comment
Hide comment
@hannesvdvreken

hannesvdvreken Dec 20, 2017

Oh true. Before it would return null, and thus you would exit (or die) with null die(null).

This will require a small change to dump($stuff); die; instead.

At least you won't run die(dump()); in production, right ;-)

hannesvdvreken commented Dec 20, 2017

Oh true. Before it would return null, and thus you would exit (or die) with null die(null).

This will require a small change to dump($stuff); die; instead.

At least you won't run die(dump()); in production, right ;-)

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Dec 20, 2017

Member

Yeah I know why it is not working anymore ;)
I don't know if this BC break is covered by our BC promise

Member

lyrixx commented Dec 20, 2017

Yeah I know why it is not working anymore ;)
I don't know if this BC break is covered by our BC promise

@hannesvdvreken

This comment has been minimized.

Show comment
Hide comment
@hannesvdvreken

hannesvdvreken Dec 20, 2017

where can we read more about that?

hannesvdvreken commented Dec 20, 2017

where can we read more about that?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Dec 20, 2017

Member

dump() is not covered by the BC policy, it's not part of any "API", it's just a temporary helper.
If you need BC, VarDumper::dump() should be used instead.

Member

nicolas-grekas commented Dec 20, 2017

dump() is not covered by the BC policy, it's not part of any "API", it's just a temporary helper.
If you need BC, VarDumper::dump() should be used instead.

@michielvandenboogaart

This comment has been minimized.

Show comment
Hide comment
@michielvandenboogaart

michielvandenboogaart Jan 31, 2018

It's a shame. This PR has broken the dump function since it both outputs and returns now. This whole functionality should have been on a different function or something.

R.I.P. dump()

michielvandenboogaart commented Jan 31, 2018

It's a shame. This PR has broken the dump function since it both outputs and returns now. This whole functionality should have been on a different function or something.

R.I.P. dump()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment