[PHP-DEV] [VOTE] Ensure correct signatures of magic methods

  110301
May 29, 2020 16:44 carusogabriel34@gmail.com (Gabriel Caruso)
Hello, internals!

I have opened the voting for
https://wiki.php.net/rfc/magic-methods-signature.

The voting period ends on 2020-06-19 at 18h (CEST).

Thanks,

-- Gabriel Caruso
  110320
May 31, 2020 13:56 nikita.ppv@gmail.com (Nikita Popov)
On Fri, May 29, 2020 at 6:45 PM Gabriel Caruso <carusogabriel34@gmail.com>
wrote:

> Hello, internals! > > I have opened the voting for > https://wiki.php.net/rfc/magic-methods-signature. > > The voting period ends on 2020-06-19 at 18h (CEST). >
The RFC is a bit unclear on what is actually being proposed. It says
> This RFC proposes to add parameter and return types checks per the following details.
and goes on to list (reasonable looking) magic method signatures, but does not say how exactly those types are going to be checked. Is this going to require exactly the same signature, or is this going to be in accordance with variance rules? For example, are all of the following signatures valid under this RFC? Only the first two? None of them? // Narrowed return type from ?array public function __debugInfo(): array {} // Narrowed return type from mixed public function __get(string $name): int {] // Widened argument type from string public function __get(string|array $name): mixed {} Also, is omitting the return type still permitted, even though it would nominally violate variance? public function __debugInfo() {} Finally, if omitting the return type is permitted, will an implicit return type be added, like we do for __toString()? Would the method automatically become public function __debugInfo(): ?array {} and report as such from reflection? Nikita
  110322
May 31, 2020 21:19 carusogabriel34@gmail.com (Gabriel Caruso)
On Sun, 31 May 2020 at 15:57, Nikita Popov ppv@gmail.com> wrote:

> On Fri, May 29, 2020 at 6:45 PM Gabriel Caruso <carusogabriel34@gmail.com> > wrote: > >> Hello, internals! >> >> I have opened the voting for >> https://wiki.php.net/rfc/magic-methods-signature. >> >> The voting period ends on 2020-06-19 at 18h (CEST). >> > > The RFC is a bit unclear on what is actually being proposed. It says > > > This RFC proposes to add parameter and return types checks per the > following details. > > and goes on to list (reasonable looking) magic method signatures, but does > not say how exactly those types are going to be checked. Is this going to > require exactly the same signature, or is this going to be in accordance > with variance rules? For example, are all of the following signatures valid > under this RFC? Only the first two? None of them? > > // Narrowed return type from ?array > public function __debugInfo(): array {} > > // Narrowed return type from mixed > public function __get(string $name): int {] > > // Widened argument type from string > public function __get(string|array $name): mixed {} >
They are going to be checked following the variance rules, not the *exactly* same as the RFC. I'll mention this, thanks for point it out. Assuming this, your examples: 1 and 2. Will be valid, following the rules introduced by the `mixed` RFC. 3. Is that allowed in PHP? If so, the RFC will compliance with that.
> > Also, is omitting the return type still permitted, even though it would > nominally violate variance? > > public function __debugInfo() {} >
Yes, this hasn't changed. The RFC only affects *typed* methods.
> > Finally, if omitting the return type is permitted, will an implicit return > type be added, like we do for __toString()? Would the method automatically > become > > public function __debugInfo(): ?array {} >
An implicit return type won't be added for any of the magic methods. I believe that's a huge BC, and I don't want to debate that for PHP 8 (maybe PHP 9, yes).
> > and report as such from reflection? >
I need more clearance on this one: are you asking how magic methods are reported via Reflection and if that will be changed?
> > Nikita >
  110333
June 2, 2020 12:26 dmitrystogov@gmail.com (Dmitry Stogov)
Does this RFC support __get() returning by reference?
See
https://github.com/php/php-src/blob/master/Zend/tests/overloaded_prop_assign_op_refs.phpt#L11

Thanks. Dmitry.

