Suggestion: remove return type declaration check from assertObjectEquals

[jrfnl]

Related to #4467 and 1dba8c3

I'm currently looking at polyfilling the assertObjectEquals() assertion for the PHPUnit Polyfills.

As things stand, the assertion does a check on the $method to make sure that:

1. a return type is declared and
2. this return type is not nullable and
3. this return type is of type bool

This prohibits the writing of PHP/PHPUnit cross-version compatible code using comparator methods, where PHP < 7.0 still has to be supported. (yeah, I know, don't get me started....).

The thing is, if I look at the implementation, that check should not have to be a blocker.

If the return type check is removed and replaced by a check that the output of $other->{$this->method}($this->expected) is of type boolean before checking that the value is true, this part of the assertion is still safeguarded, while not blocking cross-version code.

Would a change to this effect be deemed acceptable ? If so, I'd be willing to create the PR for it.

[sebastianbergmann]

Call me paranoid, but I believe that this check is needed. Could you not make it optional in your polyfill and only perform it when you're able to?

[jrfnl]

Call me paranoid, but I believe that this check is needed.

Well, I've been trying to think through everything what's being done in the assertion / ObjectEquals class while working on this and while I can justify everything else to prevent PHP errors being thrown caused by the assertion code, I cannot justify the return type check.

• The input validation - "are $expected and $actual objects and is $method a string ?" - is needed as the assertion will call $method on $actual with $expected as parameter.
  These prevent "Call to a member function $method() on non-object" errors as well as "Uncaught Error: Method name must be a string" errors.
  This check is done in the original code via parameter type declarations, while I will implement it via a different way.
• The "does the method exist on $actual ?" check is needed for the same, to prevent "Call to undefined method" errors.
• The check on the number of parameters expected and the number of required parameters is needed to prevent a "Uncaught ArgumentCountError: Too few arguments to function $actual::$method(), 0 passed" error, though the check could probably be looser as the comparator method could be declared with additional optional arguments which the assertion just wouldn't pass, but I can understand making the check strict all the same.
• The check on the first parameter of the comparator method having a type of self or a class name and verifying that $expected is of that type ahead of time, also makes sense as that prevents Uncaught TypeError: $actual::$method(): Argument #1 ($a) must be of type Foo, string given errors when calling the method.
  As the required type is a classname based-type, this also is not a blocker for PHP cross-version code as class names are fine in PHP 5, and self or parent are fine too since PHP 5.2.
  I can imagine, the assertion code could actually be improved by allowing for parent as well. (it currently only allows for self or a class name).

However, the return type check does not prevent any PHP errors when calling the comparator method.

The only "risk" there, would be that the comparator method would return a non-boolean value, but as I propose above, that risk can be mitigated by checking that the received return value is a boolean, before doing the value check for true.

What am I missing ?

Could you not make it optional in your polyfill and only perform it when you're able to?

Of course, except that doesn't solve the problem.

Having a return type would result in a parse error on PHP 5, so in that case, it would require the "project under test" to have two different versions of their ValueObjects: one with the bool return type on the comparator method for PHP 7+ and one without for PHP < 7. The project under test would then need to load and use the correct version of the ValueObject based on whichever PHP version is being used to run it, while it is perfectly possible to implement the comparator method in a cross-version compatible manner, except that the assertObjectEquals() assertion does not accept such a method.

Does my proposal make more sense now ?

[sebastianbergmann]

However, the return type check does not prevent any PHP errors when calling the comparator method.

It prevents calling the method when it is not certain that it returns bool.

Of course, except that doesn't solve the problem.

You are right, of course.

Does my proposal make more sense now?

I think I have a better understanding of your problem now, thank you.

[jrfnl]

So, would a PR to change this be acceptable ?

[sebastianbergmann]

If you want an answer right now, that answer would be "no". Maybe that will change after I had a chance to think about this some more.

In my opinion, using this assertion in projects that cannot use return type declarations does not make sense. Making old versions of PHPUnit compatible with new versions of PHP is one thing. But making an assertion such as assertObjectEquals() that, in my opinion at least, has to look at return type declarations available for PHP versions that do not support return type declarations is something else entirely.

Sure, we could remove that check, simply call the method, and then check the return type. This is less safe, though, in my opinion at least, than requiring the bool return type declaration and not calling the method when it does not match the requirements to be used with assertObjectEquals().

Removing this check would make the polyfill work, but at the expense of less safety for developers that use current versions of PHPUnit with current versions of PHP.

[jrfnl]

I'll leave you to think it over some more in that case 😉

[jrfnl]

As additional input for thinking it over - to me, this feels like PHPUnit overstepping its remit and dictating what ValueObjects in projects which use PHPUnit should look like.

The choice for a boolean return type is arbitrary. After all for ValueObjects, it could just as easily be a valid choice to implement this with a integer return type in combination with the spaceship operator.

It could even be argued that PHPUnit should also support equals() methods with an integer return type...

class ValueObject {
    public function equals( self $other )
    {
        return $this->value <=> $other->value;
    }
}

[sebastianbergmann]

As additional input for thinking it over - to me, this feels like PHPUnit overstepping its remit and dictating what ValueObjects in projects which use PHPUnit should look like.

I would use different words, but you are right. This feature is opinionated. It is based on the assumptions outlined here.

If you do not share the opinion that is expressed by these assumptions and implemented in assertObjectEquals() then you should not use this feature. Nobody forces you to do so. It is merely an offering.

[jrfnl]

If you do not share the opinion that is expressed by these assumptions and implemented in assertObjectEquals() then you should not use this feature. Nobody forces you to do so. It is merely an offering.

Well, it's not about me. I'm presuming the feature was added to PHPUnit with the intention of it being used. And used widely-enough for it to warrant the assertion being added to PHPUnit itself and not published as an add-on.

So, I'm just pointing out some things to think over which could be seen as blockers for this assertion being widely used (based on my review for the polyfills).

[jrfnl]

Another question regarding the implementation of this assertion:

When self is encountered as the parameter type for the comparator method, it is resolved to the class name.
However, what about parent ? Should this be an allowed type ?

I totally get that it is absolutely 100% debatable whether two objects of different types, but with the same parent class, should ever be considered as "equal", however, in my humble opinion, that debate should be held in the project under test and not necessarily enforced by PHPUnit.