ReflectionType::__toString() prepending \ to class names

8 years ago by Aaron Piotrowski — view source

unread

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

8 years ago by Marco Pivetta — view source

unread

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

8 years ago by Aaron Piotrowski — view source

unread

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

8 years ago by Marco Pivetta — view source

unread

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,

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

8 years ago by Aaron Piotrowski — view source

unread

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)

8 years ago by Marco Pivetta — view source

unread

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/

8 years ago by Aaron Piotrowski — view source

unread

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

8 years ago by Marco Pivetta — view source

unread

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/

8 years ago by Marco Pivetta — view source

unread

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 aaron@trowski.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/

8 years ago by Marco Pivetta — view source

unread

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/

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/

8 years ago by Aaron Piotrowski — view source

unread

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

8 years ago by Levi Morrison — view source

unread

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

--

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.

8 years ago by Levi Morrison — view source

unread

On Sun, Aug 21, 2016 at 8:37 AM, Aaron Piotrowski aaron@trowski.com
wrote:

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

--

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.

8 years ago by Marco Pivetta — view source

unread

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 8:37 AM, Aaron Piotrowski aaron@trowski.com
wrote:

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

--

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.

8 years ago by Levi Morrison — view source

unread

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

--

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.

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.

8 years ago by Marco Pivetta — view source

unread

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

--

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.

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/

8 years ago by Levi Morrison — view source

unread

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.

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.

8 years ago by Marco Pivetta — view source

unread

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

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.

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.

8 years ago by Dan Ackroyd — view source

unread

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

8 years ago by Marco Pivetta — view source

unread

Hi Dan,

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.

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.

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

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/

8 years ago by Levi Morrison — view source

unread

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.

8 years ago by Marco Pivetta — view source

unread

I still want to see code that outweights these commonly used open-source
tools before you grab that argument again.

As also said by Dan: new features => adaptation required.

Supporting ? in your own home-grown internal code-generator is something
you can do as part of a 7.1 migration path, exactly like we'll have to do
with the OSS libs.

That said, I'm muting the thread: I'm basically wasting time with this,
apparently.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

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.

8 years ago by Levi Morrison — view source

unread

I still want to see code that outweights these commonly used open-source
tools before you grab that argument again.

As also said by Dan: new features => adaptation required.

Supporting ? in your own home-grown internal code-generator is something
you can do as part of a 7.1 migration path, exactly like we'll have to do
with the OSS libs.

That said, I'm muting the thread: I'm basically wasting time with this,
apparently.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

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.

There is a BC break either way. I do not understand why you cannot seem to
fully understand this. If there is a BC break either way why not stick with
intended behavior even if it breaks code for a few tools for a single
version of PHP?

8 years ago by Dan Ackroyd — view source

unread

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

Thanks.

I missed that part - I thought this was just about ?types, not things
that have default of null.

cheers
Dan

8 years ago by Nikita Popov — view source

unread

On Sun, Aug 21, 2016 at 10:28 PM, Dan Ackroyd danack@basereality.com
wrote:

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

Thanks.

I missed that part - I thought this was just about ?types, not things
that have default of null.

cheers
Dan

--

Hi.

Now that we have made ReflectionType::__toString() be useless and not
represent the type, this method should be deprecated in favor of using
ReflectionNamedType::__toString(), ::allowsNull() and any further
extensions that will be introduced later.

I recommend doing this swiftly so as to ensure that people do not use this
method on PHP 7.1 anymore and thus avoid pain on future upgrades.

Nikita

8 years ago by Dan Ackroyd — view source

unread

Hi.

Now that we have made ReflectionType::__toString() be useless and not
represent the type, this method should be deprecated in favor of using
ReflectionNamedType::__toString(), ::allowsNull() and any further extensions
that will be introduced later.

If we want to go that route, doesn't it make sense to:

