Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [VOTE] get_class() disallow null parameter »

[VOTE] get_class() disallow null parameter

php.internals

Dan Ackroyd

9 years ago
Hi, I'm going to put the 'get_class() disallow null parameter' RFC to a vote: https://wiki.php.net/rfc/get_class_disallow_null_parameter I realise that some people might prefer to have it target 8.0, rather than 7.2. While I appreciate that is a reasonable point of view, I personally believe that more people are hitting this as a bug than are using it deliberately. As the behaviour was altered in a point release, fixing it as a bug in a minor release seems reasonable to me. Voting will end on the 8th October 2016 9pm UTC. cheers Dan Ack

Pierre Joye

9 years ago
Hi On Sep 24, 2016 6:02 PM, "Dan Ackroyd" <danack@basereality.com> wrote:
> > Hi, > > I'm going to put the 'get_class() disallow null parameter' RFC to a > vote: https://wiki.php.net/rfc/get_class_disallow_null_parameter > > I realise that some people might prefer to have it target 8.0, rather > than 7.2. While I appreciate that is a reasonable point of view, I > personally believe that more people are hitting this as a bug than are > using it deliberately. > > As the behaviour was altered in a point release, fixing it as a bug in > a minor release seems reasonable to me.
We can agree that the behavior is debatable but I am pretty sure it is by design (for what I can remember) and can make sense (can be considered ugly but still). Also this behavior is clearly documented: http://th1.php.net/manual/en/function.get-class.php "If object is omitted when inside a class, the name of that class is returned." I am opposed to break BC because we change our mind about how clean is this behavior and I recommend the (future) RMs to veto this change. Cheers Pierre

Leigh

9 years ago
On Sun, 25 Sep 2016 at 06:29 Pierre Joye <pierre.php@gmail.com> wrote:
> Also this behavior is clearly documented: > > http://th1.php.net/manual/en/function.get-class.php > > "If object is omitted when inside a class, the name of that class is > returned." > > I am opposed to break BC because we change our mind about how clean is this > behavior and I recommend the (future) RMs to veto this change. > > Cheers > Pierre >
This is ambiguous at best. "Omitted" and "Not omitted but set to null" are different things. I'm +/-0 on the RFC, but I would not agree that the documentation is clear.

Pierre Joye

9 years ago
On Sep 25, 2016 4:29 PM, "Leigh" <leight@gmail.com> wrote:
> > > > On Sun, 25 Sep 2016 at 06:29 Pierre Joye <pierre.php@gmail.com> wrote: >> >> Also this behavior is clearly documented: >> >> http://th1.php.net/manual/en/function.get-class.php >> >> "If object is omitted when inside a class, the name of that class is >> returned." >> >> I am opposed to break BC because we change our mind about how clean is
this
>> behavior and I recommend the (future) RMs to veto this change. >> >> Cheers >> Pierre > > > This is ambiguous at best. > > "Omitted" and "Not omitted but set to null" are different things. > > I'm +/-0 on the RFC, but I would not agree that the documentation is
clear. I agree but it is how it is now. I do not see why we should yet again to try to "clean up" things within 7.x. let at best deprecate this behavior and remove in 8 This is kind of tiny changes that make migrations painful. We had a good serie of releases in 5.x, 7.0 had BC breaks but good and acceptable ones given what is 7.0. we should really try to continue on this road, 7.0 is behind us and we are back to stable branches from a BC pov.

Christoph Becker

9 years ago
On 25.09.2016 at 11:29, Leigh wrote:
> On Sun, 25 Sep 2016 at 06:29 Pierre Joye <pierre.php@gmail.com> wrote: > >> Also this behavior is clearly documented: >> >> http://th1.php.net/manual/en/function.get-class.php >> >> "If object is omitted when inside a class, the name of that class is >> returned." >> >> I am opposed to break BC because we change our mind about how clean is this >> behavior and I recommend the (future) RMs to veto this change. > > This is ambiguous at best. > > "Omitted" and "Not omitted but set to null" are different things.
However, the changelog entry for 5.3.0 states: | NULL became the default value for object, so passing NULL to object | now has the same result as not passing any value. And that's what I would expect when reading the function signature; after all, NULL is the default value of $object.
-- Christoph M. Becker

Niklas Keller

