# ReflectionType::__toString() prepending \ to class names - Thread: https://externals.io/message/95263 - Original archive: https://news-web.php.net/php.internals/95263 - Started by Aaron Piotrowski on 2016-08-17T16:18:15+00:00 - Messages: 31 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/95263.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #95263 — Aaron Piotrowski — 2016-08-17T16:18:15+00:00 URL: https://externals.io/message/95263#95263 Hi all, I recently made some changes [1] to ReflectionType::__toString() that prepends a leading \ to class names. These changes follow from the discussion on ReflectionType improvements [2, 3] and the discussion on my PR to implement some of the RFC [4]. A \ should be prepended to class names returned from ReflectionType::__toString() so the output of this method can be used when generating code within a namespace. Currently, several libs such as Doctrine manually prepend a \ when generating code. Nullable types will complicate this, since a ? is prepended to the type name, requiring a \ to instead be inserted as the second character. The changes I made will alleviate the need for libs to manipulate the string returned from ReflectionType::__toString() when generating code. This will become more important if more complex types are introduced, such as callable prototypes. If anyone has objections to these changes, please let me know. Thanks! Aaron Piotrowski [1] http://git.php.net/?p=php-src.git;a=commitdiff;h=20fdd47921f423728b409fd0ae0106dab9c34573 [2] http://news.php.net/php.internals/94452 [3] https://wiki.php.net/rfc/reflectiontypeimprovements [4] https://github.com/php/php-src/pull/2068#issuecomment-240071841 --- ## #95264 — Marco Pivetta — 2016-08-17T16:22:45+00:00 URL: https://externals.io/message/95263#95264 · In reply to: #95263 Sorry, I have to object here: this is quite a BC break for Zend\Code, specifically. We will have to re-adjust the code generators to adapt to the newly introduced prepended `\`. In addition to that, there is no need for `\` to be prepended to a type string, since inside string scope, we are always dealing with the base namespace. Seems unnecessary and causes a lot of headaches, instead of actually simplifying things. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/ > Hi all, > > I recently made some changes [1] to ReflectionType::__toString() that > prepends a leading \ to class names. These changes follow from the > discussion on ReflectionType improvements [2, 3] and the discussion on my > PR to implement some of the RFC [4]. > > A \ should be prepended to class names returned from > ReflectionType::__toString() so the output of this method can be used when > generating code within a namespace. Currently, several libs such as > Doctrine manually prepend a \ when generating code. Nullable types will > complicate this, since a ? is prepended to the type name, requiring a \ to > instead be inserted as the second character. The changes I made will > alleviate the need for libs to manipulate the string returned from > ReflectionType::__toString() when generating code. This will become more > important if more complex types are introduced, such as callable prototypes. > > If anyone has objections to these changes, please let me know. > > Thanks! > > Aaron Piotrowski > > [1] http://git.php.net/?p=php-src.git;a=commitdiff;h= > 20fdd47921f423728b409fd0ae0106dab9c34573 > [2] http://news.php.net/php.internals/94452 > [3] https://wiki.php.net/rfc/reflectiontypeimprovements > [4] https://github.com/php/php-src/pull/2068#issuecomment-240071841 --- ## #95265 — Aaron Piotrowski — 2016-08-17T16:44:00+00:00 URL: https://externals.io/message/95263#95265 · In reply to: #95264 Marco, > > > Sorry, I have to object here: this is quite a BC break for Zend\Code, specifically. We will have to re-adjust the code generators to adapt to the newly introduced prepended `\`. > > In addition to that, there is no need for `\` to be prepended to a type string, since inside string scope, we are always dealing with the base namespace. > > Seems unnecessary and causes a lot of headaches, instead of actually simplifying things. > > Marco Pivetta > > http://twitter.com/Ocramius > > http://ocramius.github.com/ > Adjustments will be necessary in Zend\Code no matter what because of nullable types. If a type is nullable, ReflectionType::__toString() will return "?\Type\Name" or without the changes I committed it would return "?Type\Name". If you need the type name without the leading ? or \, use ReflectionNamedType::getName(). It would be nice to have no BC breaks, but right now I'm not seeing a way of handling nullable types in ReflectionType::__toString() without some sort of BC break. Aaron Piotrowski --- ## #95266 — Marco Pivetta — 2016-08-17T16:45:49+00:00 URL: https://externals.io/message/95263#95266 · In reply to: #95265 Since scalar types are invalid anyway if prepended with `\`, I see no point in producing a string with the `\` in it. The current consumers of `Type` assume no `\` is prepended, and we spent an age and a half dealing with `\` being in front of class names in doctrine (and finally got rid of it). This is not being really helpful, as it is. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/ > Marco, > > > […] > ocramius@gmail.com>> wrote: > > […] > specifically. We will have to re-adjust the code generators to adapt to the > newly introduced prepended `\`. > > […] > string, since inside string scope, we are always dealing with the base > namespace. > > […] > simplifying things. > > […] > > > Adjustments will be necessary in Zend\Code no matter what because of > nullable types. If a type is nullable, ReflectionType::__toString() will > return "?\Type\Name" or without the changes I committed it would return > "?Type\Name". > > If you need the type name without the leading ? or \, use > ReflectionNamedType::getName(). > > It would be nice to have no BC breaks, but right now I'm not seeing a way > of handling nullable types in ReflectionType::__toString() without some > sort of BC break. > > Aaron Piotrowski --- ## #95267 — Aaron Piotrowski — 2016-08-17T16:55:20+00:00 URL: https://externals.io/message/95263#95267 · In reply to: #95266 > > > Since scalar types are invalid anyway if prepended with `\`, I see no point > in producing a string with the `\` in it. > > The current consumers of `Type` assume no `\` is prepended, and we spent an > age and a half dealing with `\` being in front of class names in doctrine > (and finally got rid of it). > > This is not being really helpful, as it is. > > Marco Pivetta > > http://twitter.com/Ocramius > > http://ocramius.github.com/ > Scalar types do not have a \ prepended. Only class, interface, and trait names. Can you show me some of the code in Doctrine that handles this? This issue came up because of Doctrine prepending a \ in front of nullable class names [1], resulting in `\?Type`, which of course is invalid. Unfortunately I think no matter what is done, nullable types just created another headache for you. :-( Aaron Piotrowski [1] https://github.com/php/php-src/pull/2068#issuecomment-239983716 (Forgot to CC internals again... ugh) --- ## #95268 — Marco Pivetta — 2016-08-17T17:02:20+00:00 URL: https://externals.io/message/95263#95268 · In reply to: #95267 > > > […] > point > > […] > an > > […] > > Scalar types do not have a \ prepended. Only class, interface, and trait > names. > Aware. > Can you show me some of the code in Doctrine that handles this? This issue > came up because of Doctrine prepending a \ in front of nullable class names > [1], resulting in `\?Type`, which of course is invalid. > This is something to be fixed by introducing support for PHP 7.1 from our (doctrine/zendframework) side, not from PHP's side by changing existing behavior (very very very messy). Doctrine does not yet deal with 7.1, although work started on it, and I'll likely complete it once we get at last RC phases of 7.1: https://github.com/doctrine/common/pull/734/files > Unfortunately I think no matter what is done, nullable types just created > another headache for you. :-( > 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. 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. P.S.: a lot of confusion between direct/mailing-list responses. Sorry if this comes through as a new thread, that's not intentional. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/ --- ## #95269 — Aaron Piotrowski — 2016-08-17T17:17:28+00:00 URL: https://externals.io/message/95263#95269 · In reply to: #95268 > > > 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.) > 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. Aaron Piotrowski --- ## #95270 — Marco Pivetta — 2016-08-17T17:25:24+00:00 URL: https://externals.io/message/95263#95270 · In reply to: #95269 > > > […] > fixed on our end, but please don't try to outsmart it. > > […] > 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. > > […] > 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. > > […] > us what they are. What the code generator does with the definitions is up > to the code generator after that. > > […] > 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/ --- ## #95320 — Marco Pivetta — 2016-08-19T17:16:03+00:00 URL: https://externals.io/message/95263#95320 · In reply to: #95270 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:17 PM, Aaron Piotrowski > wrote: > > > […] > > 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. > > > > […] > > > 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/ --- ## #95322 — Marco Pivetta — 2016-08-19T18:31:53+00:00 URL: https://externals.io/message/95263#95322 · In reply to: #95320 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/ > 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/ > > > > > […] --- ## #95323 — Stanislav Malyshev — 2016-08-19T18:40:21+00:00 URL: https://externals.io/message/95263#95323 · In reply to: #95269 Hi! > 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 see that despite it not being as simple, BC break and clear lack of consensus the change is still not reverted? -- Stas Malyshev smalyshev@gmail.com --- ## #95325 — Marco Pivetta — 2016-08-19T18:42:01+00:00 URL: https://externals.io/message/95263#95325 · In reply to: #95323 On Fri, Aug 19, 2016 at 8:40 PM, Stanislav Malyshev wrote: > Hi! > > > […] > > I see that despite it not being as simple, BC break and clear lack of > consensus the change is still not reverted? I explicitly asked Aaron to wait for me to check out things today. Indeed, it is a mess, but it would probably already have been reverted if we managed to verify the issues earlier on. :-) Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/ --- ## #95326 — Stanislav Malyshev — 2016-08-19T18:52:53+00:00 URL: https://externals.io/message/95263#95326 · In reply to: #95325 Hi! > I explicitly asked Aaron to wait for me to check out things today. > Indeed, it is a mess, but it would probably already have been reverted > if we managed to verify the issues earlier on. :-) OK, please tell when you're ready :) Though checking could be done in a branch too... -- Stas Malyshev smalyshev@gmail.com --- ## #95340 — Damian Wadley — 2016-08-20T16:41:42+00:00 URL: https://externals.io/message/95263#95340 · In reply to: #95326 On Fri, Aug 19, 2016 at 11:52 AM, Stanislav Malyshev wrote: > Hi! > > > […] > > OK, please tell when you're ready :) Though checking could be done in a > branch too... FYI Symfony discovered the change and reported it - https://bugs.php.net/bug.php?id=72906 --- ## #95342 — Aaron Piotrowski — 2016-08-21T14:37:56+00:00 URL: https://externals.io/message/95263#95342 · In reply to: #95322 Hi Marco, > > > 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 --- ## #95343 — Levi Morrison — 2016-08-21T15:07:58+00:00 URL: https://externals.io/message/95263#95343 · In reply to: #95342 > Hi Marco, > > > […] > 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. > > > […] > implements https://wiki.php.net/rfc/reflectiontypeimprovements, even > though the RFC was declined: > > […] > var_dump((new ReflectionMethod("Foo", "bar"))->getReturnType());' > > […] > > 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 > > > -- > > > 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. --- ## #95344 — Levi Morrison — 2016-08-21T15:39:57+00:00 URL: https://externals.io/message/95263#95344 · In reply to: #95343 > > > On Sun, Aug 21, 2016 at 8:37 AM, Aaron Piotrowski > wrote: > > > […] > > 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. --- ## #95345 — Marco Pivetta — 2016-08-21T15:42:30+00:00 URL: https://externals.io/message/95263#95345 · In reply to: #95344 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/ > > > > > > […] > > 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. --- ## #95347 — Levi Morrison — 2016-08-21T15:46:11+00:00 URL: https://externals.io/message/95263#95347 · In reply to: #95345 > 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/ > > > > > […] > > 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. --- ## #95348 — Marco Pivetta — 2016-08-21T15:52:03+00:00 URL: https://externals.io/message/95263#95348 · In reply to: #95347 > > > > > > […] > If you are on 7.1 and you generate code for an earlier version of PHP > that's not really a break... > If you install a library on 7.1 and it stops working for existing 7.0-compliant code (you just want to run the same exact code, but on 7.1), then this is a break. > > This is the ultimate point: if we don't prepend the question mark it will > break code. > This is the impedence here: `ReflectionType` is a *REFLECTION*, not a code generator. Don't think of `ReflectionType#__toString()` as a way to generate code: it is merely a string representation of the reflectoion. Current code generators don't use that type directly anyway: they read it, pass it through a dozen layers that do whatever they need to do, and then it may come out on the other side in whatever shape fits the generator most. The original string representation is not used in the output representation anyway. > If do prepend the question mark it will break code. It's a BC break > whether it is done or not. > It won't break BC for existing code, but codegen for nullable/void needs adaptation. This was already underway on most code generators I work with, and this happens at every PHP version, and it is *OK*. New features => new codegen/changes, this is normal/understood/accepted. > 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. > Take php 7 code, make sure it works on 7.1 as it did before. Take php 7.1 code => requires library adaptation for forward compliance. The confusion here is between BC and FC. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/ --- ## #95349 — Levi Morrison — 2016-08-21T16:00:28+00:00 URL: https://externals.io/message/95263#95349 · In reply to: #95348 > > This is the ultimate point: if we don't prepend the question mark it will > > […] > > This is the impedence here: `ReflectionType` is a *REFLECTION*, not a code > generator. Don't think of `ReflectionType#__toString()` as a way to > generate code: it is merely a string representation of the reflectoion. > This is the last I have to say on anything: You just proved the point. It's a *reflection* of what was written. If it's a nullable type then it needs a nullable reflection in the toString. --- ## #95350 — Marco Pivetta — 2016-08-21T16:01:42+00:00 URL: https://externals.io/message/95263#95350 · In reply to: #95349 Yes, but the __toString API is used for codegen, and current code generators don't expect a `?` to appear there. How many engineers and failing unit tests does it take to explain a BC break? Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/ > This is the ultimate point: if we don't prepend the question mark it will > > […] > > This is the last I have to say on anything: > > You just proved the point. It's a *reflection* of what was written. If > it's a nullable type then it needs a nullable reflection in the toString. --- ## #95351 — Dan Ackroyd — 2016-08-21T18:55:24+00:00 URL: https://externals.io/message/95263#95351 · In reply to: #95350 > Yes, but the __toString API is used for codegen, and current code > generators don't expect a `?` to appear there. And they will continue to not have a `?` when reflecting PHP 7.0 code. It is only when reflecting 7.1 code, that has a different set of syntax, that the library will need to be changed to support a new version of PHP code. This is exactly the same as userland PHP code parsers. They continue to work in new versions of PHP, but only when analysing code from versions they were written to support. They will need to be upgraded to be able to parse syntax that wasn't present in the version of PHP that they were initially written for. > How many ... failing unit tests does it take to explain a BC break? As nullable types are only introduced in PHP 7.1, I strongly suspect that you won't have any unit tests that would work on PHP 7.0 that will start failing on PHP 7.1. So the answer is 'more than zero' ? We don't consider adding new features to be a BC break, as any code analyser or thing that uses reflection will continue to work, when they are given code from a version they were designed to support. btw I object to adding the leading slash. Escaping on output, is almost always the correct thing to do. And escaping intermediate representations of data is almost always the wrong thing. cheers Dan Ack --- ## #95352 — Marco Pivetta — 2016-08-21T19:43:25+00:00 URL: https://externals.io/message/95263#95352 · In reply to: #95351 Hi Dan, > > > […] > > And they will continue to not have a `?` when reflecting PHP 7.0 code. > Sadly, it won't. Here's an example that shows the BC break in a more explicit way: function foo(Iterator $i = null) {} var_dump((string) (new ReflectionParameter('foo', 0))->getType()); This reports `Iterator` for PHP 7.0.x, `?\Iterator` for 7.1.x. https://3v4l.org/tDkLj > It is only when reflecting 7.1 code, that has a different set of > syntax, that the library will need to be changed to support a new > version of PHP code. > Same as above. The problem is on existing code. > This is exactly the same as userland PHP code parsers. They continue > to work in new versions of PHP, but only when analysing code from > versions they were written to support. They will need to be upgraded > to be able to parse syntax that wasn't present in the version of PHP > that they were initially written for. > > > […] > > As nullable types are only introduced in PHP 7.1, I strongly suspect > that you won't have any unit tests that would work on PHP 7.0 that > will start failing on PHP 7.1. > > So the answer is 'more than zero' ? > The answer is "one that at least ran the frikken tests". > We don't consider adding new features to be a BC break, as any code > analyser or thing that uses reflection will continue to work, when > they are given code from a version they were designed to support. > New features: OK. We all agree that `void` and `?Foo` need adaptations in userland libs. I wrote it before, and specifically wrote "New features => new codegen/changes, this is normal/understood/accepted." The issue here is around changes that affect old features (__toString), where the behavior changes depending on whether a parameter is defaulted or not (also a previously existing feature) Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/ --- ## #95353 — Levi Morrison — 2016-08-21T19:48:58+00:00 URL: https://externals.io/message/95263#95353 · In reply to: #95352 > > The issue here is around changes that affect old features (__toString), > where the behavior changes depending on whether a parameter is defaulted or > not (also a previously existing feature) > Which has existed for only one release. This is a small BC break and is intended. We previously had nullable types just no way to really encode it as a type. We now do. Breaking code for one single minor version is perfectly acceptable, especially given that there are other codebases that will break if we do not include the question mark. --- Next page: https://externals.io/message/95263.md?page=2