On Mon, Jun 1, 2020 at 12:20 AM Gabriel Caruso <carusogabriel34@gmail.com>
wrote:

> On Sun, 31 May 2020 at 15:57, Nikita Popov ppv@gmail.com> wrote: > > > On Fri, May 29, 2020 at 6:45 PM Gabriel Caruso < > carusogabriel34@gmail.com> > > wrote: > > > >> Hello, internals! > >> > >> I have opened the voting for > >> https://wiki.php.net/rfc/magic-methods-signature. > >> > >> The voting period ends on 2020-06-19 at 18h (CEST). > >> > > > > The RFC is a bit unclear on what is actually being proposed. It says > > > > > This RFC proposes to add parameter and return types checks per the > > following details. > > > > and goes on to list (reasonable looking) magic method signatures, but > does > > not say how exactly those types are going to be checked. Is this going to > > require exactly the same signature, or is this going to be in accordance > > with variance rules? For example, are all of the following signatures > valid > > under this RFC? Only the first two? None of them? > > > > // Narrowed return type from ?array > > public function __debugInfo(): array {} > > > > // Narrowed return type from mixed > > public function __get(string $name): int {] > > > > // Widened argument type from string > > public function __get(string|array $name): mixed {} > > > > > They are going to be checked following the variance rules, not the > *exactly* same as the RFC. I'll mention this, thanks for point it out. > > Assuming this, your examples: > > 1 and 2. Will be valid, following the rules introduced by the `mixed` RFC. > > 3. Is that allowed in PHP? If so, the RFC will compliance with that. > > > > > > Also, is omitting the return type still permitted, even though it would > > nominally violate variance? > > > > public function __debugInfo() {} > > > > Yes, this hasn't changed. The RFC only affects *typed* methods. > > > > > > Finally, if omitting the return type is permitted, will an implicit > return > > type be added, like we do for __toString()? Would the method > automatically > > become > > > > public function __debugInfo(): ?array {} > > > > An implicit return type won't be added for any of the magic methods. I > believe that's a huge BC, and I don't want to debate that for PHP 8 (maybe > PHP 9, yes). > > > > > > and report as such from reflection? > > > > I need more clearance on this one: are you asking how magic methods are > reported via Reflection and if that will be changed? > > > > > > Nikita > > >
  110353
June 3, 2020 21:05 carusogabriel34@gmail.com (Gabriel Caruso)
> > On Mon, Jun 1, 2020 at 12:20 AM Gabriel Caruso <carusogabriel34@gmail.com> > wrote: > >> On Sun, 31 May 2020 at 15:57, Nikita Popov ppv@gmail.com> wrote: >> >> > On Fri, May 29, 2020 at 6:45 PM Gabriel Caruso < >> carusogabriel34@gmail.com> >> > wrote: >> > >> >> Hello, internals! >> >> >> >> I have opened the voting for >> >> https://wiki.php.net/rfc/magic-methods-signature. >> >> >> >> The voting period ends on 2020-06-19 at 18h (CEST). >> >> >> > >> > The RFC is a bit unclear on what is actually being proposed. It says >> > >> > > This RFC proposes to add parameter and return types checks per the >> > following details. >> > >> > and goes on to list (reasonable looking) magic method signatures, but >> does >> > not say how exactly those types are going to be checked. Is this going >> to >> > require exactly the same signature, or is this going to be in accordance >> > with variance rules? For example, are all of the following signatures >> valid >> > under this RFC? Only the first two? None of them? >> > >> > // Narrowed return type from ?array >> > public function __debugInfo(): array {} >> > >> > // Narrowed return type from mixed >> > public function __get(string $name): int {] >> > >> > // Widened argument type from string >> > public function __get(string|array $name): mixed {} >> > >> >> >> They are going to be checked following the variance rules, not the >> *exactly* same as the RFC. I'll mention this, thanks for point it out. >> >> Assuming this, your examples: >> >> 1 and 2. Will be valid, following the rules introduced by the `mixed` RFC. >> >> 3. Is that allowed in PHP? If so, the RFC will compliance with that. >> >> >> > >> > Also, is omitting the return type still permitted, even though it would >> > nominally violate variance? >> > >> > public function __debugInfo() {} >> > >> >> Yes, this hasn't changed. The RFC only affects *typed* methods. >> >> >> > >> > Finally, if omitting the return type is permitted, will an implicit >> return >> > type be added, like we do for __toString()? Would the method >> automatically >> > become >> > >> > public function __debugInfo(): ?array {} >> > >> >> An implicit return type won't be added for any of the magic methods. I >> believe that's a huge BC, and I don't want to debate that for PHP 8 (maybe >> PHP 9, yes). >> >> >> > >> > and report as such from reflection? >> > >> >> I need more clearance on this one: are you asking how magic methods are >> reported via Reflection and if that will be changed? >> >> >> > >> > Nikita >> > >> > On Tue, 2 Jun 2020 at 14:26, Dmitry Stogov <dmitrystogov@gmail.com> wrote:
> Does this RFC support __get() returning by reference? > See > https://github.com/php/php-src/blob/master/Zend/tests/overloaded_prop_assign_op_refs.phpt#L11 > > Thanks. Dmitry.
Hello Dmitry, Yes, I'm not changing any behavior of magic methods and references. -- Gabriel Caruso
  110349