9 years ago
2016-09-25 15:19 GMT+02:00 Christoph M. Becker <cmbecker69@gmx.de>:
> On 25.09.2016 at 11:29, Leigh wrote: > > > On Sun, 25 Sep 2016 at 06:29 Pierre Joye <pierre.php@gmail.com> wrote: > > > >> Also this behavior is clearly documented: > >> > >> http://th1.php.net/manual/en/function.get-class.php > >> > >> "If object is omitted when inside a class, the name of that class is > >> returned." > >> > >> I am opposed to break BC because we change our mind about how clean is > this > >> behavior and I recommend the (future) RMs to veto this change. > > > > This is ambiguous at best. > > > > "Omitted" and "Not omitted but set to null" are different things. > > However, the changelog entry for 5.3.0 states: > > | NULL became the default value for object, so passing NULL to object > | now has the same result as not passing any value. > > And that's what I would expect when reading the function signature; > after all, NULL is the default value of $object.
I think what matters is the documentation of the return value, not the changelog. Returns the name of the class of which object is an instance. Returns FALSE
> if object is not an object. > If object is omitted when inside a class, the name of that class is > returned.
It clearly says "omitted", that's passing nothing at all. Passing `null` doesn't omit an argument, it passes `null`. I am opposed to break BC because we change our mind about how clean is this I guess most code that might pass null is probably broken, do you have a use case where the current behavior even makes sense? Regards, Niklas

Pierre Joye

9 years ago
On Sep 26, 2016 12:09 AM, "Niklas Keller" <me@kelunik.com> wrote:
> > 2016-09-25 15:19 GMT+02:00 Christoph M. Becker <cmbecker69@gmx.de>: >> >> On 25.09.2016 at 11:29, Leigh wrote: >> >> > On Sun, 25 Sep 2016 at 06:29 Pierre Joye <pierre.php@gmail.com> wrote: >> > >> >> Also this behavior is clearly documented: >> >> >> >> http://th1.php.net/manual/en/function.get-class.php >> >> >> >> "If object is omitted when inside a class, the name of that class is >> >> returned." >> >> >> >> I am opposed to break BC because we change our mind about how clean
is this
>> >> behavior and I recommend the (future) RMs to veto this change. >> > >> > This is ambiguous at best. >> > >> > "Omitted" and "Not omitted but set to null" are different things. >> >> However, the changelog entry for 5.3.0 states: >> >> | NULL became the default value for object, so passing NULL to object >> | now has the same result as not passing any value. >> >> And that's what I would expect when reading the function signature; >> after all, NULL is the default value of $object. > > > I think what matters is the documentation of the return value, not the
changelog.
> >> Returns the name of the class of which object is an instance. Returns
FALSE if object is not an object.
>> >> If object is omitted when inside a class, the name of that class is
returned.
> > > It clearly says "omitted", that's passing nothing at all. Passing `null`
doesn't omit an argument, it passes `null`.
> >> I am opposed to break BC because we change our mind about how clean is
this
> > > I guess most code that might pass null is probably broken, do you have a
use case where the current behavior even makes sense? Probably is sadly not a fact. We restore it because it was breaking code. If I remember correctly it was due to some other non existent features that allows the same (or not working). All I am saying is this is a BC break with little to no value because what willing to support 7.1+ only can simply use the alternatives.

Niklas Keller

9 years ago
2016-09-25 20:58 GMT+02:00 Pierre Joye <pierre.php@gmail.com>:
> On Sep 26, 2016 12:09 AM, "Niklas Keller" <me@kelunik.com> wrote: > > > > 2016-09-25 15:19 GMT+02:00 Christoph M. Becker <cmbecker69@gmx.de>: > >> > >> On 25.09.2016 at 11:29, Leigh wrote: > >> > >> > On Sun, 25 Sep 2016 at 06:29 Pierre Joye <pierre.php@gmail.com> > wrote: > >> > > >> >> Also this behavior is clearly documented: > >> >> > >> >> http://th1.php.net/manual/en/function.get-class.php > >> >> > >> >> "If object is omitted when inside a class, the name of that class is > >> >> returned." > >> >> > >> >> I am opposed to break BC because we change our mind about how clean > is this > >> >> behavior and I recommend the (future) RMs to veto this change. > >> > > >> > This is ambiguous at best. > >> > > >> > "Omitted" and "Not omitted but set to null" are different things. > >> > >> However, the changelog entry for 5.3.0 states: > >> > >> | NULL became the default value for object, so passing NULL to object > >> | now has the same result as not passing any value. > >> > >> And that's what I would expect when reading the function signature; > >> after all, NULL is the default value of $object. > > > > > > I think what matters is the documentation of the return value, not the > changelog. > > > >> Returns the name of the class of which object is an instance. Returns > FALSE if object is not an object. > >> > >> If object is omitted when inside a class, the name of that class is > returned. > > > > > > It clearly says "omitted", that's passing nothing at all. Passing `null` > doesn't omit an argument, it passes `null`. > > > >> I am opposed to break BC because we change our mind about how clean is > this > > > > > > I guess most code that might pass null is probably broken, do you have a > use case where the current behavior even makes sense? > > Probably is sadly not a fact. > > We restore it because it was breaking code. If I remember correctly it was > due to some other non existent features that allows the same (or not > working). > > All I am saying is this is a BC break with little to no value because what > willing to support 7.1+ only can simply use the alternatives. >
Which alternatives?

