Re: ReflectionType::__toString() prepending \ to class names

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 <levim@php.net> wrote: > > > On Sun, Aug 21, 2016 at 9:07 AM, Levi Morrison <levim@php.net> wrote: > >> >> >> On Sun, Aug 21, 2016 at 8:37 AM, Aaron Piotrowski <aaron@trowski.com> >> wrote: >> >>> Hi Marco, >>> >>> > On Aug 19, 2016, at 1:31 PM, Marco Pivetta <ocramius@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. >