protocolbuffers / protobuf

Protocol Buffers - Google's data interchange format
http://protobuf.dev
Other
65.63k stars 15.49k forks source link

PHP: better debug info for Message objects in protobuf C-extension #14872

Open bshaffer opened 11 months ago

bshaffer commented 11 months ago

See https://github.com/protocolbuffers/protobuf/issues/12714 and https://github.com/protocolbuffers/protobuf/pull/12718, which resolved this problem for the native library.

When the protobuf c-extension is enabled for PHP, calling var_dump on a protobuf message object does not output any useful information:

// Calling var_dump on a Protobuf message (extension enabled):
php > $timestamp = new Google\Protobuf\Timestamp();
php > $timestamp->setSeconds(12345);
php > var_dump($timestamp);
object(Google\Protobuf\Timestamp)#1 (0) {
}

Ideally this would output something like this:

object(Google\Protobuf\Timestamp)#12 (1) {
  ["seconds"]=>
  int(12345)
}
northmule commented 10 months ago

A similar problem. Checked on protobuf 3.12.4, 3.17, 3.24.4 php.ini

extension=protobuf.so

php -v

PHP 7.3.29 (cli) (built: Jul 22 2021 03:24:49) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.3.29, Copyright (c) 1998-2018 Zend Technologies
    with Xdebug v3.1.6, Copyright (c) 2002-2022, by Derick Rethans
    with Zend OPcache v7.3.29, Copyright (c) 1999-2018, by Zend Technologies
bshaffer commented 10 months ago

@northmule what is the problem you're seeing?

For clarification, this issue is a feature request for the extension to produce better debugging info. It has not yet been implemented.

northmule commented 10 months ago

The problem is that when connecting the .so library in the php.ini file, the ability to view the class object inherited from the parent class \Google\Protobuf\Internal\Message disappears. I have prepared two visual screenshots from the Phpstorm IDE and the configured xDebug debugger. In one screenshot in php.ini, the library connection is disabled - ;extension=protobuf.so In another screenshot, the library connection is enabled - extension=protobuf.so

The library is connected here

extension=protobuf.so

Screenshot_20231212_084650

The library is not connected here

;extension=protobuf.so

Screenshot_20231212_084512

I highlighted the object in red.

If the library is not connected, I can view the object, it is filled with data

If the library is connected, I cannot view the object.

For simplicity, I took the \Google\Protobuf\Timestamp class, but this is reproduced on any other object from any class with inheritance from \Google\Protobuf\Internal\Message

bshaffer commented 9 months ago

I assume this is because in the first example, the object is implemented in the C extension, and so is not visible by the IDE. This feature request is to implement a hook in the message class implemented in C++ which, similar to __debugInfo in PHP, when var_dump is called, it prints the properties inside the C object.

haberman commented 9 months ago

I think this is a reasonable feature request, and we'd be happy to accept a contribution.

This would involve implementing get_debug_info for Message, RepeatedField, and MapField.

There was previously a PR to return an empty hash table for get_debug_info, which is a good example of how this might look, except that we'd want the returned hash table to be fully populated: https://github.com/protocolbuffers/protobuf/pull/7562/files

haberman commented 9 months ago

I just noticed that we have a test for debug info that is disabled for the C extension: https://github.com/protocolbuffers/protobuf/blob/6f1d88107f268b8ebdad6690d116e74c403e366e/php/tests/DebugInfoTest.php#L37-L42

So we essentially already have a spec for how the C extension should work in this regard.

bshaffer commented 9 months ago

@haberman This isn't a justification for the existing behavior, it's just confirming that it isn't currently supported.

I added that test (and the feature). It was required to skip since this hasn't been implemented in the C extension yet.

See https://github.com/protocolbuffers/protobuf/pull/12718

EDIT: I think I misunderstood your comment to mean this was WAD, but after rereading it, I think you're just saying that the C-extension's implementation can follow the "spec" of the native implementation (and pass the tests), and I totally agree. Sorry for the confusion.

haberman commented 9 months ago

I think you're just saying that the C-extension's implementation can follow the "spec" of the native implementation (and pass the tests), and I totally agree.

Exactly. I meant it as a hint/pointer for whoever ends up implementing this for C. The easiest way to start is to re-enable the test, and code a solution until the test passes.