# Fix ReflectionType::__toString() BC break - Thread: https://externals.io/message/96045 - Original archive: https://news-web.php.net/php.internals/96045 - Started by Nicolas Grekas on 2016-09-21T08:55:00+00:00 - Messages: 31 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/96045.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #96045 — Nicolas Grekas — 2016-09-21T08:55:00+00:00 URL: https://externals.io/message/96045#96045 Hello, our turn to be hit by this 7.1 BC break in Symfony: `ReflectionType::__toString()` now adds a `?` in front of type hints where `null` is allowed by their default values. But since `zend_bool` is really an `unsigned char`, we have plenty of room to store and forward this semantic subtlety (nullable being set through `?Foo $arg` vs `Foo $arg = null`) so that `ReflectionType::__toString()` can be accurate again, thus BC. See https://github.com/php/php-src/pull/2136 Thank for you consideration of this patch. Cheers, Nicolas --- ## #96046 — johannes@schlueters.de — 2016-09-21T09:56:54+00:00 URL: https://externals.io/message/96045#96046 · In reply to: #96045 Hi, > our turn to be hit by this 7.1 BC break in Symfony: > `ReflectionType::__toString()` now adds a `?` in front of type hints where > `null` is allowed by their default values. Curious: Why do you depend on the string representation? What information is not available in a proper interface? I won't like to call a toString() representation a committed interface. johannes --- ## #96047 — Nicolas Grekas — 2016-09-21T10:12:02+00:00 URL: https://externals.io/message/96045#96047 · In reply to: #96046 > Curious: Why do you depend on the string representation? __toString is the only public interface on ReflectionType that allows getting the type name, so people already build things on top. See http://php.net/ReflectionType --- ## #96052 — Nikita Popov — 2016-09-21T13:14:47+00:00 URL: https://externals.io/message/96045#96052 · In reply to: #96045 On Wed, Sep 21, 2016 at 10:55 AM, Nicolas Grekas wrote: > Hello, > > our turn to be hit by this 7.1 BC break in Symfony: > `ReflectionType::__toString()` now adds a `?` in front of type hints where > `null` is allowed by their default values. > > But since `zend_bool` is really an `unsigned char`, we have plenty of room > to store and forward this semantic subtlety (nullable being set through > `?Foo $arg` vs `Foo $arg = null`) so that `ReflectionType::__toString()` > can be accurate again, thus BC. > > See https://github.com/php/php-src/pull/2136 > > Thank for you consideration of this patch. > I don't like this. The way I see it, either ReflectionType::__toString() gives you a complete representation of the type (which includes "?" no matter how you happen to write it in the source code) or it does not -- in which case this function is fundamentally broken and needs to go away. In other words, either we stick with the behavior currently implemented in PHP 7.1, or we go back to the PHP 7 behavior (there will never be a leading ?, it just returns the name) **and we deprecate the method**. I consider the last part important because going to the PHP 7 behavior fundamentally breaks the method (it is not actually a string representation of the type). Furthermore, this forces consumers of ReflectionType to switch to the new API based on getName() and isNullable(). Nikita --- ## #96055 — Niklas Keller — 2016-09-21T13:27:29+00:00 URL: https://externals.io/message/96045#96055 · In reply to: #96052 2016-09-21 15:14 GMT+02:00 Nikita Popov : > On Wed, Sep 21, 2016 at 10:55 AM, Nicolas Grekas > […] > wrote: > > > […] > where > > […] > room > > […] > > I don't like this. The way I see it, either ReflectionType::__toString() > gives you a complete representation of the type (which includes "?" no > matter how you happen to write it in the source code) or it does not -- in > which case this function is fundamentally broken and needs to go away. In > other words, either we stick with the behavior currently implemented in PHP > 7.1, or we go back to the PHP 7 behavior (there will never be a leading ?, > it just returns the name) **and we deprecate the method**. > > I consider the last part important because going to the PHP 7 behavior > fundamentally breaks the method (it is not actually a string representation > of the type). Furthermore, this forces consumers of ReflectionType to > switch to the new API based on getName() and isNullable(). > > Nikita > As Nikita said, starting with PHP 7.1, we'll have ReflectionType::getName() which does exactly what you want. You can use `__toString()` on previous versions and use the proper `getName()` starting with PHP 7.1. Regards, Niklas --- ## #96056 — Marco Pivetta — 2016-09-21T13:31:14+00:00 URL: https://externals.io/message/96045#96056 · In reply to: #96055 That still means that code designed for 7.0 will break while reflecting 7.0 code (`= null` scenario) when run on 7.1, effectively preventing upgrade, Niklas. > 2016-09-21 15:14 GMT+02:00 Nikita Popov : > > > […] > nicolas.grekas@gmail.com > > […] > `ReflectionType::__toString()` > > […] > in > > […] > PHP > > […] > ?, > > […] > representation > > […] > > As Nikita said, starting with PHP 7.1, we'll have ReflectionType::getName() > which does exactly what you want. You can use `__toString()` on previous > versions and use the proper `getName()` starting with PHP 7.1. > > Regards, Niklas --- ## #96057 — Julien Pauli — 2016-09-21T13:32:38+00:00 URL: https://externals.io/message/96045#96057 · In reply to: #96052 > On Wed, Sep 21, 2016 at 10:55 AM, Nicolas Grekas > wrote: > > > […] > > I don't like this. The way I see it, either ReflectionType::__toString() > gives you a complete representation of the type (which includes "?" no > matter how you happen to write it in the source code) or it does not -- in > which case this function is fundamentally broken and needs to go away. In > other words, either we stick with the behavior currently implemented in PHP > 7.1, or we go back to the PHP 7 behavior (there will never be a leading ?, > it just returns the name) **and we deprecate the method**. > > I consider the last part important because going to the PHP 7 behavior > fundamentally breaks the method (it is not actually a string representation > of the type). Furthermore, this forces consumers of ReflectionType to > switch to the new API based on getName() and isNullable(). > > Nikita People used __toString() because there was no other way to "get the name" of the param. But __toString() should give a "string representation of the objects it is affected to". The fact that in 7.0 it returned the exact name of the param, was a coincidence. Now, in 7.1 , we changed it to represent the param more accurately, including the nullable fact, thus adding a '?' to the type ; BUT we also added a way to get the name of the param (like you expect it) : the getName() method. So now, in 7.1 , one who used __toString() should now use getName(). And __toString() changed in 7.1 , breaking BC yes. Should the break stay ? It is RM to decide. But, if we must wait for PHP 8 to be able to change so little things, then we perhaps need to change our release management to have one major per year ? I mean, we need to change things, we need to evoluate , and that can't be done without breaking BC in some parts. That is all the challenge behind BC breaks .... RM thoughts ? Julien.Pauli --- ## #96062 — Levi Morrison — 2016-09-21T14:39:58+00:00 URL: https://externals.io/message/96045#96062 · In reply to: #96056 How big of a deal will this be come 7.2? or 8.0? A few projects will break on 7.1 regardless of whether the question mark is prepended or not, as has already been discussed in previous threads. The key is as Nikita has pointed out that if we don't prepend it this method is basically useless and must be deprecated. Whereas if we do prepend it then it works as it supposed to and is distinct from getName(). > That still means that code designed for 7.0 will break while reflecting 7.0 > code (`= null` scenario) when run on 7.1, effectively preventing upgrade, > Niklas. > > > > > […] --- ## #96065 — Nicolas Grekas — 2016-09-21T16:13:09+00:00 URL: https://externals.io/message/96045#96065 · In reply to: #96052 > > See https://github.com/php/php-src/pull/2136 > > […] > > I don't like this. > I understand, really. I do have to deal with BC on Symfony side also and it's a really hard constraint. Yet, we stick to it as much as possible, in order to not add pain to others. Sometime, we have to be pragmatic and accept solutions that are not "pure", for the shake of our BC promise. PHP internals has such a BC policy, it should really stick to it. It's not always fun, for sure, but the pain for others it strong. It's not only me&Ocramius, nor Symfony&Zend: everybody not following internals to adapt to latest BC breaks will be hit, potentially. This is bad for the reputation of PHP. It makes the PHP platform unstable as far as confidence is concerned. In this case, we should find a technical solution that preserves BC. Being this patch or unconditionally returning the type name with nullable info is fine, you'll decide what's best. But please don't consider BC as something that one can bypass when it gets annoying. Thanks from me and from many others for considering :) Nicolas --- ## #96066 — Nikita Popov — 2016-09-21T16:19:41+00:00 URL: https://externals.io/message/96045#96066 · In reply to: #96065 On Wed, Sep 21, 2016 at 6:13 PM, Nicolas Grekas wrote: > > > > […] > > I understand, really. I do have to deal with BC on Symfony side also and > it's a really hard constraint. Yet, we stick to it as much as possible, in > order to not add pain to others. > Sometime, we have to be pragmatic and accept solutions that are not > "pure", for the shake of our BC promise. > > PHP internals has such a BC policy, it should really stick to it. It's not > always fun, for sure, but the pain for others it strong. It's not only > me&Ocramius, nor Symfony&Zend: everybody not following internals to adapt > to latest BC breaks will be hit, potentially. This is bad for the > reputation of PHP. It makes the PHP platform unstable as far as confidence > is concerned. > > In this case, we should find a technical solution that preserves BC. Being > this patch or unconditionally returning the type name with nullable info is > fine, you'll decide what's best. > > But please don't consider BC as something that one can bypass when it gets > annoying. > > Thanks from me and from many others for considering :) > > Nicolas > Thank you for quoting a single sentence out of a larger mail and completely ignoring the rest :/ I don't insist on keeping the current behavior. I'm just saying that if we don't keep it, we should also deprecate the method. This is both backwards-compatible (per the usual interpretation that a deprecation is not a BC break) and ensures that "everybody not following internals to adapt to latest BC breaks" is still made aware that they should change their code to use the getName() API in 7.1 to avoid future problems of this kind. Nikita --- ## #96067 — Ryan Pallas — 2016-09-21T16:30:10+00:00 URL: https://externals.io/message/96045#96067 · In reply to: #96065 On Wed, Sep 21, 2016 at 10:13 AM, Nicolas Grekas wrote: > > […] > > I understand, really. I do have to deal with BC on Symfony side also and > it's a really hard constraint. Yet, we stick to it as much as possible, in > order to not add pain to others. > Sometime, we have to be pragmatic and accept solutions that are not "pure", > for the shake of our BC promise. > > PHP internals has such a BC policy, it should really stick to it. It's not > always fun, for sure, but the pain for others it strong. It's not only > me&Ocramius, nor Symfony&Zend: everybody not following internals to adapt > to latest BC breaks will be hit, potentially. This is bad for the > reputation of PHP. It makes the PHP platform unstable as far as confidence > is concerned. > > In this case, we should find a technical solution that preserves BC. Being > this patch or unconditionally returning the type name with nullable info is > fine, you'll decide what's best. > > But please don't consider BC as something that one can bypass when it gets > annoying. > > Thanks from me and from many others for considering :) > I'm not so sure its a BC though. Technically nullable types don't exist in 7.0 and as such would be impossible to return said question mark. Its only adding support for a new feature, not changing how an existing feature works IMO. If nullable types had been in 7.0 its highly probably, that __toString would have returned the leading question mark back then. > Nicolas --- ## #96068 — Nicolas Grekas — 2016-09-21T16:34:56+00:00 URL: https://externals.io/message/96045#96068 · In reply to: #96067 > > I'm not so sure its a BC though. Technically nullable types don't exist in > 7.0 and as such would be impossible to return said question mark. Its only > adding support for a new feature, not changing how an existing feature > works IMO. If nullable types had been in 7.0 its highly probably, that > __toString would have returned the leading question mark back then. > Here is the BC break: https://3v4l.org/VUbIS As light as it looks to be, the output on PHP7.1RC2 is not the same as in PHP7.0. Yet, __toString() is the only public interface that is available on PHP 7.0, so people have already built things on top. Nicolas p.s. to Nikita: my bad for the too short quote... --- ## #96070 — Ryan Pallas — 2016-09-21T16:54:51+00:00 URL: https://externals.io/message/96045#96070 · In reply to: #96068 On Wed, Sep 21, 2016 at 10:34 AM, Nicolas Grekas wrote: > > > > […] > > Here is the BC break: https://3v4l.org/VUbIS > > As light as it looks to be, the output on PHP7.1RC2 is not the same as in > PHP7.0. > Yet, __toString() is the only public interface that is available on PHP > 7.0, > so people have already built things on top. > > I get that the output changed, but so did the type definition with the new feature. However, I feel this is a problem with the way types are inferred. To me Type $foo = null is an entirely different definition than ?Type $foo, but that's not how its implemented in the language. To me Type $foo = null should say "I have a parameter that is not required, but when its passed it needs to be an instance of Type", whereas ?Type $foo should say "I require this parameter, and it needs to be an instance of Type or null". If these definition were the result of those expressions, then we could have a third state, namely ?Type $foo = null which would say "I do not require this parameter, but when its passed it needs to be an instance of Type or null". I think the reason it works this way is, get this, for BC since previously Type $foo = null was the only way to make a parameter nullable, while having the intentional or unintentional side effect of making it optional (or the only way to make it optional, while having the side effect of making it nullable). However, given the current workings, Type $foo = null IS in fact nullable, therefore the string representation of that type is in fact ?Type. To handle this in code written around current __toString seems pretty simple: $type = method_exists($RefType, 'getName') ? $RefType->getName() : (string) $RefType; Given how simple that is, I'm not understanding what makes it such a big deal for you (or anyone else) either? > > Nicolas > p.s. to Nikita: my bad for the too short quote... --- ## #96071 — Nicolas Grekas — 2016-09-21T17:13:25+00:00 URL: https://externals.io/message/96045#96071 · In reply to: #96070 > To handle this in code written around current __toString seems pretty simple Yes it is, but that's not what we're talking about: BC is about having perfectly fine code working in e.g. 7.0 be still working fine on 7.1 *without any change*. Right now, we have red test suites on php7.1rc2. This is the symptom of a BC break, by definition. And the issue is not the existing code we have, but the new one that is changing the behavior of the engine. --- ## #96072 — Levi Morrison — 2016-09-21T18:07:11+00:00 URL: https://externals.io/message/96045#96072 · In reply to: #96071 On Wed, Sep 21, 2016 at 11:13 AM, Nicolas Grekas wrote: > > […] > simple > > Yes it is, but that's not what we're talking about: > BC is about having perfectly fine code working in e.g. 7.0 be still working > fine on 7.1 *without any change*. > > Right now, we have red test suites on php7.1rc2. > This is the symptom of a BC break, by definition. > And the issue is not the existing code we have, but the new one that is > changing the behavior of the engine. This was understood when the decision was made. You seem to not be understanding the bigger issue and instead focusing on the BC break for a *single minor release, and a dot zero at that*. If we keep the BC compat this method is redundant and useless forever. If we fix it we break your code for *one single minor release, and a dot zero at that*. Which is the bigger disruption? This is why the decision was made. It is better to have the useful functionality from here on out than to preserve BC with a single minor release, and a dot-zero at that. --- ## #96073 — Levi Morrison — 2016-09-21T18:17:40+00:00 URL: https://externals.io/message/96045#96073 · In reply to: #96072 > On Wed, Sep 21, 2016 at 11:13 AM, Nicolas Grekas > wrote: > > […] > > This was understood when the decision was made. You seem to not be > understanding the bigger issue and instead focusing on the BC break > for a *single minor release, and a dot zero at that*. If we keep the > BC compat this method is redundant and useless forever. If we fix it > we break your code for *one single minor release, and a dot zero at > that*. Which is the bigger disruption? > > This is why the decision was made. It is better to have the useful > functionality from here on out than to preserve BC with a single minor > release, and a dot-zero at that. I should also add that the dot-zero is the same version it was released. Having backwards compatibility breaks in the version after its release in some cases is not that uncommon (though unfortunate). Essentially I understand the frustration and empathize with having your code broken. However, I still think it is better for the overall language health to prepend the question mark, which is why it is there in the RC releases despite the previous discussion thread. --- ## #96074 — Marco Pivetta — 2016-09-21T18:19:50+00:00 URL: https://externals.io/message/96045#96074 · In reply to: #96072 It's a point release, it's not really "up for decision" whether BC can be broken or not on functionality that is working as intended (unless I misunderstand the release process). Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/ > On Wed, Sep 21, 2016 at 11:13 AM, Nicolas Grekas > wrote: > > […] > working > > […] > > This was understood when the decision was made. You seem to not be > understanding the bigger issue and instead focusing on the BC break > for a *single minor release, and a dot zero at that*. If we keep the > BC compat this method is redundant and useless forever. If we fix it > we break your code for *one single minor release, and a dot zero at > that*. Which is the bigger disruption? > > This is why the decision was made. It is better to have the useful > functionality from here on out than to preserve BC with a single minor > release, and a dot-zero at that. --- ## #96075 — Levi Morrison — 2016-09-21T18:40:18+00:00 URL: https://externals.io/message/96045#96075 · In reply to: #96045 On Wed, Sep 21, 2016 at 2:55 AM, Nicolas Grekas wrote: > Hello, > > our turn to be hit by this 7.1 BC break in Symfony: > `ReflectionType::__toString()` now adds a `?` in front of type hints where > `null` is allowed by their default values. > > But since `zend_bool` is really an `unsigned char`, we have plenty of room > to store and forward this semantic subtlety (nullable being set through > `?Foo $arg` vs `Foo $arg = null`) so that `ReflectionType::__toString()` > can be accurate again, thus BC. > > See https://github.com/php/php-src/pull/2136 > > Thank for you consideration of this patch. > > Cheers, > Nicolas On a technical note this is a potentially breaking change for C extensions. Since it is a currently a `zend_bool` its value is truthy/falsy; you are now applying specific meaning to values. Maybe someone else will have more insight into what might be affected realistically, but this is one reason this was not implemented this way to begin with. --- ## #96076 — Jakub Zelenka — 2016-09-21T18:44:37+00:00 URL: https://externals.io/message/96045#96076 · In reply to: #96072 Hi, > On Wed, Sep 21, 2016 at 11:13 AM, Nicolas Grekas > wrote: > > […] > working > > […] > > This was understood when the decision was made. You seem to not be > understanding the bigger issue and instead focusing on the BC break > for a *single minor release, and a dot zero at that*. If we keep the > BC compat this method is redundant and useless forever. If we fix it > we break your code for *one single minor release, and a dot zero at > that*. Which is the bigger disruption? > > This is why the decision was made. It is better to have the useful > functionality from here on out than to preserve BC with a single minor > release, and a dot-zero at that. > > I'm just wondering, how is it possible that this got changed when the only RFC mentioning this change got rejected ( https://wiki.php.net/rfc/reflectiontypeimprovements )? I don't see any consensus in the later discussion so unless I missed something, which is quite possible, this change should not be there in the first place, right? Cheers Jakub --- ## #96077 — Nicolas Grekas — 2016-09-21T18:47:18+00:00 URL: https://externals.io/message/96045#96077 · In reply to: #96075 > > […] > > On a technical note this is a potentially breaking change for C > extensions. Since it is a currently a `zend_bool` its value is > truthy/falsy; you are now applying specific meaning to values. > > Maybe someone else will have more insight into what might be affected > realistically, but this is one reason this was not implemented this > way to begin with. > Note that Nikita's proposal (i.e. stick to 7.0 behavior and ignore the "nullable" bit) is free from this problem yet really fine also as far as BC is concerned. --- ## #96078 — Levi Morrison — 2016-09-21T18:47:42+00:00 URL: https://externals.io/message/96045#96078 · In reply to: #96076 > Hi, > > > > […] > > I'm just wondering, how is it possible that this got changed when the only > RFC mentioning this change got rejected ( > https://wiki.php.net/rfc/reflectiontypeimprovements )? I don't see any > consensus in the later discussion so unless I missed something, which is > quite possible, this change should not be there in the first place, right? > > Cheers > > Jakub > You should check the mailing lists in situations like this, and in this situation you will find the answer. Basically everyone who bothered to discuss anything that some minimal changes needed to be made before 7.1 launched or it's forever broken. (Spoiler: it isn't just me) I understand that some people really don't like reading the mailing list but I have no sympathy for people who don't read it then wonder what happened. It's literally our only official communication channel. --- ## #96079 — Ryan Pallas — 2016-09-21T18:51:59+00:00 URL: https://externals.io/message/96045#96079 · In reply to: #96076 > Hi, > > > > > […] > I'm just wondering, how is it possible that this got changed when the only > RFC mentioning this change got rejected ( https://wiki.php.net/rfc/ > reflectiontypeimprovements )? I don't see any consensus in the later > discussion so unless I missed something, which is quite possible, this > change should not be there in the first place, right? > The best I can find are these messages [1] where it specifically mentions that toString should change even though this was rejected, and it had at least some agreement at the time. [1] http://php-news.ctrl-f5.net/message/php.internals/94452 > > Cheers > > Jakub --- ## #96080 — Jakub Zelenka — 2016-09-21T19:11:13+00:00 URL: https://externals.io/message/96045#96080 · In reply to: #96079 > > > > > > […] > > The best I can find are these messages [1] where it specifically mentions > that toString should change even though this was rejected, and it had at > least some agreement at the time. > > [1] http://php-news.ctrl-f5.net/message/php.internals/94452 > > > […] Yeah my bad, missed that one! Thanks --- ## #96081 — Nikita Popov — 2016-09-21T19:46:29+00:00 URL: https://externals.io/message/96045#96081 · In reply to: #96077 On Wed, Sep 21, 2016 at 8:47 PM, Nicolas Grekas wrote: > > […] > > > Note that Nikita's proposal (i.e. stick to 7.0 behavior and ignore the > "nullable" bit) is free from this problem yet really fine also as far as BC > is concerned. > Here's a PR for the revert-to-7.0 + deprecate variant, in case we wish to adopt it: https://github.com/php/php-src/pull/2137 Nikita --- ## #96082 — Marco Pivetta — 2016-09-21T20:00:02+00:00 URL: https://externals.io/message/96045#96082 · In reply to: #96081 > Here's a PR for the revert-to-7.0 + deprecate variant, in case we wish to > adopt it: https://github.com/php/php-src/pull/2137 > Pretty much the most sensible solution to all this, from my point of view. Avoiding overloading a string with more meaning seems like the correct way to go forward on this, making it die with 8.0. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/ --- Next page: https://externals.io/message/96045.md?page=2