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 C extension protobuf message class introspection not possible #4761

Closed
hpuac opened this issue Jun 13, 2018 · 14 comments
Closed

PHP C extension protobuf message class introspection not possible #4761

hpuac opened this issue Jun 13, 2018 · 14 comments

Comments

@hpuac
Copy link

hpuac commented Jun 13, 2018

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.

@TeBoring
Copy link
Contributor

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?

@hpuac
Copy link
Author

hpuac commented Jun 13, 2018

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?

@TeBoring
Copy link
Contributor

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.

@tobiasgies
Copy link

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?

@hpuac
Copy link
Author

hpuac commented Jun 14, 2018

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.

@mcos
Copy link
Contributor

mcos commented Jun 14, 2018

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

@TeBoring
Copy link
Contributor

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

@TeBoring
Copy link
Contributor

TeBoring commented Jun 15, 2018

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

@mcos
Copy link
Contributor

mcos commented Jun 15, 2018

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

@tmatsuo
Copy link

tmatsuo commented Jun 20, 2018

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.

@TeBoring
Copy link
Contributor

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

@bshaffer
Copy link
Contributor

bshaffer commented Jul 18, 2018

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(
  )),
)

tmatsuo pushed a commit to googleapis/google-cloud-php that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-bigquerydatatransfer that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-bigtable that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-container that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-core that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-dataproc that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-debugger that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-dialogflow that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-dlp that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-errorreporting that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-firestore that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-iot that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-kms that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-language that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-logging that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-monitoring that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-oslogin that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-pubsub that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-redis that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-spanner that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-speech that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-tasks that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-text-to-speech that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-trace that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-videointelligence that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
dwsupplee pushed a commit to googleapis/google-cloud-php-vision that referenced this issue Jul 19, 2018
* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase
sduskis pushed a commit to googleapis/google-cloud-php that referenced this issue Jul 23, 2018
* Add isRacy() to SafeSearch (#1166)

The SafeSearch class included `is` functions for each SafeSearch feature (adult, spoof, medical, violence), but was missing Racy. This Pull Request adds `isRacy()` to the SafeSearch class.

* Exclude component vendor folder from snippet coverage (#1168)

cc @tmatsuo

* Allow context to handle Throwable interface (#1164)

* Add getServiceAccount to the BigQueryClient (#1167)

* Add getServiceAccount to the BigQueryClient

See:
https://cloud.google.com/bigquery/docs/reference/rest/v2/projects/getServiceAccount

* Added $options to getServiceAccount and fixed the indent

* Use single quote

* Add snippet test

* Comment update, use self instead of $this

* Comment update

* Prepare v0.71.0 (#1169)

* Prepare v0.71.0

* patch release for Logging

* Correct version for Logging

* [Kms] Regenerate with the new gapic config (#1165)

* Update for the new gapic configuration

* Docs update for the new and nicer namespace

* Removed the files with the old namespace

* Use the new namespace in the code sample

Add back the deprecated files

* Changed the wording for the deprecation warning

* Fix firestore queries (#1161)

* Bump gax to 0.35 (#1170)

* Add an interactive release builder. (#1160)

* Add an interactive release builder.

* Create build directory if it doesn't exist

* Add getServiceAccount method to StorageClient (#1173)

* Re-generate library using Tasks/synth.py (#1174)

* Re-generate library using Tasks/synth.py

* Use new namespace in user visible area, tweak the deprecation wording

* Add support for Numeric type (#1172)

* Add support for Numeric type

* Added a cast in Numeric's constructor, added system tests

* Allow '123.' and '.123', update tests

* [Breaking Change] Add support for Document Snapshots in Firestore Query Cursors (#1162)

cc @schmidt-sebastian

Extracted from #923 and updated to address pull request comments.

Breaking change is the standardization in Query of using `InvalidArgumentException`, replacing various throws of `BadMethodCallException`.

Closes #851.

* Fix Storage Requesterpays system tests (#1180)

The requester pays system tests have been broken for some time. This change fixes them.

* Bandaid for protobuf 4761 (#1176)

* Temporary workaround for protobuf extension issue

protocolbuffers/protobuf#4761

* Bump gax to 0.36

* Configure comparators for the unit test

* Revert back to normal TestCase

* Install protobuf extension in the PHP 7.2 test runner

* Revert to TestCase

* Prepare v0.72.0 (#1181)

* Added a document for the time filter on BigQueryClient->jobs() (#1183)

* Narrowed the time filter in the system test (#1185)

* Re-generate library using BigQueryDataTransfer/synth.py (#1184)

* Re-generate library using BigQueryDataTransfer/synth.py

* Tweak the wording on the deprecation warning

Also use the new namespace in the sample code

* pin auth version until we have a way to silence warnings (#1189)
@TeBoring
Copy link
Contributor

I am working on a fix: #4946

@deannagarcia
Copy link
Member

This issue is very old and covers many different problems. Going to close but please open a specific bug about a feature if you still need it.

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

8 participants