Re: Fix ReflectionType::__toString() BC break

On Wed, Sep 21, 2016 at 3:14 PM, Nikita Popov <nikita.ppv@gmail.com> wrote: > On Wed, Sep 21, 2016 at 10:55 AM, Nicolas Grekas <nicolas.grekas@gmail.com> > 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 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