June 3, 2020 10:32 nikita.ppv@gmail.com (Nikita Popov)
On Sun, May 31, 2020 at 11:20 PM Gabriel Caruso <carusogabriel34@gmail.com>
wrote:

> On Sun, 31 May 2020 at 15:57, Nikita Popov ppv@gmail.com> wrote: > >> On Fri, May 29, 2020 at 6:45 PM Gabriel Caruso <carusogabriel34@gmail.com> >> wrote: >> >>> Hello, internals! >>> >>> I have opened the voting for >>> https://wiki.php.net/rfc/magic-methods-signature. >>> >>> The voting period ends on 2020-06-19 at 18h (CEST). >>> >> >> The RFC is a bit unclear on what is actually being proposed. It says >> >> > This RFC proposes to add parameter and return types checks per the >> following details. >> >> and goes on to list (reasonable looking) magic method signatures, but >> does not say how exactly those types are going to be checked. Is this going >> to require exactly the same signature, or is this going to be in accordance >> with variance rules? For example, are all of the following signatures valid >> under this RFC? Only the first two? None of them? >> >> // Narrowed return type from ?array >> public function __debugInfo(): array {} >> >> // Narrowed return type from mixed >> public function __get(string $name): int {] >> >> // Widened argument type from string >> public function __get(string|array $name): mixed {} >> > > > They are going to be checked following the variance rules, not the > *exactly* same as the RFC. I'll mention this, thanks for point it out. > > Assuming this, your examples: > > 1 and 2. Will be valid, following the rules introduced by the `mixed` RFC. > > 3. Is that allowed in PHP? If so, the RFC will compliance with that. >
Yes, it is allowed. It makes little sense in this particular case, but it's allowed. Also, is omitting the return type still permitted, even though it would
>> nominally violate variance? >> >> public function __debugInfo() {} >> > > Yes, this hasn't changed. The RFC only affects *typed* methods. >
>> Finally, if omitting the return type is permitted, will an implicit >> return type be added, like we do for __toString()? Would the method >> automatically become >> >> public function __debugInfo(): ?array {} >> > > An implicit return type won't be added for any of the magic methods. I > believe that's a huge BC, and I don't want to debate that for PHP 8 (maybe > PHP 9, yes). >
Why would this be a BC break? To make sure we're on the same page, I'm suggesting to do the same as we do for __toString(), where if you declare public function __toString() {} we automatically convert it into public function __toString(): string {} internally. We could do the same for all other magic methods, and I don't think it would introduce a particularly severe BC break. We did this for __toString() to work with the Stringable interface, and we don't have the same requirement for other magic methods, but I still think it's worth considering this for consistency reasons. Nikita
  110354