Leave ReflectionType::__toString() as it pre-7.1.0beta3 - i.e. not
including the ?nullable marker or the slash.
Explain that ReflectionType::__toString() is a legacy function that
isn't nullable aware and that people should use the other functions to
get the full type information for anything that might contain
functions that have nullable parameters.
Add a ReflectionType::isNullable() function so that can be reflected
properly, exactly as the user wrote them rather than an approximation.

Levi Morrison wrote:

You must attempt to generate exactly what was written in user code

This is exactly why I don't think it's acceptable for the type
information to be including a question mark to indicate nullable.

I use reflection to take a function that has this signature:

function foo(Bar $sc = null) {...}

which is valid PHP 5.x and 7.0 code, and then generate a decorated
function like:

function loggingFoo(Bar $sc = null) {
echo "before \n";
foo($sc);
echo "after \n";
}

If the type for the parameter suddenly starts having a ?nullable in
it, it means I can't run the code generator to produce code that can
be used on 5.x and 7.0 - even though the code being reflected on is
valid 5.x and 7.0 code.

We need to be able to generate exactly what was written in the user
code from Reflection, not just an equivalent form that only works on
7.1+

cheers
Dan
Ack

8 years ago by Levi Morrison — view source

unread

Hi.

Now that we have made ReflectionType::__toString() be useless and not
represent the type, this method should be deprecated in favor of using
ReflectionNamedType::__toString(), ::allowsNull() and any further
extensions
that will be introduced later.

If we want to go that route, doesn't it make sense to:

Leave ReflectionType::__toString() as it pre-7.1.0beta3 - i.e. not
including the ?nullable marker or the slash.

Explain that ReflectionType::__toString() is a legacy function that
isn't nullable aware and that people should use the other functions to
get the full type information for anything that might contain
functions that have nullable parameters.

Add a ReflectionType::isNullable() function so that can be reflected
properly, exactly as the user wrote them rather than an approximation.

Levi Morrison wrote:

You must attempt to generate exactly what was written in user code

This is exactly why I don't think it's acceptable for the type
information to be including a question mark to indicate nullable.

I use reflection to take a function that has this signature:

function foo(Bar $sc = null) {...}

which is valid PHP 5.x and 7.0 code, and then generate a decorated
function like:

function loggingFoo(Bar $sc = null) {
echo "before \n";
foo($sc);
echo "after \n";
}

If the type for the parameter suddenly starts having a ?nullable in
it, it means I can't run the code generator to produce code that can
be used on 5.x and 7.0 - even though the code being reflected on is
valid 5.x and 7.0 code.

We need to be able to generate exactly what was written in the user
code from Reflection, not just an equivalent form that only works on
7.1+

cheers
Dan
Ack

Let's step back for a moment here and look at some data.

We introduced ReflectionType in a major release, a dot zero. Now in the
next minor release (a dot one) we are adjusting its toString representation
because of a new feature that affects the string representation of a single
class of types in Reflection, those with a default of null. There are four
projects affected by this change if it happens and at least one affected if
it does not. If this is changed then four very recent and active projects
are affected on a single minor release and if we do not change it then
at best it is redundant and at least one project is affected.
Furthermore, if generics or callable prototypes or union or intersection
types or theoretically any number of type system changes occurs then the
toString will change, and these four mentioned projects have to be adjusted
anyway.

The choice is very clear to me. We preserve the original intention and
prepend the question mark.

Lastly I acknowledge that my wording earlier was too strict; we don't
generate exactly what the user wrote because of aliases, namespaces etc.
A better descriptor would have been "equivalent".

8 years ago by Stanislav Malyshev — view source

unread

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

8 years ago by Marco Pivetta — view source

unread

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/

8 years ago by Stanislav Malyshev — view source

unread

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

8 years ago by Damian Wadley — view source

unread

On Fri, Aug 19, 2016 at 11:52 AM, Stanislav Malyshev
smalyshev@gmail.com wrote:

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

FYI Symfony discovered the change and reported it -
https://bugs.php.net/bug.php?id=72906