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

Dumper: use __debugInfo if present by default to export object #135

Closed
wants to merge 1 commit into from
Closed

Dumper: use __debugInfo if present by default to export object #135

wants to merge 1 commit into from

Conversation

vojtech-dobes
Copy link
Contributor

This is resurrection of nette/nette#1415.

Tests are added.

I've also added possibility to switch this off with Dumper::DEBUGINFO => FALSE. I've chosen TRUE as default because it feels to me not only as new feature, but rather matching default behavior of PHP. Last point: I've enabled it only for PHP >=5.6 where this magic method is actually supported by PHP.

@hrach
Copy link
Contributor

hrach commented Feb 11, 2016

The current master is PHP 5.6+ only, so I think the condition may be removed :)

@vojtech-dobes
Copy link
Contributor Author

The current master is PHP 5.6+ only, so I think the condition may be removed :)

Ah, indeed, good point :). I will gladly do that.

@vojtech-dobes vojtech-dobes changed the title Dumper: use __debugInfo in PHP >=5.6 by default if present Dumper: use __debugInfo if present by default to export object Feb 11, 2016
@dg
Copy link
Member

dg commented Feb 11, 2016

Uses __debugInfo anyone?

@vojtech-dobes
Copy link
Contributor Author

Uses __debugInfo anyone?

We do, can't speak for anyone else :).

@staabm
Copy link
Contributor

staabm commented Feb 11, 2016

For aerys we also use it.

@enumag
Copy link
Contributor

enumag commented Feb 11, 2016

I certainly will when this is merged.

@JanTvrdik
Copy link
Contributor

👎 On using debug info by default. The only real experience with debug info I have is that it used to cause some strange failures / crashes, not sure whether this has been fixed in PHP 7.

@hrach
Copy link
Contributor

hrach commented Feb 12, 2016

Personally, I'd love to see the support for this. The mentioned php bug is a little problem here, so I can imagine this as a non-default bahavior. Would be great to be able enable/disable this globally.

@fprochazka
Copy link
Contributor

I think making objects dump their guts is violation of SRP, in the same way adding a public getter to be able to test the state of object is.

Imho the right way to solve this is this nette/nette#1090 (comment)


Are there some native classes, that have this method implemented?

@vojtech-dobes
Copy link
Contributor Author

What I would like to use this for can be seen in attached test case. I don't want to expose internals of object, rather conceal them from being dumped somewhere in logs... So I will definitely use __debugInfo, but it would be nice if Tracy would understand it too and I wouldn't have to write specific code for it.

@hrach
Copy link
Contributor

hrach commented Feb 12, 2016

@fprochazka This is internal functionality, the purpose is for debugging. Such method has nothing to do with SRP.

@fprochazka
Copy link
Contributor

@hrach I disagree. But then again, I'm not strongly against this feature. Just wanned to give my two cents and point to an existing and IMHO better way of dumping&debugging object data.

@vojtech-dobes password censoring is a good use-case, that can be also solved using custom Tracy\Dumper::$objectExporters handler.

@vojtech-dobes
Copy link
Contributor Author

@fprochazka It can, additionally to __debugInfo :).

@dg
Copy link
Member

dg commented Feb 19, 2016

I am really not sure whether it is useful to support __debugInfo, but in the meantime you can emulate it:

Dumper::$objectExporters[null] = function ($obj) {
    return method_exists($obj, '__debugInfo') ? $obj->__debugInfo() : (array) $obj;
};

@vojtech-dobes
Copy link
Contributor Author

@dg That's nice - may I ask what's the trick? Why does NULL match every object?

@JanTvrdik
Copy link
Contributor

@vojtech-dobes Have you seen 8037533?

@vojtech-dobes
Copy link
Contributor Author

@vojtech-dobes Have you seen 8037533?

Thanks. I've discovered it today when exploring new release - clear now.

dg pushed a commit that referenced this pull request Apr 3, 2018
dg pushed a commit that referenced this pull request Apr 6, 2018
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

7 participants