June 3, 2020 21:11 carusogabriel34@gmail.com (Gabriel Caruso)
On Wed, 3 Jun 2020 at 12:32, Nikita Popov ppv@gmail.com> wrote:

> On Sun, May 31, 2020 at 11:20 PM Gabriel Caruso <carusogabriel34@gmail.com> > wrote: > >> On Sun, 31 May 2020 at 15:57, Nikita Popov ppv@gmail.com> wrote: >> >>> On Fri, May 29, 2020 at 6:45 PM Gabriel Caruso < >>> carusogabriel34@gmail.com> wrote: >>> >>>> Hello, internals! >>>> >>>> I have opened the voting for >>>> https://wiki.php.net/rfc/magic-methods-signature. >>>> >>>> The voting period ends on 2020-06-19 at 18h (CEST). >>>> >>> >>> The RFC is a bit unclear on what is actually being proposed. It says >>> >>> > This RFC proposes to add parameter and return types checks per the >>> following details. >>> >>> and goes on to list (reasonable looking) magic method signatures, but >>> does not say how exactly those types are going to be checked. Is this going >>> to require exactly the same signature, or is this going to be in accordance >>> with variance rules? For example, are all of the following signatures valid >>> under this RFC? Only the first two? None of them? >>> >>> // Narrowed return type from ?array >>> public function __debugInfo(): array {} >>> >>> // Narrowed return type from mixed >>> public function __get(string $name): int {] >>> >>> // Widened argument type from string >>> public function __get(string|array $name): mixed {} >>> >> >> >> They are going to be checked following the variance rules, not the >> *exactly* same as the RFC. I'll mention this, thanks for point it out. >> >> Assuming this, your examples: >> >> 1 and 2. Will be valid, following the rules introduced by the `mixed` RFC. >> >> 3. Is that allowed in PHP? If so, the RFC will compliance with that. >> > > Yes, it is allowed. It makes little sense in this particular case, but > it's allowed. >
Ok, so let's allow that as well. I'll cover that with tests.
> > Also, is omitting the return type still permitted, even though it would >>> nominally violate variance? >>> >>> public function __debugInfo() {} >>> >> >> Yes, this hasn't changed. The RFC only affects *typed* methods. >> > >>> Finally, if omitting the return type is permitted, will an implicit >>> return type be added, like we do for __toString()? Would the method >>> automatically become >>> >>> public function __debugInfo(): ?array {} >>> >> >> An implicit return type won't be added for any of the magic methods. I >> believe that's a huge BC, and I don't want to debate that for PHP 8 (maybe >> PHP 9, yes). >> > > Why would this be a BC break? To make sure we're on the same page, I'm > suggesting to do the same as we do for __toString(), where if you declare > > public function __toString() {} > > we automatically convert it into > > public function __toString(): string {} > > internally. >
> We could do the same for all other magic methods, and I don't think it > would introduce a particularly severe BC break. > > We did this for __toString() to work with the Stringable interface, and we > don't have the same requirement for other magic methods, but I still think > it's worth considering this for consistency reasons. >
Ok, let me see if I understood it: so if someone creates a public function __set($name, $value) {} we would automatically convert (as per this RFC) to public function(string $name, mixed $value): void {} internally, right? Isn't this a BC if someone is returning something inside that method? Or no, are you talking that we only convert that for Reflection purpose?
> > Nikita >
  110409
June 7, 2020 16:01 carusogabriel34@gmail.com (Gabriel Caruso)
On Wed, 3 Jun 2020 at 23:11, Gabriel Caruso <carusogabriel34@gmail.com>
wrote:

