Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:94385
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.220.41 as permitted sender)
MIME-Version: 1.0
Sender: morrison.levi@gmail.com
In-Reply-To: <1f39f97a-6aaf-8550-9f82-a7e80465f903@telia.com>
References: <CAFMT4Noe07E9--sasW-Fiw5B1hFFZbXDG2_wumkfZCbqM9wh7A@mail.gmail.com>
 <CAF+90c9RCfLdFGgLVXtc3iPy-LcPK3cfKqHNz-7Mm+QygER1Tg@mail.gmail.com> <1f39f97a-6aaf-8550-9f82-a7e80465f903@telia.com>
Date: Tue, 5 Jul 2016 07:21:28 -0600
Message-ID: <CAFMT4NrEPA31tL-vuyOHSO6=w+HjzGD6e-WUwyeROP2PKZ82Qw@mail.gmail.com>
To: =?UTF-8?Q?Bj=C3=B6rn_Larsson?= <bjorn.x.larsson@telia.com>
Cc: Nikita Popov <nikita.ppv@gmail.com>, PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] [RFC][Vote] ReflectionType Improvements
From: levim@php.net (Levi Morrison)

On Mon, Jul 4, 2016 at 10:51 AM, Bj=C3=B6rn Larsson
<bjorn.x.larsson@telia.com> wrote:
> Den 2016-06-30 kl. 23:57, skrev Nikita Popov:
>>
>> On Thu, Jun 30, 2016 at 6:06 PM, Levi Morrison <levim@php.net> wrote:
>>
>>> The RFC for improving ReflectionType[1] is now in voting phase. The
>>> voting
>>> window is June 30th through July 8th. I have not finished the patch but
>>> I'll have it done before the end of voting.
>>>
>>>    [1]: https://wiki.php.net/rfc/ReflectionTypeImprovements
>>
>>
>> Replying on-list as multiple people asked:
>>
>> I'm voting against this RFC, because it introduced not only the
>> ReflectionNamedType class (which is reasonable), but also the
>> ReflectionClassType class (which is not).
>>
>> My main objection to ReflectionClassType is that it is autoloading
>> dependent (*). Something like getReturnType() on a class type hint will
>> either return a ReflectionClassType if the class can be loaded, or a
>> ReflectionNamedType if it can't. I think this is confusing and I'm sure
>> that this will lead to broken code: For example, people will try to use
>> "$type instanceof ReflectionClassType" to check whether something is a
>> class type hint, while the currently correct way, which works
>> independently
>> of class loading, is to check isBuiltin() instead.
>>
>> I don't think that most consumers of ReflectionType are interested in
>> obtaining a ReflectionClass for the type hint anyway (which is the only
>> functionality that ReflectionClassType provides), and if necessary this
>> can
>> be easily done in userland. There is no need to over-complicate the
>> ReflectionType functionality in this manner, especially with the propose=
d
>> semantics.
>>
>> Nikita
>>
> Maybe one should split the vote into separate for each function.
> I mean pity if vote fails because one function is not attractive while
> the other one is...
>
> What do you think?
>
> Regards //Bj=C3=B6rn Larsson

I think that it's disappointing that I received no feedback on this
RFC at all and yet have this ratio of votes in the negative.

I'll take this opportunity to share a portion of a conversation I had
with Nikita that demonstrates why ReflectionClassType is actually
useful:

> Nikita: But does it really help them? I don't see a lot of differences be=
tween these two:
    if ($type instanceof ReflectionClassType) {
        $r =3D $type->getClass();
    }
    // and
    if (class_exists($type->getName())) {
        $r =3D new ReflectionClass($type->getName());
    }
> Levi: ...[I]t will fail for interfaces.] ReflectionClass can be built for=
 those but class_exists() will be false.
> Nikita: True :)

The (currently) correct code would be:

    if (class_exists($type->getName()) || interface_exists($type->getName()=
)) {
        $r =3D new ReflectionClass($type->getName());
    }

But if we add some other type such as enums we may have to add more
conditions. As long as ReflectionClass can be made from the type then
doing it in the engine is fully forward compatible.

I hope people who voted no will share why they have done so, but I
also hope they'll switch to yes if they voted no because they don't
see it as being useful.