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

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()êÖ >> b1ˆÇ¢ó–ÎàlÜ 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.