> On Wed, 3 Jun 2020 at 12:32, Nikita Popov ppv@gmail.com> wrote: > >> On Sun, May 31, 2020 at 11:20 PM Gabriel Caruso < >> carusogabriel34@gmail.com> wrote: >> >>> On Sun, 31 May 2020 at 15:57, Nikita Popov ppv@gmail.com> wrote: >>> >>>> On Fri, May 29, 2020 at 6:45 PM Gabriel Caruso < >>>> carusogabriel34@gmail.com> wrote: >>>> >>>>> Hello, internals! >>>>> >>>>> I have opened the voting for >>>>> https://wiki.php.net/rfc/magic-methods-signature. >>>>> >>>>> The voting period ends on 2020-06-19 at 18h (CEST). >>>>> >>>> >>>> The RFC is a bit unclear on what is actually being proposed. It says >>>> >>>> > This RFC proposes to add parameter and return types checks per the >>>> following details. >>>> >>>> and goes on to list (reasonable looking) magic method signatures, but >>>> does not say how exactly those types are going to be checked. Is this going >>>> to require exactly the same signature, or is this going to be in accordance >>>> with variance rules? For example, are all of the following signatures valid >>>> under this RFC? Only the first two? None of them? >>>> >>>> // Narrowed return type from ?array >>>> public function __debugInfo(): array {} >>>> >>>> // Narrowed return type from mixed >>>> public function __get(string $name): int {] >>>> >>>> // Widened argument type from string >>>> public function __get(string|array $name): mixed {} >>>> >>> >>> >>> They are going to be checked following the variance rules, not the >>> *exactly* same as the RFC. I'll mention this, thanks for point it out. >>> >>> Assuming this, your examples: >>> >>> 1 and 2. Will be valid, following the rules introduced by the `mixed` >>> RFC. >>> >>> 3. Is that allowed in PHP? If so, the RFC will compliance with that. >>> >> >> Yes, it is allowed. It makes little sense in this particular case, but >> it's allowed. >> > > Ok, so let's allow that as well. I'll cover that with tests. > > >> >> Also, is omitting the return type still permitted, even though it would >>>> nominally violate variance? >>>> >>>> public function __debugInfo() {} >>>> >>> >>> Yes, this hasn't changed. The RFC only affects *typed* methods. >>> >> >>>> Finally, if omitting the return type is permitted, will an implicit >>>> return type be added, like we do for __toString()? Would the method >>>> automatically become >>>> >>>> public function __debugInfo(): ?array {} >>>> >>> >>> An implicit return type won't be added for any of the magic methods. I >>> believe that's a huge BC, and I don't want to debate that for PHP 8 (maybe >>> PHP 9, yes). >>> >> >> Why would this be a BC break? To make sure we're on the same page, I'm >> suggesting to do the same as we do for __toString(), where if you declare >> >> public function __toString() {} >> >> we automatically convert it into >> >> public function __toString(): string {} >> >> internally. >> > >> We could do the same for all other magic methods, and I don't think it >> would introduce a particularly severe BC break. >> >> We did this for __toString() to work with the Stringable interface, and >> we don't have the same requirement for other magic methods, but I still >> think it's worth considering this for consistency reasons. >> > > Ok, let me see if I understood it: so if someone creates a > > public function __set($name, $value) {} > > we would automatically convert (as per this RFC) to > > public function(string $name, mixed $value): void {} > > internally, right? Isn't this a BC if someone is returning something > inside that method? > > Or no, are you talking that we only convert that for Reflection purpose? > >> >> Nikita >> >
To exemplify my question: is this change that you are suggesting for us to add: https://3v4l.org/JKj9f vs. https://3v4l.org/JKj9f/rfc#git-php-master?
  110620
June 17, 2020 08:33 nikita.ppv@gmail.com (Nikita Popov)
On Wed, Jun 3, 2020 at 11:11 PM Gabriel Caruso <carusogabriel34@gmail.com>
wrote:

