[VOTE] Ensure correct signatures of magic methods

4 years ago by Gabriel Caruso — view source

unread

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

4 years ago by Nikita Popov — view source

unread

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

4 years ago by Gabriel Caruso — view source

unread

> On Fri, May 29, 2020 at 6:45 PM Gabriel Caruso
> 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

4 years ago by Dmitry Stogov — view source

unread

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

4 years ago by Gabriel Caruso — view source

unread

>
> On Mon, Jun 1, 2020 at 12:20 AM Gabriel Caruso
> 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
>> >
>>
>

> 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

4 years ago by Nikita Popov — view source

unread

On Sun, May 31, 2020 at 11:20 PM Gabriel Caruso
wrote:

>
>
>> On Fri, May 29, 2020 at 6:45 PM Gabriel Caruso
>> 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

4 years ago by Gabriel Caruso — view source

unread

> On Sun, May 31, 2020 at 11:20 PM Gabriel Caruso
> 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

4 years ago by Gabriel Caruso — view source

unread

On Wed, 3 Jun 2020 at 23:11, Gabriel Caruso
wrote:

>
>
>> On Sun, May 31, 2020 at 11:20 PM Gabriel Caruso <
>> carusogabriel34@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?

4 years ago by Nikita Popov — view source

unread

On Wed, Jun 3, 2020 at 11:11 PM Gabriel Caruso
wrote:

>
>
>> On Sun, May 31, 2020 at 11:20 PM Gabriel Caruso <
>> carusogabriel34@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

4 years ago by Gabriel Caruso — view source

unread

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!