Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:94389
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmx.de designates 212.227.17.22 as permitted sender)
To: Levi Morrison <levim@php.net>, =?UTF-8?Q?Bj=c3=b6rn_Larsson?=
 <bjorn.x.larsson@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>
 <CAFMT4NrEPA31tL-vuyOHSO6=w+HjzGD6e-WUwyeROP2PKZ82Qw@mail.gmail.com>
Cc: Nikita Popov <nikita.ppv@gmail.com>,
 PHP internals <internals@lists.php.net>
Message-ID: <13e40d6f-c63c-6d0b-db09-7f5805ed018c@gmx.de>
Date: Tue, 5 Jul 2016 17:30:06 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.1.1
MIME-Version: 1.0
In-Reply-To: <CAFMT4NrEPA31tL-vuyOHSO6=w+HjzGD6e-WUwyeROP2PKZ82Qw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] [RFC][Vote] ReflectionType Improvements
From: cmbecker69@gmx.de (Christoph Becker)

On 05.07.2016 at 15:21, Levi Morrison wrote:

> On Mon, Jul 4, 2016 at 10:51 AM, Björn 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 proposed
>>> 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örn 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 have not yet voted, but I tend to be -1 on this RFC.

> 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 between these two:
>     if ($type instanceof ReflectionClassType) {
>         $r = $type->getClass();
>     }
>     // and
>     if (class_exists($type->getName())) {
>         $r = 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 = new ReflectionClass($type->getName());
>     }

ACK.  If this code is regarded as too verbose, a function
class_or_interface_exists() (or something like that) can easily be
defined in userland.  Anyhow, using (class|interface)_exists() offers
the option to use autoloading or not, contrary to the RFC which always
uses autoloading, what might not be desired.

> 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.

Iff we add some other type, we can always reconsider to add another
subclass.  In my opinion, it doesn't make sense to introduce complexity
(albeit minor in this case) to cater to potential future changes.

Another (minor) issue I have with the RFC is the introduction of
ReflectionNamedType.  A simpler alternative appears to be to add the
getName() method to ReflectionType; for unnamed types this could simply
return (string) $reflectionType.  That might not be the most farsighted
design decision, though.

-- 
Christoph M. Becker