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 ***@***.***> 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 <#7650 (comment)>, 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 ***@***.***> 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 <#7650 (comment)>, 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 ***@***.***> 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 ***@***.***> 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 > <#7650 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABHUPZIPMHO5OQF3UJXQ4MLSDU2YJANCNFSM4OIWWXXA> > . > > >