Everything

php

Home » Project » Newsgroups » php.internals » Fix ReflectionType::__toString() BC break »

Fix ReflectionType::__toString() BC break

php.internals

Nicolas Grekas

9 years ago

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

Johannes Schlueter

9 years ago

Hi,

On Wed, 2016-09-21 at 10:55 +0200, Nicolas Grekas wrote:

> 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

Nicolas Grekas

9 years ago

> 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

Nikita Popov

9 years ago

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

Niklas Keller

9 years ago

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
> > `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
>

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

Marco Pivetta

9 years ago

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.

On 21 Sep 2016 15:27, "Niklas Keller" <me@kelunik.com> wrote:

Levi Morrison

9 years ago

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().

On Wed, Sep 21, 2016 at 7:31 AM, Marco Pivetta <ocramius@gmail.com> wrote:

Julien Pauli

9 years ago

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

Nicolas Grekas

9 years ago

>
> 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

Nikita Popov

9 years ago

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

Ryan Pallas

9 years ago

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 Grekas

9 years ago

>
> 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...

Ryan Pallas

9 years ago

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 Grekas

9 years ago

> 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.

Levi Morrison

9 years ago

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
> 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.

Levi Morrison

9 years ago

On Wed, Sep 21, 2016 at 12:07 PM, Levi Morrison <levim@php.net> wrote:

> 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
>> 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.

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.

Marco Pivetta

9 years ago

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 8:07 PM, Levi Morrison <levim@php.net> wrote:

Jakub Zelenka

9 years ago

Hi,

On Wed, Sep 21, 2016 at 7:07 PM, Levi Morrison <levim@php.net> wrote:

> 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
> > 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.
>
>

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

Levi Morrison

9 years ago

On Wed, Sep 21, 2016 at 12:44 PM, Jakub Zelenka <bukka@php.net> wrote:

> Hi,
>
> On Wed, Sep 21, 2016 at 7:07 PM, Levi Morrison <levim@php.net> wrote:
>>
>> 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
>> > 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.
>>
>
> 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.

Ryan Pallas

9 years ago

On Wed, Sep 21, 2016 at 12:44 PM, Jakub Zelenka <bukka@php.net> wrote:

> Hi,
>
> On Wed, Sep 21, 2016 at 7:07 PM, Levi Morrison <levim@php.net> wrote:
>
>> 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
>> > 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.
>>
>>
> 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

Pierre Joye

9 years ago

On Sep 22, 2016 1:07 AM, "Levi Morrison" <levim@php.net> wrote:

>
> 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
> > 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,

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.

Levi Morrison

9 years ago

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
> `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.

Nicolas Grekas

9 years ago

> > See https://github.com/php/php-src/pull/2136
>
> 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.

Nikita Popov

9 years ago

On Wed, Sep 21, 2016 at 8:47 PM, Nicolas Grekas <nicolas.grekas@gmail.com>
wrote:

> > > See https://github.com/php/php-src/pull/2136
> >
> > 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.
>

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

Marco Pivetta

9 years ago

On Wed, Sep 21, 2016 at 9:46 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:

> 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/