> On Wed, 3 Jun 2020 at 12:32, Nikita Popov ppv@gmail.com> wrote: > >> On Sun, May 31, 2020 at 11:20 PM Gabriel Caruso < >> carusogabriel34@gmail.com> wrote: >> >>> On Sun, 31 May 2020 at 15:57, Nikita Popov ppv@gmail.com> wrote: >>> >>>> On Fri, May 29, 2020 at 6:45 PM Gabriel Caruso < >>>> carusogabriel34@gmail.com> wrote: >>>> >>>>> Hello, internals! >>>>> >>>>> I have opened the voting for >>>>> https://wiki.php.net/rfc/magic-methods-signature. >>>>> >>>>> The voting period ends on 2020-06-19 at 18h (CEST). >>>>> >>>> >>>> The RFC is a bit unclear on what is actually being proposed. It says >>>> >>>> > This RFC proposes to add parameter and return types checks per the >>>> following details. >>>> >>>> and goes on to list (reasonable looking) magic method signatures, but >>>> does not say how exactly those types are going to be checked. Is this going >>>> to require exactly the same signature, or is this going to be in accordance >>>> with variance rules? For example, are all of the following signatures valid >>>> under this RFC? Only the first two? None of them? >>>> >>>> // Narrowed return type from ?array >>>> public function __debugInfo(): array {} >>>> >>>> // Narrowed return type from mixed >>>> public function __get(string $name): int {] >>>> >>>> // Widened argument type from string >>>> public function __get(string|array $name): mixed {} >>>> >>> >>> >>> They are going to be checked following the variance rules, not the >>> *exactly* same as the RFC. I'll mention this, thanks for point it out. >>> >>> Assuming this, your examples: >>> >>> 1 and 2. Will be valid, following the rules introduced by the `mixed` >>> RFC. >>> >>> 3. Is that allowed in PHP? If so, the RFC will compliance with that. >>> >> >> Yes, it is allowed. It makes little sense in this particular case, but >> it's allowed. >> > > Ok, so let's allow that as well. I'll cover that with tests. > > >> >> Also, is omitting the return type still permitted, even though it would >>>> nominally violate variance? >>>> >>>> public function __debugInfo() {} >>>> >>> >>> Yes, this hasn't changed. The RFC only affects *typed* methods. >>> >> >>>> Finally, if omitting the return type is permitted, will an implicit >>>> return type be added, like we do for __toString()? Would the method >>>> automatically become >>>> >>>> public function __debugInfo(): ?array {} >>>> >>> >>> An implicit return type won't be added for any of the magic methods. I >>> believe that's a huge BC, and I don't want to debate that for PHP 8 (maybe >>> PHP 9, yes). >>> >> >> Why would this be a BC break? To make sure we're on the same page, I'm >> suggesting to do the same as we do for __toString(), where if you declare >> >> public function __toString() {} >> >> we automatically convert it into >> >> public function __toString(): string {} >> >> internally. >> > >> We could do the same for all other magic methods, and I don't think it >> would introduce a particularly severe BC break. >> >> We did this for __toString() to work with the Stringable interface, and >> we don't have the same requirement for other magic methods, but I still >> think it's worth considering this for consistency reasons. >> > > Ok, let me see if I understood it: so if someone creates a > > public function __set($name, $value) {} > > we would automatically convert (as per this RFC) to > > public function(string $name, mixed $value): void {} > > internally, right? Isn't this a BC if someone is returning something > inside that method? >
Yes, that's right, there would be a BC break. I ran some analysis for the void cases (excluding constructor/destructor) with the following results on top 2k packages: https://gist.github.com/nikic/9bc4f025a85c322a14c21128d8202c64 There were 91 issues found. I have no easy way to analyze the non-void cases. The BC issue is also being discussed in the constructor thread. So yes, just adding the type automatically may not be possible :( Or no, are you talking that we only convert that for Reflection purpose?
>
No, that would make Reflection lie. We don't want Reflection to lie :) Regards, Nikita
  110687
June 19, 2020 16:13 carusogabriel34@gmail.com (Gabriel Caruso)
On Fri, 29 May 2020 at 18:44, Gabriel Caruso <carusogabriel34@gmail.com>
wrote:

> Hello, internals! > > I have opened the voting for > https://wiki.php.net/rfc/magic-methods-signature. > > The voting period ends on 2020-06-19 at 18h (CEST). > > Thanks, > > -- Gabriel Caruso >
The RFC has been accepted with 45 votes in favor of it and 2 against. The Pull Request on GitHub with its implementation will be updated during the next days. Once it's merged into the main branch, I'll update this thread. Thanks everyone for the discussion!