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

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/ >> >> >