On Sun, Aug 21, 2016 at 9:42 AM, Marco Pivetta <ocram...@gmail.com> wrote:
> Including the question mark still breaks code-gen based on simple nullable
> types. PHP 7.0.x-compliant code will do following:
>
> - reflect function foo(Bar $bar = null) {}
> - cast ReflectionType to string
> - crash (unrecognized `?` symbol)
>
> That's part of what I tested on Friday on just one of the many libraries
> involved.
>
> The new API already includes `ReflectionType#allowsNull()`, so that's
> sufficient to build forward-compliant codegen. Breaking BC is a no-go
> though.
>
> Marco Pivetta
>
> http://twitter.com/Ocramius
>
> http://ocramius.github.com/
>
> On Sun, Aug 21, 2016 at 5:39 PM, Levi Morrison <le...@php.net> wrote:
>
>>
>>
>> On Sun, Aug 21, 2016 at 9:07 AM, Levi Morrison <le...@php.net> wrote:
>>
>>>
>>>
>>> On Sun, Aug 21, 2016 at 8:37 AM, Aaron Piotrowski <aa...@trowski.com>
>>> wrote:
>>>
>>>> Hi Marco,
>>>>
>>>> > On Aug 19, 2016, at 1:31 PM, Marco Pivetta <ocram...@gmail.com>
>>>> wrote:
>>>> >
>>>> > 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)
>>>> >
>>>>
>>>> I've reverted the changes so that `ReflectionType::__toString()` is now
>>>> identical to 7.0, including *not* prepending a ? for nullable types. The
>>>> method is now just an alias of `ReflectionNamedType::getName()`.
>>>>
>>>> `ReflectionType::__toString()` should be discouraged for code
>>>> generation going forward, as it seems there's just not a way to add type
>>>> features in a BC way. My attempt to incorporate nullable types in a way
>>>> that would allow for even more complex types such as `callable(?\Type\Name,
>>>> ?bool)` just caused too many problems.
>>>>
>>>> > 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) {
>>>> > }
>>>>
>>>> Only `ReflectionNamedType` was added so the object returned from
>>>> parameter and return types could have a `getName()` method. The rest of the
>>>> RFC was not implemented. This should be completely BC while allowing future
>>>> types like unions or callables. See some discussion here:
>>>> https://github.com/php/php-src/pull/2068
>>>>
>>>> Aaron Piotrowski
>>>>
>>>>
>>>> --
>>>> PHP Internals - PHP Runtime Development Mailing List
>>>> To unsubscribe, visit: http://www.php.net/unsub.php
>>>>
>>>
>>> This is too big of a revert. You must attempt to generate exactly what
>>> was written in user code which requires the prepended question mark.
>>>
>>
>> To clarify we do resolve certain things such as names which are aliased
>> by use and the like. But we can't resolve all names such as self and parent
>> because of traits. The use of toString is to dump out the type information
>> in a string that would be suitable for regenerating that exact type
>> declaration only. It is not meant to be parsed or analyzed. It was designed
>> this way from the beginning, and any deviation of this is a misuse. By not
>> prepending the ? we will break other libs, so it's a BC break either way.
>> Please put the question mark back as discussed in the beginning. Do not
>> include the leading slash.
>>
>
>
If you are on 7.1 and you generate code for an earlier version of PHP
that's not really a break...
This is the ultimate point: if we don't prepend the question mark it will
break code. If do prepend the question mark it will break code. It's a BC
break whether it is done or not. So what to do? How to pick which is
correct? In my opinion we should preserve the original intention of the
code. Thus we should prepend the question mark.
