Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:95322
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.42 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CADyq6s+_fS--RaO9zm4rxnLJt1EsCAY4evhn3SjAnyC0cT=-UQ@mail.gmail.com>
References: <0668D584-EE51-4932-85D7-01D8BF70E409@trowski.com>
 <CADyq6s+qQEwXSqZUB6cs=zabfp28is57ZGK4nKb4TxivvhiZ8Q@mail.gmail.com>
 <15E198DD-FF66-475D-ABBC-54ECD2B6BF62@trowski.com> <CADyq6s+PEq9dtbKvv=7dWH6maePTwKTj9ki9W_25w4waC+orMw@mail.gmail.com>
 <D3DD20C1-9EF4-44EB-83E7-2DCB115C8057@trowski.com> <CADyq6s+sCY3UGACKXr-bJi8z_YNOOZwdMJuzDgA5M3VvJkyqHg@mail.gmail.com>
 <E4EE6269-8D50-4529-9A21-10B40D4D3B4A@trowski.com> <CADyq6sK22PQaji6pa727k6VS-pK6HzQhM5ccBORhC303_O7JgA@mail.gmail.com>
 <CADyq6s+_fS--RaO9zm4rxnLJt1EsCAY4evhn3SjAnyC0cT=-UQ@mail.gmail.com>
Date: Fri, 19 Aug 2016 20:31:53 +0200
Message-ID: <CADyq6sJve5XyRmubtyY6MeSsJYLFn2Y=W5a1kYy_7rAhH-Kn7g@mail.gmail.com>
To: Aaron Piotrowski <aaron@trowski.com>
Cc: internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a114b3ab8dd63cd053a70ea05
Subject: Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names
From: ocramius@gmail.com (Marco Pivetta)

--001a114b3ab8dd63cd053a70ea05
Content-Type: text/plain; charset=UTF-8

Hi Aaron et all,

I tried to implement support for 7.1 in zend-code as a start:

https://github.com/zendframework/zend-code/pull/87

A few issues arise:

 * `ReflectionType#__toString()` is too volatile, especially if we want to
support multiple versions of PHP, therefore it's a good idea to not think
too much about it, and instead deprecate it. Most issues I had while
working with the feature were related with string formatting, and that's
simply gotta die: just using a more specific API should cut it (getName,
getClass, isNullable, etc. As few strings as possible, please!).
 * A page where we can see the current state of the `ReflectionType` API
(and its subtypes) would be golden.
 * `ReflectionType#__toString()` seems to crash in very interesting ways
when `?string` is reflected (see issue above - couldn't isolate precisely)

Cheers,


Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On Fri, Aug 19, 2016 at 7:16 PM, Marco Pivetta <ocramius@gmail.com> wrote:

> Hey Aaron,
>
> I am currently going through the changes, and just figured that 7.1
> implements https://wiki.php.net/rfc/reflectiontypeimprovements, even
> though the RFC was declined:
>
> ./sapi/cli/php -r 'class Foo { public function bar() : ?Foo {} }
> var_dump((new ReflectionMethod("Foo", "bar"))->getReturnType());'
> object(ReflectionNamedType)#2 (0) {
> }
>
> Was there a newer RFC that I missed?
>
> Marco Pivetta
>
> http://twitter.com/Ocramius
>
> http://ocramius.github.com/
>
> On Wed, Aug 17, 2016 at 7:25 PM, Marco Pivetta <ocramius@gmail.com> wrote:
>
>> On Wed, Aug 17, 2016 at 7:17 PM, Aaron Piotrowski <aaron@trowski.com>
>> wrote:
>>
>>>
>>> > On Aug 17, 2016, at 12:02 PM, Marco Pivetta <ocramius@gmail.com>
>>> wrote:
>>> >
>>> > That would have been a headache anyway. We saw it coming, and it will
>>> be fixed on our end, but please don't try to outsmart it.
>>> > I know that there is good intention on your side, but this is really
>>> going to just make it an issue.
>>>
>>> Looks like this problem is more complicated than I thought. I thought
>>> prepending the \ would mean little work on your end, but it appears I was
>>> wrong.
>>>
>>> I'm still confused as to what's going on and what the best solution
>>> is... Currently Doctrine is manually prepending \ to class names. Obviously
>>> your logic would have to change between 7.0 and 7.1, but then going forward
>>> you could rely on ReflectionType::__toString() to return a syntax-valid
>>> type name, rather than modifying it. Or perhaps rather than relying on
>>> casting to a string and examining the string, Doctrine should be using
>>> ReflectionNamedType::getName() and ReflectionType::allowsNull() for 7.1 and
>>> beyond. (Just a suggestion, I'd have to dig into the code to really
>>> understand what's going on, and I don't have a ton of time to do so at the
>>> moment.)
>>>
>>
>> The problem is that we're not talking about 1 library, but a few (and I'm
>> only talking about the ones I know of).
>> Changing behavior is going to cause issues.
>>
>>
>>> > From the codegen-side (I do write a lot of code generators), having
>>> `\` prepended in front of stuff makes things just more complex to deal
>>> with, since I have to strip it and re-introduce it anyway in multiple
>>> locations in the code, while it should just be attached in the final
>>> output-logic bit.
>>> > Instead, please keep the reflector on-spot: reflecting things, telling
>>> us what they are. What the code generator does with the definitions is up
>>> to the code generator after that.
>>> >
>>> > We have to adjust the code for `void` and `?` anyway, so this is just
>>> more changes to keep track of, and it would break existing code.
>>>
>>> It sounds like you'd prefer the ? was not prepended to the string as
>>> well, is that correct? Again it sounds like it would be better to use
>>> methods other than __toString(). I understand __toString() was the only way
>>> to get the type name before, but now that this has been fixed perhaps it
>>> should be avoided in your use-cases.
>>
>>
>> I think that adding the `?` would be semantically correct, from a
>> reflector perspective (remember, we are only reflecting: please completely
>> ignore the idea of using this for codegen, it is a separate domain).
>>
>> I can't tell you for sure right now, but I will check on Friday.
>>
>> Libraries that directly affect me personally are doctrine/common,
>> zendframework/zend-code and ocramius/proxy-manager, so I am only talking
>> about these 3 for now.
>> If I remember correctly, in all three a `(string)` cast is being used for
>> discovering scalar types, although I am not sure.
>>
>> Can you please poke me at EOD on Friday, so maybe we look at this
>> together?
>>
>> Cheers,
>>
>> Marco Pivetta
>>
>> http://twitter.com/Ocramius
>>
>> http://ocramius.github.com/
>>
>>
>

--001a114b3ab8dd63cd053a70ea05--