[PHP-DEV] Make `always true` SPL methods return void

  110615
June 17, 2020 02:00 carusogabriel34@gmail.com (Gabriel Caruso)
Hello, internals,

Inspired by the bug report #75958 (http://bugs.php.net/75958), I'd like to
change the return type of some SPL methods that are always returning `true`
and only `true`.

These "always `true`" returns make no sense, as you can't wrap it in an
`if`/`else` to catch something that happened, so changing it to `void` and
just invoke these methods sounds reasonable to me.

You can find the PR, with some discussion already, at
https://github.com/php/php-src/pull/5314.

Can I proceed with these changes, or should I create an RFC for that?

-- Gabriel Caruso
  110627
June 17, 2020 10:17 Danack@basereality.com (Dan Ackroyd)
On Wed, 17 Jun 2020 at 03:00, Gabriel Caruso <carusogabriel34@gmail.com> wrote:
> > so changing it to `void` and > just invoke these methods sounds reasonable to me.
What's the benefit of doing the change? There will almost certainly be some code somewhere that would be broken by changing it, so it needs some benefit for doing it. I strongly suspect just updating the documentation to be correct, and then leaving the code behaving as it has been for the past decade or so, would be a better strategy. cheers Dan Ack
  110678
June 19, 2020 03:17 jr@rushlow.dev (Jesse Rushlow)
* Disclaimer - I have not thoroughly reviewed the bug report or PR
mentioned.

From a PHP developers perspective, if I was calling a method that returned
true. I would automatically assume that the method is capable of returning
the inverse of that as well. I see Dan's point of "let sleeping dogs lie"
per say and just adjust the docs. But I'm leaning more towards if a method
can't return false & true, then it should return something else or void.

Thanks
Jesse Rushlow

On Wed, Jun 17, 2020 at 6:18 AM Dan Ackroyd <Danack@basereality.com> wrote:

> On Wed, 17 Jun 2020 at 03:00, Gabriel Caruso <carusogabriel34@gmail.com> > wrote: > > > > so changing it to `void` and > > just invoke these methods sounds reasonable to me. > > What's the benefit of doing the change? > > There will almost certainly be some code somewhere that would be > broken by changing it, so it needs some benefit for doing it. > > I strongly suspect just updating the documentation to be correct, and > then leaving the code behaving as it has been for the past decade or > so, would be a better strategy. > > cheers > Dan > Ack > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: https://www.php.net/unsub.php > >
  110690
June 21, 2020 01:15 johannes@schlueters.de (Johannes =?ISO-8859-1?Q?Schl=FCter?=)
On Thu, 2020-06-18 at 23:17 -0400, Jesse Rushlow wrote:
> * Disclaimer - I have not thoroughly reviewed the bug report or PR > mentioned. > > From a PHP developers perspective, if I was calling a method that > returned true. I would automatically assume that the method is > capable of returning the inverse of that as well.
The fact that it doesn't today, might change tomorrow. If it now returns true this can be changed with less BC issues later. Also returning true allows chaining with `&&`, which void wont't. Also to the specific case: The specific case is about this: $test = new \SplStack(); var_dump( $test->push(1)); Assume we add a SplLimitedStack with a ore-defined size. There push might return false if the stack is full. By having "all" push() methods following the same pattern, generic code can work on either the same way. Thus looking at a single method isn't good for a design, but one should look at a class of types. (and then it might be a decision that push always returns void and errors should be reported via exception) Just to point out that there is a bit more than "oh, this always returns true" Also another side note: We have echo and print. print purposely always returns true for being usable in expression chains. johannes
  110685
June 19, 2020 13:42 ocramius@gmail.com (Marco Pivetta)
Hey Dan, Gabriel,

On Wed, Jun 17, 2020 at 12:17 PM Dan Ackroyd <Danack@basereality.com> wrote:

> On Wed, 17 Jun 2020 at 03:00, Gabriel Caruso <carusogabriel34@gmail.com> > wrote: > > > > so changing it to `void` and > > just invoke these methods sounds reasonable to me. > > What's the benefit of doing the change? > > There will almost certainly be some code somewhere that would be > broken by changing it, so it needs some benefit for doing it. > > I strongly suspect just updating the documentation to be correct, and > then leaving the code behaving as it has been for the past decade or > so, would be a better strategy. >
Perhaps this is a good chance to fix the quirks around the `false` type, as well as introducing the `true` type (and using it for these specific functions)? See https://3v4l.org/ZnWmc/rfc#output See https://3v4l.org/MqsvC/rfc#output Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/