Pierre Joye

9 years ago
On Mon, Sep 26, 2016 at 4:06 AM, Niklas Keller <me@kelunik.com> wrote:
> 2016-09-25 20:58 GMT+02:00 Pierre Joye <pierre.php@gmail.com>:
>> All I am saying is this is a BC break with little to no value because what >> willing to support 7.1+ only can simply use the alternatives. > > Which alternatives?
Wrong wording. In any case this kind of BC breaks in minor releases bring nothing and should be done in a major release. I think it is clear now that this behavior is by design and was decided back then with 5.3. I do not see any valid reasons to break valid working code because of some concepts about APIs cleanness. Or we can just start to break other that are much annoying, in any minor release. Cheers,
-- Pierre @pierrejoye | http://www.libgd.org

Dan Ackroyd

9 years ago
> On 26 Sep 2016, at 10:20, Pierre Joye <pierre.php@gmail.com> wrote: > I think it is > clear now that this behavior is by design
No it is not. You ignored my previous email which showed the commit message doesn't do what it says. The code doesn't do what it says in the manual either. If you're not going to read facts that contradict your perception of reality, then this isn't a useful conversation. cheers Dan Ack

Dan Ackroyd

9 years ago
On 25 September 2016 at 06:29, Pierre Joye <pierre.php@gmail.com> wrote:
> I am pretty sure it is by design (for what I can remember)
I do not believe that is correct. The commit message* says: "Restore PHP-5.2 behaviour when passing null inside object scope to get_class()" The commit does not restore the previous behaviour.** I'm pretty certain it was just a mistake. As Leigh noted, the behaviour is not clearly documented. In fact the manual also says: "If get_class() is called with anything other than an object, an E_WARNING level error is raised.". I'm pretty sure that null is not an object, so this RFC actually moves the behaviour closer to what the manual says.
> I recommend the (future) RMs to veto this change.
It is the role for Release Managers to prevent accidental changes happening, and to prevent changes being applied to releases when they haven't gone through the RFC process. Is is not the role of a Release Manager to prevent clear RFCs that have been through the RFC process from being applied. cheers Dan Ack * https://github.com/php/php-src/commit/dc74c5136e227bdc2ba1f2ce6f99170a7cf18cc8#diff-3ea93da7d1026a9f53948a5d14b1f7bd ** https://3v4l.org/f3G4N

Christoph Becker

9 years ago
On 25.09.2016 at 14:20, Dan Ackroyd wrote:
> On 25 September 2016 at 06:29, Pierre Joye <pierre.php@gmail.com> wrote: > >> I am pretty sure it is by design (for what I can remember) > > I do not believe that is correct. > > The commit message* says: "Restore PHP-5.2 behaviour when passing null > inside object scope to get_class()" > > The commit does not restore the previous behaviour.** I'm pretty > certain it was just a mistake.
Indeed, the commit message doesn't fit to the commit. However, that does not necessarily mean that the behavior was deliberately changed. See cataphract's explanation in <https://bugs.php.net/bug.php?id=52605>: | The convention is to give internal functions that have optional | arguments a default value and have them behave the same way when the | default value is given and when the argument's not passed at all. IMHO, that's how it should be. Behaving otherwise would contradict the Principle of least astonishment. Perhaps we better should have two functions, say get_current_class(void) and get_class_of(object $object), and deprecate get_class().
> As Leigh noted, the behaviour is not clearly documented. In fact the > manual also says: "If get_class() is called with anything other than > an object, an E_WARNING level error is raised.". I'm pretty sure that > null is not an object, so this RFC actually moves the behaviour closer > to what the manual says.
… but would contradict the changlog entry for PHP 5.3.0.
-- Christoph M. Becker