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
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
nullis 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
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.
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
nullis allowed by their default values.But since
zend_boolis really anunsigned char, we have plenty of room
to store and forward this semantic subtlety (nullable being set through
?Foo $argvsFoo $arg = null) so thatReflectionType::__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
2016-09-21 15:14 GMT+02:00 Nikita Popov nikita.ppv@gmail.com:
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
nullis allowed by their default values.But since
zend_boolis really anunsigned char, we have plenty of
room
to store and forward this semantic subtlety (nullable being set through
?Foo $argvsFoo $arg = null) so thatReflectionType::__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
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
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 nikita.ppv@gmail.com:
On Wed, Sep 21, 2016 at 10:55 AM, Nicolas Grekas <
nicolas.grekas@gmail.comwrote:
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
nullis allowed by their default values.But since
zend_boolis really anunsigned char, we have plenty of
room
to store and forward this semantic subtlety (nullable being set through
?Foo $argvsFoo $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
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 propergetName()starting with PHP 7.1.Regards, Niklas
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 (= nullscenario) when run on 7.1, effectively preventing upgrade,
Niklas.2016-09-21 15:14 GMT+02:00 Nikita Popov nikita.ppv@gmail.com:
On Wed, Sep 21, 2016 at 10:55 AM, Nicolas Grekas <
nicolas.grekas@gmail.comwrote:
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
nullis allowed by their default values.But since
zend_boolis really anunsigned char, we have plenty of
room
to store and forward this semantic subtlety (nullable being set through
?Foo $argvsFoo $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
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 propergetName()starting with PHP 7.1.Regards, Niklas
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
nullis allowed by their default values.But since
zend_boolis really anunsigned char, we have plenty of room
to store and forward this semantic subtlety (nullable being set through
?Foo $argvsFoo $arg = null) so thatReflectionType::__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
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
On Wed, Sep 21, 2016 at 6:13 PM, Nicolas Grekas nicolas.grekas@gmail.com
wrote:
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
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
On Wed, Sep 21, 2016 at 10:13 AM, Nicolas Grekas nicolas.grekas@gmail.com
wrote:
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 :)
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
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...
On Wed, Sep 21, 2016 at 10:34 AM, Nicolas Grekas nicolas.grekas@gmail.com
wrote:
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.
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...
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.
On Wed, Sep 21, 2016 at 11:13 AM, Nicolas Grekas
nicolas.grekas@gmail.com wrote:
To handle this in code written around current __toString seems pretty
simpleYes 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.
On Wed, Sep 21, 2016 at 11:13 AM, Nicolas Grekas
nicolas.grekas@gmail.com wrote:To handle this in code written around current __toString seems pretty
simpleYes 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.
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.
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
On Wed, Sep 21, 2016 at 11:13 AM, Nicolas Grekas
nicolas.grekas@gmail.com wrote:To handle this in code written around current __toString seems pretty
simpleYes 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.
Hi,
On Wed, Sep 21, 2016 at 11:13 AM, Nicolas Grekas
nicolas.grekas@gmail.com wrote:To handle this in code written around current __toString seems pretty
simpleYes 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.
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
Hi,
On Wed, Sep 21, 2016 at 11:13 AM, Nicolas Grekas
nicolas.grekas@gmail.com wrote:To handle this in code written around current __toString seems pretty
simpleYes 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.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.
Hi,
On Wed, Sep 21, 2016 at 11:13 AM, Nicolas Grekas
nicolas.grekas@gmail.com wrote:To handle this in code written around current __toString seems pretty
simpleYes 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.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
Hi,
On Wed, Sep 21, 2016 at 11:13 AM, Nicolas Grekas
nicolas.grekas@gmail.com wrote:To handle this in code written around current __toString seems pretty
simpleYes 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.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
Yeah my bad, missed that one! Thanks
On Wed, Sep 21, 2016 at 11:13 AM, Nicolas Grekas
nicolas.grekas@gmail.com wrote:To handle this in code written around current __toString seems pretty
simpleYes 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,
Which does not allow BC breaks but for extreme cases. I do not consider
this case as extreme, at all.
I share Nicolas concerns here. This is the kind of changes making the
migration path harder than it should without a strong reason behind it.
I agree with Nikita but it is something that can be solved using the
depreciation flag and then handle in the next major.
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?
Obviously the sooner. And what is the next BC breaks for 7.2/3/4?
This is exactly why we introduce the no BC break rule.
In this case it is even more clear as one can use getName if desired to
support 7.1+ only, which I suppose is most likely not the case for a large
majority of users.
On Wed, Sep 21, 2016 at 2: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
nullis allowed by their default values.But since
zend_boolis really anunsigned char, we have plenty of room
to store and forward this semantic subtlety (nullable being set through
?Foo $argvsFoo $arg = null) so thatReflectionType::__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.
On a technical note this is a potentially breaking change for C
extensions. Since it is a currently azend_boolits 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.
On Wed, Sep 21, 2016 at 8:47 PM, Nicolas Grekas nicolas.grekas@gmail.com
wrote:
On a technical note this is a potentially breaking change for C
extensions. Since it is a currently azend_boolits 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.
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
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