Dont\ToString lies to its consumers in PHP 8 because it implements Stringable

[lexidor]

The manner in which Dont\ToString is enforced, makes instanceof tell lies.

final class Plain {}
final class DontToStringMe { use Dont\ToString; }
$plain = new Plain;
$dont = new DontToStringMe;
var_dump($plain instanceof Stringable); // false
var_dump($dont instanceof Stringable); // true

Both classes throw when cast to a string. One uses the standard PHP message, could not convert object of type X to a string. The other throw an exception from this library.

If the class you are writing is final, then you don't need this trait. You either don't inhirit a __toString, which gives you sane default behavior in PHP. If you do inherit one, you violate liskov substitution by using this trait. If the class you are writing is not final (and intended to be extended by library users), why limit them in being able to implement their own __toString()?

Dont\Serialise does not implement Serializable > #21 (comment) <, since this would be lying to consumers. I believe that Dont\ToString isn't like the other traits for this reason.

I am interested to know whether others have an opposing view. I don't check a lot of things using instanceof nor do I use Stringable as a typehint, so it took a while for this to dawn on me. I have since stopped using Dont\JustDont and made my own trait which includes all traits, except for ToString, because of this behavior.

[Ocramius]

Overall makes sense: type level declaration > runtime failure. The question is whether we can somehow enforce this at type level 🤔

…

On Thu, Jun 17, 2021, 18:46 Lexidor Digital ***@***.***> wrote: The manner in which Dont\ToString is enforced, makes instanceof tell lies. final class Plain {}final class DontToStringMe { use Dont\ToString; }$plain = new Plain;$dont = new DontToStringMe;var_dump($plain instanceof Stringable); // falsevar_dump($dont instanceof Stringable); // true Both classes throw when cast to a string. One uses the standard PHP message, could not convert object of type X to a string. The other throw an exception from this library. If the class you are writing is final, then you don't need this trait. You either don't inhirit a __toString, which gives you sane default behavior in PHP. If you do inherit one, you violate liskov substitution by using this trait. If the class you are writing is not final (and intended to be extended by library users), why limit them in being able to implement their own __toString()? Dont\Serialise does not implement Serializable > #21 (comment) <#21 (comment)> <, since this would be lying to consumers. I believe that Dont\ToString isn't like the other traits for this reason. I am interested to know whether others have an opposing view. I don't check a lot of things using instanceof nor do I use Stringable as a typehint, so it took a while for this to dawn on me. I have since stopped using Dont\JustDont and made my own trait which includes all traits, except for ToString, because of this behavior. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#34>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFVEHUWSA3IALE6ZP2I2TTTIRGLANCNFSM464BRTOQ> .

[lexidor]

I have shared the contents of my mind here. I hope this is a convincing case against declaring an anti implementation for __toString().

If psalm starts becoming more strict about stringification (requiring an object to be Stringable when typechecking), this anti implementation would make a static analysis issue a runtime issue. Psalm would allow an implicit cast to string, exactly because of the anti implementation.

If you don't want to be stringified, just don't define a __toString method. I think this kind of thing could be enforced in a unittest somewhere. Assert not instanceof Stringable (PHP 8.0+) or reflection to see there is no __toString should do the trick.

The PHP 7.4+ behavior is throwing an \Error Object of class X could not be converted to string. PHP 5.2+ raises a recoverable fatal error via the php error handler machanism. PHP 5.0+ stringifies objects as Object id #\d. But if you are running code on PHP 5.0 or 5.1, you have other problems.

The default of PHP is already sufficient to prevent accidental stringification. If you are a library author who publishes a final class, you have full control, so add a unittest that makes sure you don't make your objects Stringable by mistake, if it is important to you. If you are publishing a non-final class or a trait, why limit the end user from making their object Stringable. You could still unittest your trait or class, if a part of your API is that users can define their own __toString. If you are not a library author, it is up to you if you make your objects Stringable, but you could also write a unittest, to prevent anyone on the team adding a __toString by accident.

Blocking __toString() with an anti implementation does not uphold an object invariant. Blocking the methods listed below does uphold an invariant. Either because the PHP default is bad, which is a reason to override the behavior. Or because a subclass declaring this method would be bad.

• __serialize() If I hold a database connection, I'd not want to be (de)serialized, since a runtime resource is not serializable.
• __unserialize() If I have a destructor which has a side effect, I don't want an attacker to craft a payload for deserialize() which will invoke my destructor and do harm.
• __clone() If hold a Mutex-like primitive member or I am a singleton, I would not want to be cloned, since this violates correctness.
• __set() If I want to be json encoded using the default public properties (not implementing JsonSerializable), I don't want people adding new dynamic properties at runtime, since this changes the shape of the json I get serialized into.
• __callStatic() If I expect $this to be an instance of an object (and not null), I don't want non static methods to be called statically. (PHP 8 does the right thing by default and disallows mixing the calling convention.)

The other magic methods that are defined in Dont\JustDont use do not uphold an object invariant. Blocking them with an anti implementation has other benefits.

• __get() PHP returns null when reading a missing property name. It emits a warning, but failing loudly saves everyone time. Using __get() also makes code really hard to reason about, so let's just ban it for reasonability about code.
• __call() The default PHP behavior when calling something that is protected or does not exist is throwing an \Error. So the default is fine. But just like __get(), it makes code a pain to reason about. A subclass adds __call() and listens for doStuff(). If the superclass adds a public method doStuff() this will take presedence over the magic in the subclass. And instead of failing loudly about this conflict, the method from the superclass would silently be put in place. So yeah, let's not allow that either.

__toString() is just not like the others. The anti implementation doesn't provide new meaningful behavior over the default. It doesn't prevent a subclass from doing something bad.

[johanrosenson]

I agree with @lexidor, can a new version be released without DontToString?

Or atleast removed from JustDont.

[lexidor]

I have made a PR doing just that.
Removing the trait is a "useless" BC break.
Removing the use clause in JustDont is a breaking change according to semver.
I don't have the knowledge of the applications that use this out in the wild.
Code like this will behave differently:

final class X { use Dont\JustDont; }
function takes_stringable(\Stringable $_): void {}
takes_stringable(new X()); // X does not implement Stringable
// or
final class Y implements \Stringable { use Dont\JustDont; } // missing __toString
// or
final class Z extends Donter { public function __toString(): string {...}} // no fatal

The PHP community is a lot stricter on semver than most, so I am leaving the dot (x|y|z) decision up to others.