Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Stringable interface #5083

Open
wants to merge 1 commit into
base: master
from
Open

Conversation

@nicolas-grekas
Copy link
Contributor

nicolas-grekas commented Jan 13, 2020

This PR introduces a new Stringable interface that ppl can declare when implementing __toString.

This goal is to allow using the string|Stringable union type in PHP 8, to accept both strings and objects that implement __toString() and declare this interface. This is critically missing currently, on codes that deal with stringable objects: they can't be made type-safe.

On purpose, there is no corresponding is_stringable() function, because that would require defining what happens when e.g. an int is passed to this function, and there are no simple answers to this concern.

On purpose also, for BC, classes that implement __toString() aren't required to declare the interface.
Such classes wouldn't pass the above union type, but that's expected, as that's the semantics of the language.

By being simple and without any magic capabilities on its own, this interface is trivially polyfilled on PHP < 8.

Once a polyfill becomes widely available (I would personally add one to symfony/polyfill-php80 immediately after this PR is accepted), I expect code style checkers to be able to enforce declaring the interface when __toString() is used. For projects that don't use cs checkers, that's fine: they'll notice quickly that they missed adding the interface because their users will ask for it when they'll want to pass the string|Stringable union type.

Here is the stub declaration of the interface:

interface Stringable
{
   public function __toString(): string;
}

Because it adds the string return type, this interface has the potential to force a BC break on any existing libraries that want to adopt it.

In order to ease forward and backward compatibility, the PR also adds the return type to existing __toString methods when it's not explicit declared already. Returning a string is already enforced by the engine so this doesn't change any semantics.

This way, code moving to PHP8 won't be forced to add the return type explicitly (which would break BC on their side), and code in PHP < 8 can adopt a polyfill interface immediately (one that doesn't declare the return type for the same BC reasons.) Providing an easy forward-path is the second goal of this PR.

For reference, here are some annotations in Symfony, added by contributions from real-world use cases and that currently cannot be expressed precisely enough using any union types in PHP 8:
https://github.com/symfony/symfony/search?q=%22%40param+string%7Cobject%22+stringable&unscoped_q=%22%40param+string%7Cobject%22+stringable

@carusogabriel carusogabriel added the RFC label Jan 13, 2020
Copy link
Contributor

Ocramius left a comment

