Support byteSize() in PHP's C implementation

[fiboknacky]

What version of protobuf and what language are you using?
Version: 3.21.2 (installed via PECL)
Language: PHP

What operating system (Linux, Windows, ...) and version?
Linux

What runtime / compiler are you using (e.g., python version or gcc version)
N/A

What did you do?
Do the following and I get Error: Call to undefined method Google\Ads\GoogleAds\V11\Resources\Campaign::byteSize():

use Google\Ads\GoogleAds\V11\Resources\Campaign;

$campaign = new Campaign();
print $campaign->byteSize();

Same for jsonByteSize().

What did you expect to see
Some numbers printed.

What did you see instead?
Error: Call to undefined method Google\Ads\GoogleAds\V11\Resources\Campaign::byteSize():

Anything else we should know about your project / environment
I use google-ads-php, but I believe it would occur to any object created.
This error doesn't occur when I use the PHP implementation installed by Composer.

Besides, when I installed (updated) the extension using pecl, there was a segmentation fault:

Build process completed successfully
Installing '/usr/lib/php/20210902/protobuf.so'
install ok: channel://[pecl.php.net/protobuf-3.21.2](http://pecl.php.net/protobuf-3.21.2)
configuration option "php_ini" is not set to php.ini location
You should add "extension=protobuf.so" to php.ini
Segmentation fault

[haberman]

Has byteSize() ever worked for the C extension?

Looking at the code, I don't believe it was ever implemented for C, and I'm not sure it was intended to be exposed as a public API.

Can you paste the command that triggered the segfault?

[fiboknacky]

Good question. I don't have memory about that either.
Having said that, shouldn't the two versions work the same?

Actually, what I need here is whether the message is empty (have no fields set at all). It's available in some forms in Java and .NET. If we shouldn't rely on this method, do you have alternatives?

[fiboknacky]

Any updates please?

[haberman]

byteSize() is fine to use for this purpose, when it is available.

I think byteSize() would be reasonable to add to the official public API (supported in both pure-PHP and PHP/C), but that would be a feature request.

For now probably the simplest thing is to serialize the proto and test whether the serialized string is empty.

[fiboknacky]

For byteSize(), do you want me to file a new FR? Or can I repurpose this bug?

BTW, I try to use the serializeToJsonString() for now, but found that getRealContainingOneof is also not available in the C implementation. (The same Error: Call to undefined method error)

I begin to wonder why the implementations of the two versions are quite different. Do we have any unit tests to ensure that they'll be closely aligned? Or is that not a goal at first?

[haberman]

For byteSize(), do you want me to file a new FR?

We can use this issue.

BTW, I try to use the serializeToJsonString() for now, but found that getRealContainingOneof is also not available in the C implementation. (The same Error: Call to undefined method error)

I begin to wonder why the implementations of the two versions are quite different. Do we have any unit tests to ensure that they'll be closely aligned? Or is that not a goal at first?

It is definitely a goal for the two implementations to have the same public API. This does not apply to anything in an Internal namespace, but for public APIs it is a goal to keep them the same.

The majority of our unit tests are run against both implementations, to ensure that there is a single public API for both. The only exception is https://github.com/protocolbuffers/protobuf/blob/main/php/tests/PhpImplementationTest.php which is only run against the pure-PHP implementation.

I believe that getRealContainingOneof() was added by you in #10102. It was added to PHP but not C, and this was not caught because there is no unit test. I probably should have asked you to add a unit test for this, but I was trying to be sensitive to the schedule pressure you were feeling, so I didn't press for adding unit tests. In retrospect, unit tests would have been a good idea since they would have caught this.

[fiboknacky]

Thanks.

I believe that getRealContainingOneof() was added by you in #10102. It was added to PHP but not C, and this was not caught because there is no unit test. I probably should have asked you to add a unit test for this, but I was trying to be sensitive to the schedule pressure you were feeling, so I didn't press for adding unit tests. In retrospect, unit tests would have been a good idea since they would have caught this.

Sorry that I wasn't clear. Our users use both implementations so all features must be supported in both versions.
Could you please help add getRealContainingOneof to the C implementation as well?

[fiboknacky]

@haberman Friendly ping. Could you please help at least unblock us for using getRealContainingOneof in the C implementation? That's our blocker and we cannot even use what I implemented in #10102 if this is not fixed properly.

The byteSize() support for C implementation can be done later.

[haberman]

I will get you something for getRealContainingOneof() in C sometime this week. Sorry for the delay.

[fiboknacky]

Thanks!

[fiboknacky]

I see your PR is merged. When will this be included in the PECL extension?

[haberman]

We are planning to do a release this week.

[fiboknacky]

Thanks. Will wait for that.

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.