Classes with __toString() should be tested like strings when appropriate

[mtdowling]

PHPUnit currently does not work with things like:

$this->assertRegExp($pattern, $class);

When $class implements __toString(). Assertions that work on strings should cast objects to a string if the object has a __toString() method. PHP does this when using objects in any method that accepts a string, so I think it makes sense that PHPUnit adopts the same behavior.

[jeremeamia]

👍 Makes sense to me.

[whatthejeff]

I've thought about this before, but I'm still not sure if implicit type conversions are helpful or dangerous when testing. Consider that all the following examples match in PHP:

var_dump(preg_match('/0/', new Exception()));
var_dump(preg_match('/1/', true));
var_dump(preg_match('/2/', 512));
var_dump(preg_match('/3/', 5.37));

@sebastianbergmann, what do you think?

[mtdowling]

var_dump(preg_match('/0/', new Exception()));

This is and should be a match because casting an Exception to a string yields something like:

'exception \'Exception\' in php shell code:1
Stack trace:
#0 {main}'

This would be a really bad test because the regular expression is way too broad.

var_dump(preg_match('/1/', true));
var_dump(preg_match('/2/', 512));
var_dump(preg_match('/3/', 5.37));

I'm not talking about changing anything to accept other things where it should accept a string. I'm only talking about making all string based assertions honor a classes __toString() method.

[whatthejeff]

@mtdowling, maybe I didn't do a very good job of explaining myself :P

Those examples are meant to be contrived. I didn't intend to suggest that those are examples of good tests or that they shouldn't match for some reason. I understand why they match.

I was trying to suggest that "PHPUnit should do x because PHP does x" is not always the best line of reasoning. PHP also accepts other scalars (integers, floats, booleans) in most methods that accept strings, but I don't think PHPUnit should.

I understand that you're only suggesting that string-based assertions should use __toString() if it's available. I understand that explicit type casting is sort of inconvenient. I just wanted to make sure we consider the value of explicitness in a testing framework.

In the end, I'm not really for or against this. I asked for @sebastianbergmann's opinion because I'm kind of on the fence. Sorry if my original response was misleading, I'm just a bit pressed for time today :)

[sebastianbergmann]

Also on the fence here, leaning towards the "too much magic" side.

[whatthejeff]

Maybe @edorian has an opinion?

[Tatsh]

Would be cool to support, but I am also mostly on the side of forcing unit tests to be more explicit when using those assertions, whether it be by directly calling __toString() or by casting on the line (with a comment hopefully).

Otherwise, the implicit __toString() handling in PHP is overall useful, but magical (fwrite($handle, new SpecialClass());).

[whatthejeff]

Alright, I think it's time to close this. Thanks for the input everyone, but it doesn't seem like any of the core maintainers are in favor of this feature.