Everything

php

ReflectionType::__toString() prepending \ to class names

php.internals

Aaron Piotrowski

10 years ago

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

Marco Pivetta

10 years ago

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/

On Wed, Aug 17, 2016 at 6:18 PM, Aaron Piotrowski <aaron@trowski.com> wrote:

Aaron Piotrowski

10 years ago

Marco,

> On Aug 17, 2016, at 11:22 AM, Marco Pivetta <ocramius@gmail.com <mailto:ocramius@gmail.com>> wrote:
>
> 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://twitter.com/Ocramius>
>
> http://ocramius.github.com/ <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

Marco Pivetta

10 years ago

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/

On Wed, Aug 17, 2016 at 6:44 PM, Aaron Piotrowski <aaron@trowski.com> wrote:

Aaron Piotrowski

10 years ago

> On Aug 17, 2016, at 11:45 AM, Marco Pivetta <ocramius@gmail.com> wrote:
>
> 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)

Marco Pivetta

10 years ago

On Wed, Aug 17, 2016 at 6:55 PM, Aaron Piotrowski <aaron@trowski.com> wrote:

>
> > On Aug 17, 2016, at 11:45 AM, Marco Pivetta <ocramius@gmail.com> wrote:
> >
> > 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.
>

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/

Aaron Piotrowski

10 years ago

> 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.)

> 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

Marco Pivetta

10 years ago

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/

Marco Pivetta

10 years ago

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:

Marco Pivetta

10 years ago

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:

Stas Malyshev

10 years ago

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

Marco Pivetta

10 years ago

On Fri, Aug 19, 2016 at 8:40 PM, Stanislav Malyshev <smalyshev@gmail.com>
wrote:

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

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/

Stas Malyshev

10 years ago

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