Protobuf: PHP C extension protobuf message class introspection not possible

Created on 13 Jun 2018  路  13Comments  路  Source: protocolbuffers/protobuf

Issue

When var_dumping, json_encoding or array casting a generated protobuf message class, an E_USER_ERROR "Cannot access private properties." is triggered in the protobuf C extension.
This behaviour is different from PHPs native behaviour and thus diffierent to the behaviour of the protobuf PHP library.
I found this issue after updating the protobuf extension because the comparison of two protobuf message classes with PHPUnit's assertEquals function was failing.

Expected behaviour

It should follow the PHP native behaviour, that means:
The error should only be triggered if there is a real direct private property access ($instance->property).
When json_encoding it should return an empty object. {}
When var_dumping it should show the private properties and its values.
Example:

object(MyClass)#1 (1) {
  ["my_property":"MyClass":private]=>
  string(9) "someValue"
}

When array casting, it should return an array with the keys and values.
Example:

array(1) {
  ["MyClassmy_property"]=>
  string(9) "someValue"
}

Additional information

The E_USER_ERROR was introduced with v3.6.0rc1. Curiously, code that would implement the expected behavior was also added in that commit, it is just unreachable because of the zend_error call.

P2 enhancement php to-be-fixed

Most helpful comment

In my opinion the main issue is that the C Extension is behaving different than the PHP package.
Things that work with the PHP package are causing an E_USER_ERROR in the C Extension.
Also the C Extension is behaving different than plain PHP classes would do.
I guess we all agree that this is not how it should be.
We have now two solutions:

  1. Adjust the PHP package to follow the C extension.
    This can be done by implementing the functions __debugInfo and __get and implementing the ArrayAccess interface in the Message class to trigger the E_USER_ERROR like the C extension is triggering it right now.
  2. Adjust the C extension to behave like the PHP package/PHP classes.

I would prefer the second approach since it would be a way more natural behaviour and increase the usability.

All 13 comments

We haven't supported assertEquals for message. For now, your test should not depend on that.
For var_dump, could you do that with php implementation only?

Hey @TeBoring, thank you for your quick response!
We already adjusted our assertion to check the properties over the provided getters.
Nevertheless the behaviour of the C extension is not as intended and I would love to see it getting improved.
I don't really understand your last question. Could you maybe explain it again?

In c extension, get_properties is called when var_dump or any operations that transform protobuf message object into array (by design we don't allow message object to be transformed into array). Therefore, the only valid use case for get_properties to be executed is var_dump.
IMO, var_dump is only used for debug. When you put your code into production server, there is no need to var_dump a protobuf message.

Hi @TeBoring, while I agree that var_dump is unlikely to be used in production, that is just one example of a function that can't be used with proto message objects. In my opinion, casting the object to an array or running it through json_encode are definitely use cases that we should be able to use in production. Would you agree?

In my opinion the main issue is that the C Extension is behaving different than the PHP package.
Things that work with the PHP package are causing an E_USER_ERROR in the C Extension.
Also the C Extension is behaving different than plain PHP classes would do.
I guess we all agree that this is not how it should be.
We have now two solutions:

  1. Adjust the PHP package to follow the C extension.
    This can be done by implementing the functions __debugInfo and __get and implementing the ArrayAccess interface in the Message class to trigger the E_USER_ERROR like the C extension is triggering it right now.
  2. Adjust the C extension to behave like the PHP package/PHP classes.

I would prefer the second approach since it would be a way more natural behaviour and increase the usability.

I'm hitting this issue also. For now, I'm using serializeToString() to do assertEquals(), but that's not ideal, obviously.

@tobiasgies, what's your use case for casting the object to an array?
Is json_encode related to this issue?

@mcos, assertEquals is a separate issue, right? php implementation also doesn't suppport assert Equals

It mightn't be fully supported, but assertEquals with a protobuf message argument works with the composer library.

Oh yes. The unit tests in google-cloud-php actually depend on assertEquals with protobuf messages and they passed with protobuf extension 3.5.1.1. With 3.6.0, the tests started to fail.

It's welcome to have a PR for this issue.

It is possible in PHP to convert an object to an array using a simple array cast. Check out the documentation on PHP arrays under "Converting to array". I found the following example also from this helpful stack overflow article:

class Foo
{
    private $foo;
    protected $bar;
    public $baz;

    public function __construct()
    {
        $this->foo = 1;
        $this->bar = 2;
        $this->baz = new StdClass;
    }
}

var_export((array) new Foo);

This will output:

array (
  '' . "\0" . 'Foo' . "\0" . 'foo' => 1,
  '' . "\0" . '*' . "\0" . 'bar' => 2,
  'baz' => 
  stdClass::__set_state(array(
  )),
)

I am working on a fix: #4946

Was this page helpful?
0 / 5 - 0 ratings