Would require .phpt additions to verify the static definitions of the interface (see #2535 for examples)

Zend/zend_interfaces.stub.php Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:stringable branch from fc39453 to 8b15e23 Jan 13, 2020
@ntzm

This comment has been minimized.

Copy link
Contributor

ntzm commented Jan 13, 2020

Can I ask why you don't also support other types that can be cast to a string? e.g. everything but arrays and object that don't have __toString declared?

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 13, 2020

Can I ask why you don't also support other types that can be cast to a string? e.g. everything but arrays and object that don't have __toString declared?

I'm sorry I totally miss your question. There is no such thing as "supporting other types".

@Ocramius

This comment has been minimized.

Copy link
Contributor

Ocramius commented Jan 13, 2020

@ntzm (array), (int), (float) and (bool) casts do not go through magic methods: there has been a separate discussion about that at https://externals.io/message/105589

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:stringable branch 3 times, most recently from d7a345c to 52d1736 Jan 13, 2020
@slavcodev

This comment has been minimized.

Copy link

slavcodev commented Jan 13, 2020

On purpose also, for BC, classes that implement __toString aren't required to declare the interface.
Such classes wouldn't pass the above union type, but that's expected, as that's the semantics of the language.

Is there any reason to keep the magic method __toString in new interface then?

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 13, 2020

Is there any reason to keep the magic method __toString in new interface then?

of course there is: we want to be sure, from a type pov, that the object implements the casting magic method.

@slavcodev

This comment has been minimized.

Copy link

slavcodev commented Jan 13, 2020

But for example JsonSerializable does not require magic method.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:stringable branch 6 times, most recently from 5d9cb20 to 75eae57 Jan 13, 2020
ext/spl/spl_directory.h Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:stringable branch from 75eae57 to 4e344b1 Jan 13, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 13, 2020

Is there any reason to keep the magic method __toString in new interface then?
But for example JsonSerializable does not require magic method.

I think I don't understand your question. I have a few ideas about what you may mean but can't decide which one is the good one. Can you give examples of what you mean? Or more details, please?

@slavcodev

This comment has been minimized.

Copy link

slavcodev commented Jan 13, 2020

Sure, I mean something like

interface Stringable
{
   function toString(): string;
}

seems this also solves the issue with return type and BC.

lassemettovaara referenced this pull request in laravel/framework Jan 14, 2020
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:stringable branch 4 times, most recently from aa675c5 to 0789a35 Jan 14, 2020
@GrahamCampbell

This comment has been minimized.

Copy link
Contributor

GrahamCampbell commented Jan 14, 2020

Is there an RFC for this. Maybe pose the two options for a vote, with three choices. No interface, option one, option two?

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 14, 2020

@GrahamCampbell it's way too early to think about voting options. There has been very little discussion on the matter, and there has been none on php-internals, which is still the only official way AFAIK.

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 14, 2020

I think I'm on the edge of what I can provide here, on the C-side of the proposal. @nikic, @sgolemon and maybe other persons that know the engine well, I might need your help when you can... :)

@bwoebi

This comment has been minimized.

Copy link
Contributor

bwoebi commented Jan 14, 2020

I think the most useful variant of this would be rather an autoapplied interface to anything implementing __toString() instead of requiring an explicit declaration (i.e. duck typing).
We might add a deprecation notice that __toString() without the interface will eventually disappear, but not sure whether that's a good idea. (P.s.: I do not think this is a good idea either, but better than the second commit.)

In particular with the second commit, toString() is not magic any more and I do not like that. __toString() has always been a debugging tool similarly to __debugInfo() (just as concise string instead of bigger array). It just makes it harder to "quickly debug it (i.e. just print the var)" - or what I expect what will happen - people are going to implement __toString() and toString() which is just plain redundant.
As it stands now with this second commit I will probably reject this proposal.

To avoid a noticeable BC break in the former case, I'd rather propose to implicitly set string as return type whenever the compiler encounters a __toString(). So that function __toString() {} and function __toString(): string {} are strictly equivalent post-compilation. (This IMHO should happen anyway, independently of this RFC if this RFC does not pass.)

@slavcodev

This comment has been minimized.

Copy link

slavcodev commented Jan 14, 2020

I expect what will happen - people are going to implement __toString() and toString() which is just plain redundant.

I think the proposal is to equalize the behaviors of objects which implement __toString() and those which implement Stringable interface. No need to implement both. I guess most of new classes will be with no magic, the old classes contrinue to work and the migration is simple enough, just need to rename method or make an alias.

@bwoebi

This comment has been minimized.

Copy link
Contributor

bwoebi commented Jan 14, 2020

Let me repeat myself: the purpose of __toString() is being able to write echo $e; and immediately dumping a string representation of the variable - old style debugging.

I do not think we should ever have objects which disguise themselves as strings. For that we shall have concretely named functions, whose name is not toString(), but which have a name indicating what part(s) of the internal structure is dumped. We do not need a globally defined interface for this. As an example, you can have a class Name { public string $first; public string $last; } (I know names are not that simple, but to illustrate). It might be tempting to just define function toString() { return "{$this->first} {$this->last}"; } … or you could just meaningfully define a function fullname(). You could obviously add a function __toString() as well to be able to quickly print the relevant state of the object. But __toString() is like __debugInfo(): do not invoke it in your actual logic.

And as such, I know it's tempting and people are doing it, we shall not make a generic Stringable::toString() a first class citizen in PHP.

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 14, 2020

@bwoebi you may be missing the use cases for these, but there are very legit ones that do need type safety in PHP8 with union types. Drupal, for example, is using __toString to lazily compute some error/validation messages when they are otherwise too heavy to compute and can be discarded before the computation happens. I hope other people with voting rights will consider php-internals should support existing practices in userland rather that letting them in the dust with a "too bad for you we don't care" position... (I'm not saying that's your position, but I hope that's not your position!)

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 14, 2020

For reference, here are some annotations in Symfony, added by contributions from real-world use cases and that currently cannot be expressed precisely enough using any union types in PHP 8:
https://github.com/symfony/symfony/search?q=%22%40param+string%7Cobject%22+stringable&unscoped_q=%22%40param+string%7Cobject%22+stringable

@bwoebi

This comment has been minimized.

Copy link
Contributor

bwoebi commented Jan 14, 2020

So, you are essentially saying that your main use case is delayed computation of strings?
What's the advantage of introducing stringable instead of function():string (possibly type aliased to stringable)? I think we should really rather push for the generically applicable syntax than just the restricted subset.

I even could imagine some sort of duck typing: function foo(function __toString():string $strCallable) { $computedString = $strCallable(); } - if the object has a __toString method (or a closure is passed), that specific function will be passed.

While this should also cover this use case, it would be much wider in scope and I think also generally useful.

Your proposal seems to me like a solution to quite a narrow problem which has a general useful possible solution.

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 15, 2020

I'd rather propose to implicitly set string as return type whenever the compiler encounters a __toString(). So that function __toString() {} and function __toString(): string {} are strictly equivalent post-compilation.

@bwoebi do you know how to do this? I'd be happy to revert to the first commit if that's possible! can you help me achieve this? (I figured out how)

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 15, 2020

Thanks for the feedback @bwoebi

The PR is now updated with:

interface Stringable
{
   public function __toString(): string;
}

The string return type is now auto-declared in zend_compile.c so that BC is preserved and the upgrade path from PHP7 remains easy.

In PHP7, we'll be able to declare a same-name polyfilled interface without the return type, while still providing forward compatibility with PHP8, and BC with existing code.

This approach solves both my main goals:

  1. providing a type-safe solution to mean string | object-with-__toString()
  2. providing a forward upgrade path from PHP7 to 8

@slavcodev about your comment in #5083 (comment), I think what @bwoebi wrote makes sense: userland doesn't need help from internals to define an interface with a toString() method - and internals should remain as simple as possible.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:stringable branch from 7de961f to 110c834 Jan 15, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 15, 2020

I now added a draft RFC on https://wiki.php.net/rfc, see https://wiki.php.net/rfc/stringable
I would appreciate any kind of feedback to improve it before submitting to the mailing list!

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jan 15, 2020

@nicolas-grekas Use %%__toString()%% to avoid formatting issues.

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 15, 2020

Thanks @nikic, formatting updated!

@slavcodev

This comment has been minimized.

Copy link

slavcodev commented Jan 15, 2020

@slavcodev about your comment in #5083 (comment), I think what @bwoebi wrote makes sense: userland doesn't need help from internals to define an interface with a toString() method - and internals should remain as simple as possible.

I'm sorry, I disagree. Let me try to explain why with my bad English.

I think @bwoebi 's opinion is based on conviction that __toString is used for debugging only which is not so. Many frameworks implemented or going to implement the stringable object, needed to add some behaviours or contraints for the some kinds of strings it's required fairly often in community. Using the same example

class Name {
  public string | \Stringable $firstName;
  public string | \Stringable $firstName;
  public function fullname(): string | \Stringable {
    return "{$this->first} {$this->last}";
  }
}

The new interface solves one issue but not another which it might easily solve.
Furthermore, new interface complicates further progress of PHP in this matter because I hardly see we will have something like NewStringable in next version after this PR.

And finally, I'm just wondering, introducing interface with method __toString,
this method isn't magic anymore, isn't it? Why it starts with __ then?

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 15, 2020

@slavcodev I created #5088 to keep track of the patch with your proposal. You can have a look there for its implications. What immediately strikes me: this requires existing classes to implement both methods, to preserve BC. That's overhead. Then, yes, the method has still the same magical power as the current one: it is called by the engine on any cast-to-string. This has a very technical consequence: having a new method to check adds runtime overhead, as visible in zend_object_handlers.c on the linked PR ($obj instanceof Stringable). Currently, the check for __toString() is heavily optimized, but the new one is not. While I suspect my implementation can be optimized, all this leads me to this feeling: unneeded complexity. The patch in this very PR is straight forward. The other one is a lot less.

@@ -56,8 +53,7 @@ function __wakeup() {}
/** @return string */
final function getTraceAsString() {}

/** @return string */
function __toString() {}
function __toString(): string {}

This comment has been minimized.

Copy link
@deleugpn

deleugpn Jan 16, 2020

Isn't this a BC break that has not been specified in the RFC? https://3v4l.org/9jlIJ
Maybe Throwable should not extend Stringable?

This comment has been minimized.

Copy link
@bwoebi

bwoebi Jan 16, 2020

Contributor

No, it isn't. __toString() has been changed to be implicitly string in the compiler - so it will be implicitly compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.