Protobuf: PHP equality comparison between messages always returns true

[haberman]

Comparing messages (or repeated fields, etc) in PHP always returns true, regardless of the contents:

        $m1 = new TestMessage();
        $m2 = new TestMessage();
        $m3 = new TestMessage();
        $rf1 = new RepeatedField(GPBType::INT32);
        $rf2 = new RepeatedField(GPBType::INT32);

        $rf1[] = 5;

        $m1->setOptionalInt32(1);
        $m2->setOptionalInt32(1);
        $m3->setOptionalInt32(3);

        // These succeed.
        $this->assertTrue($m1 == $m2);
        $this->assertFalse($m1 == $rf1);  // Mixing types

        // These fail.
        $this->assertFalse($m1 == $m3);
        $this->assertFalse($rf1 == $rf2);

This is true for the C extension (haven't tested pure-PHP).

Should we change this? Should equality compare contents? Should it be a deep comparison? We should provide answers to these questions and possibly change/fix the existing behavior.

[jtattermusch]

AFAIK, using == is an antipattern in PHP? ("Always use ==="?)

[haberman]

@jtattermusch I think that == (comparison) and === (object identity) implement different operations? Why would object comparison be an anti-pattern?

We support deep comparison in other languages, for example in Ruby:

• Message#==: Performs a deep equality comparison between two messages.

[haberman]

I'm leaning towards having == in PHP implement a deep comparison between two objects of the same type. For objects of different types it could either return false or signal an error.

[bshaffer]

In addition to it being intuitive, the default behavior for PHP objects is the way you suggest protobuf messages behaving:

$s1 = new stdClass();
$s2 = new stdClass();
var_dump($s1 == $s2);
// bool(true)
var_dump($s1 === $s2);
// bool(false)

$s1->foo = 'bar';
var_dump($s1 == $s2);
// bool(false)

class Foo { public $bar; }
$f1 = new Foo();
$f2 = new Foo();
var_dump($f1 == $f2);
// bool(true)

$f1->bar = '1';
var_dump($f1 == $f2);
// bool(false)

[fiboknacky]

I'm aligned with Brent (incidentally again). :)

For objects of different types it could either return false or signal an error.

In this case, I think it's intuitive to just return false.

[jtattermusch]

@jtattermusch I think that == (comparison) and === (object identity) implement different operations? Why would object comparison be an anti-pattern?

I'm not an expert on PHP, but looks like "==" behavior is deeply flawed in PHP (in general, not in relation to protobuf at all).
See "Operator Madness" section in https://whydoesitsuck.com/why-does-php-suck/

We support deep comparison in other languages, for example in Ruby:

• Message#==: Performs a deep equality comparison between two messages.

[PierrickVoulet]

This is true for the C extension (haven't tested pure-PHP).

I just tested with pure-PHP and it behaves differently: it seems to perform a deep equality comparison instead.

$rf1 = new RepeatedField(GPBType::INT32);
$rf2 = new RepeatedField(GPBType::INT32);
$rf2[] = 1;

var_dump($rf1 == $rf2);
// With PHP Extension (C implementation)
// bool(true)

// With PHP Module (PHP implementation)
// bool(false)

[PierrickVoulet]

@haberman @TeBoring this issue is becoming problematic for https://github.com/googleads/google-ads-php, is there anything that we could do to prioritize this?

[TeBoring]

Can user code compare field by field for now?

On Tue, Sep 1, 2020 at 11:03 AM Pierrick Voulet [e-mail]
wrote:

>
>

@haberman https://github.com/haberman @TeBoring
https://github.com/TeBoring this issue is becoming problematic for
https://github.com/googleads/google-ads-php, is there anything that we
could do to prioritize this?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/protocolbuffers/protobuf/issues/7650#issuecomment-685038257,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABHUPZOQNGKCWDTRQ2MBZ3LSDUZOLANCNFSM4OIWWXXA
.

[haberman]

I agree it's problematic if the two implementations are having different behavior.

Is the behavior given by pure-PHP right now the right behavior? Or are they both wrong?

Maybe the first step would be to determine exactly what semantics are implemented by pure-PHP right now.

[TeBoring]

I think pure php compare implementation detail that the array
representation of both are equal.

On Tue, Sep 1, 2020 at 11:14 AM Joshua Haberman [e-mail]
wrote:

>
>

I agree it's problematic if the two implementations are having different
behavior.

Is the behavior given by pure-PHP right now the right behavior? Or are
they both wrong?

Maybe the first step would be to determine exactly what semantics are
implemented by pure-PHP right now.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/protocolbuffers/protobuf/issues/7650#issuecomment-685045502,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABHUPZIPMHO5OQF3UJXQ4MLSDU2YJANCNFSM4OIWWXXA
.

[TeBoring]

pure php’s implementation was provided by default. In our c extension, we
didn’t provide the handler to convert to array, so we couldn’t use the
default implementation.

On Tue, Sep 1, 2020 at 11:16 AM Paul Yang [e-mail] wrote:

I think pure php compare implementation detail that the array
representation of both are equal.

On Tue, Sep 1, 2020 at 11:14 AM Joshua Haberman [e-mail]
wrote:

>
>

I agree it's problematic if the two implementations are having different
behavior.

Is the behavior given by pure-PHP right now the right behavior? Or are
they both wrong?

Maybe the first step would be to determine exactly what semantics are
implemented by pure-PHP right now.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/protocolbuffers/protobuf/issues/7650#issuecomment-685045502,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABHUPZIPMHO5OQF3UJXQ4MLSDU2YJANCNFSM4OIWWXXA
.

[haberman]

But does pure-PHP do deep comparison between messages? If you have two arrays of messages, how will it compare the messages? Will it compare them field by field, recursively? What about maps?

[TeBoring]

I think it’s deep. Map would be dict.

On Tue, Sep 1, 2020 at 11:56 AM Joshua Haberman [e-mail]
wrote:

>
>

But does pure-PHP do deep comparison between messages? If you have two
arrays of messages, how will it compare the messages? Will it compare them
field by field, recursively? What about maps?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/protocolbuffers/protobuf/issues/7650#issuecomment-685068018,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABHUPZJRKBR4AWSI4R7FIR3SDU7VTANCNFSM4OIWWXXA
.

[haberman]

We should verify this. Specifically, we should add tests that check the pure-PHP behavior, and then verify that it implements the semantics we want.

If the pure-PHP semantics are correct, then we can change the C extension to match.

[AnashOommen]

Folks, gentle ping for an update on this issue. We have broken code examples in Google Ads that are dependent on the resolution of this bug.

[bshaffer]

As far as I can tell, the Pure PHP implementation functions as expected and the C implementation needs to change to match:

$m1 = new TestMessage();
$m2 = new TestMessage();
$n1 = new TestMessage\NestedMessage();
$n2 = new TestMessage\NestedMessage();

printf("protobuf extension enabled? %s\n\n", var_export(extension_loaded('protobuf'), true));

print "Should be true: identity\n";
var_dump($m1 === $m1);
print "Should be true: equality\n";
var_dump($m1 == $m2);
$m1->setNestedMessage($n1);

print "Should be false: different nested message\n";
var_dump($m1 == $m2);

print "Should be true: identical nested message\n";
$m2->setNestedMessage($n1);
var_dump($m1 == $m2);
print "Should be true: equal nested message\n";
$m2->setNestedMessage($n2);
var_dump($m1 == $m2);

print "Should be false: inequal nested mesage property\n";
$n1->setX(1);
var_dump($m1 == $m2);

print "Should be true: equal nested mesage property\n";
$n2->setX(1);
var_dump($m1 == $m2);

The output for C and pure php looks like this:

protobuf extension enabled? false

Should be true: identity
bool(true)
Should be true: equality
bool(true)
Should be false: different nested message
bool(false)
Should be true: identical nested message
bool(true)
Should be true: equal nested message
bool(true)
Should be false: inequal nested mesage property
bool(false)
Should be true: equal nested mesage property
bool(true)

protobuf extension enabled? true

Should be true: identity
bool(true)
Should be true: equality
bool(true)
Should be false: different nested message
bool(true)
Should be true: identical nested message
bool(true)
Should be true: equal nested message
bool(true)
Should be false: inequal nested mesage property
bool(true)
Should be true: equal nested mesage property
bool(true)

[PierrickVoulet]

@haberman I may be missing something obvious but I am still getting the same behavior while using your new version:

$rf1 = new RepeatedField(GPBType::INT32);
$rf2 = new RepeatedField(GPBType::INT32);
$rf2[] = 1;

var_dump($rf1 == $rf2);
// With PHP Extension (C implementation)
// bool(true)

// With PHP Module (PHP implementation)
// bool(false)

[TeBoring]

I think user is not supposed to create repeated field by himself.

On Tue, Sep 15, 2020 at 11:21 AM Pierrick Voulet [e-mail]
wrote:

>
>

@haberman https://github.com/haberman I may be missing something
obvious but I am still getting the same behavior while using your new
version:

$rf1 = new RepeatedField(GPBType::INT32);

$rf2 = new RepeatedField(GPBType::INT32);

$rf2[] = 1;

var_dump($rf1 == $rf2);

// With PHP Extension (C implementation)

// bool(true)

// With PHP Module (PHP implementation)

// bool(false)

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/protocolbuffers/protobuf/issues/7650#issuecomment-692890683,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABHUPZNNEG2SIG3NBKKX2G3SF6WA5ANCNFSM4OIWWXXA
.

[PierrickVoulet]

I did more digging and 2 of our comparison tests are fixed with the new version, thanks!

I still see our comparison test with a repeated field of messages failing though:

============ TEST 'Modify message in a repeated field'
LEFT: {"foos":[{"num":"1"},{"num":"2"}]}
RIGHT: {"foos":[{"num":"1"},{"num":"3"}]}
- expected diff: foos 
- actual diff: