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

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

Open
bshaffer opened this issue Nov 28, 2023 · 8 comments
Open

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

bshaffer opened this issue Nov 28, 2023 · 8 comments

Comments

@bshaffer
Copy link
Contributor

See #12714 and #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)
}
@bshaffer bshaffer added the untriaged auto added to all issues by default when created. label Nov 28, 2023
@anandolee anandolee added php and removed untriaged auto added to all issues by default when created. labels Nov 29, 2023
@bshaffer bshaffer changed the title PHP: better debug info for Message objects in C-extension PHP: better debug info for Message objects in protobuf C-extension Dec 5, 2023
@northmule
Copy link

northmule commented Dec 11, 2023

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
Copy link
Contributor Author

@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
Copy link

northmule commented Dec 12, 2023

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
Copy link
Contributor Author

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
Copy link
Member

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 haberman removed their assignment Jan 30, 2024
@haberman
Copy link
Member

I just noticed that we have a test for debug info that is disabled for the C extension:

public function setUp(): void
{
if (extension_loaded('protobuf')) {
$this->markTestSkipped('__debugInfo is not supported for the protobuf extension');
}
}

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

@bshaffer
Copy link
Contributor Author

bshaffer commented Jan 31, 2024

@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 #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
Copy link
Member

haberman commented Feb 1, 2024

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.

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

No branches or pull requests

4 participants