# [VOTE] get_class() disallow null parameter - Thread: https://externals.io/message/96129 - Original archive: https://news-web.php.net/php.internals/96129 - Started by Dan Ackroyd on 2016-09-24T11:01:58+00:00 - Messages: 12 - 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 --- ## #96129 — Dan Ackroyd — 2016-09-24T11:01:58+00:00 URL: https://externals.io/message/96129#96129 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 --- ## #96131 — Pierre Joye — 2016-09-25T05:29:16+00:00 URL: https://externals.io/message/96129#96131 · In reply to: #96129 Hi > > 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 --- ## #96132 — Leigh — 2016-09-25T09:29:11+00:00 URL: https://externals.io/message/96129#96132 · In reply to: #96131 > 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. --- ## #96133 — Dan Ackroyd — 2016-09-25T12:20:02+00:00 URL: https://externals.io/message/96129#96133 · In reply to: #96131 > 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 --- ## #96134 — Pierre Joye — 2016-09-25T12:31:04+00:00 URL: https://externals.io/message/96129#96134 · In reply to: #96132 > > > > > > […] this > > […] > > > 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. --- ## #96135 — Christoph M. Becker — 2016-09-25T13:19:07+00:00 URL: https://externals.io/message/96129#96135 · In reply to: #96132 > > > > […] > > 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 --- ## #96136 — Niklas Keller — 2016-09-25T17:09:52+00:00 URL: https://externals.io/message/96129#96136 · In reply to: #96135 2016-09-25 15:19 GMT+02:00 Christoph M. Becker : > > > > […] > this > > […] > > 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 --- ## #96138 — Pierre Joye — 2016-09-25T18:58:38+00:00 URL: https://externals.io/message/96129#96138 · In reply to: #96136 > > 2016-09-25 15:19 GMT+02:00 Christoph M. Becker : > > […] is this > > […] > > > I think what matters is the documentation of the return value, not the changelog. > > > […] `FALSE` if object is not an object. > > […] returned. > > > It clearly says "omitted", that's passing nothing at all. Passing `null` doesn't omit an argument, it passes `null`. > > > […] 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. --- ## #96140 — Niklas Keller — 2016-09-25T21:06:17+00:00 URL: https://externals.io/message/96129#96140 · In reply to: #96138 2016-09-25 20:58 GMT+02:00 Pierre Joye : > > > […] > wrote: > > […] > is this > > […] > changelog. > > […] > `FALSE` if object is not an object. > > […] > returned. > > […] > doesn't omit an argument, it passes `null`. > > […] > this > > […] > 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? --- ## #96149 — Pierre Joye — 2016-09-26T09:20:41+00:00 URL: https://externals.io/message/96129#96149 · In reply to: #96140 > 2016-09-25 20:58 GMT+02:00 Pierre Joye : > > […] > > 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 --- ## #96150 — Dan Ackroyd — 2016-09-26T10:13:05+00:00 URL: https://externals.io/message/96129#96150 · In reply to: #96149 > > 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 --- ## #96151 — Christoph M. Becker — 2016-09-26T10:49:24+00:00 URL: https://externals.io/message/96129#96151 · In reply to: #96133 > > > > […] > > 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 : | 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