Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Levi Morrison]

On Mon, Aug 22, 2016 at 2:56 PM, Dan Ackroyd  wrote:

> On 22 August 2016 at 06:33, Nikita Popov  wrote:
> >
> > 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".

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Dan Ackroyd]

On 22 August 2016 at 06:33, Nikita Popov  wrote:
>
> 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

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Nikita Popov]

On Sun, Aug 21, 2016 at 10:28 PM, Dan Ackroyd 
wrote:

> On 21 August 2016 at 20:43, Marco Pivetta  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
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>
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

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Dan Ackroyd]

On 21 August 2016 at 20:43, Marco Pivetta  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

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Levi Morrison]

On Sun, Aug 21, 2016 at 1:52 PM, Marco Pivetta  wrote:

> 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/
>
> On Sun, Aug 21, 2016 at 9:48 PM, Levi Morrison  wrote:
>
>> 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?

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Marco Pivetta]

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/

On Sun, Aug 21, 2016 at 9:48 PM, Levi Morrison  wrote:

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

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Levi Morrison]

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

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Marco Pivetta]

Hi Dan,

On Sun, Aug 21, 2016 at 8:55 PM, Dan Ackroyd  wrote:

> On 21 August 2016 at 17:01, Marco Pivetta  wrote:
> > 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/

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Dan Ackroyd]

On 21 August 2016 at 17:01, Marco Pivetta  wrote:
> 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

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Levi Morrison]

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

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Marco Pivetta]

On Sun, Aug 21, 2016 at 5:46 PM, Levi Morrison  wrote:

>
>
> On Sun, Aug 21, 2016 at 9:42 AM, Marco Pivetta  wrote:
>
>> 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 5:39 PM, Levi Morrison  wrote:
>>
>>>
>>>
>>> On Sun, Aug 21, 2016 at 9:07 AM, Levi Morrison  wrote:
>>>


 On Sun, Aug 21, 2016 at 8:37 AM, Aaron Piotrowski 
 wrote:

> Hi Marco,
>
> > On Aug 19, 2016, at 1:31 PM, Marco Pivetta 
> 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
>
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>

 This is too big of a revert. You must attempt to generate exactly what
 was written in user code which requires the prepended question mark.

>>>
>>> To clarify we do resolve certain things such as names which are aliased
>>> by use and the like. But we can't resolve all names such as self and parent
>>> because of traits. The use of toString is to dump out the type information
>>> in a string that would be suitable for regenerating that exact type
>>> declaration only. It is not meant to be parsed or analyzed. It was designed
>>> this way from the beginning, and any deviation of this is a misuse. By not
>>> prepending the ? we will break other libs, so it's a BC break either way.
>>> Please put the question mark back as discussed in the beginning. Do not
>>> include the leading slash.
>>>
>>
>>
> 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 

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Levi Morrison]

On Sun, Aug 21, 2016 at 9:42 AM, Marco Pivetta  wrote:

> 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 5:39 PM, Levi Morrison  wrote:
>
>>
>>
>> On Sun, Aug 21, 2016 at 9:07 AM, Levi Morrison  wrote:
>>
>>>
>>>
>>> On Sun, Aug 21, 2016 at 8:37 AM, Aaron Piotrowski 
>>> wrote:
>>>
 Hi Marco,

 > On Aug 19, 2016, at 1:31 PM, Marco Pivetta 
 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


 --
 PHP Internals - PHP Runtime Development Mailing List
 To unsubscribe, visit: http://www.php.net/unsub.php

>>>
>>> This is too big of a revert. You must attempt to generate exactly what
>>> was written in user code which requires the prepended question mark.
>>>
>>
>> To clarify we do resolve certain things such as names which are aliased
>> by use and the like. But we can't resolve all names such as self and parent
>> because of traits. The use of toString is to dump out the type information
>> in a string that would be suitable for regenerating that exact type
>> declaration only. It is not meant to be parsed or analyzed. It was designed
>> this way from the beginning, and any deviation of this is a misuse. By not
>> prepending the ? we will break other libs, so it's a BC break either way.
>> Please put the question mark back as discussed in the beginning. Do not
>> include the leading slash.
>>
>
>
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.

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Marco Pivetta]

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 5:39 PM, Levi Morrison  wrote:

>
>
> On Sun, Aug 21, 2016 at 9:07 AM, Levi Morrison  wrote:
>
>>
>>
>> On Sun, Aug 21, 2016 at 8:37 AM, Aaron Piotrowski 
>> wrote:
>>
>>> Hi Marco,
>>>
>>> > On Aug 19, 2016, at 1:31 PM, Marco Pivetta  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
>>>
>>>
>>> --
>>> PHP Internals - PHP Runtime Development Mailing List
>>> To unsubscribe, visit: http://www.php.net/unsub.php
>>>
>>
>> This is too big of a revert. You must attempt to generate exactly what
>> was written in user code which requires the prepended question mark.
>>
>
> To clarify we do resolve certain things such as names which are aliased by
> use and the like. But we can't resolve all names such as self and parent
> because of traits. The use of toString is to dump out the type information
> in a string that would be suitable for regenerating that exact type
> declaration only. It is not meant to be parsed or analyzed. It was designed
> this way from the beginning, and any deviation of this is a misuse. By not
> prepending the ? we will break other libs, so it's a BC break either way.
> Please put the question mark back as discussed in the beginning. Do not
> include the leading slash.
>

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Levi Morrison]

On Sun, Aug 21, 2016 at 9:07 AM, Levi Morrison  wrote:

>
>
> On Sun, Aug 21, 2016 at 8:37 AM, Aaron Piotrowski 
> wrote:
>
>> Hi Marco,
>>
>> > On Aug 19, 2016, at 1:31 PM, Marco Pivetta  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
>>
>>
>> --
>> PHP Internals - PHP Runtime Development Mailing List
>> To unsubscribe, visit: http://www.php.net/unsub.php
>>
>
> This is too big of a revert. You must attempt to generate exactly what was
> written in user code which requires the prepended question mark.
>

To clarify we do resolve certain things such as names which are aliased by
use and the like. But we can't resolve all names such as self and parent
because of traits. The use of toString is to dump out the type information
in a string that would be suitable for regenerating that exact type
declaration only. It is not meant to be parsed or analyzed. It was designed
this way from the beginning, and any deviation of this is a misuse. By not
prepending the ? we will break other libs, so it's a BC break either way.
Please put the question mark back as discussed in the beginning. Do not
include the leading slash.

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Levi Morrison]

On Sun, Aug 21, 2016 at 8:37 AM, Aaron Piotrowski  wrote:

> Hi Marco,
>
> > On Aug 19, 2016, at 1:31 PM, Marco Pivetta  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
>
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>

This is too big of a revert. You must attempt to generate exactly what was
written in user code which requires the prepended question mark.

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Aaron Piotrowski]

Hi Marco,

> On Aug 19, 2016, at 1:31 PM, Marco Pivetta  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


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Damian Wadley]

On Fri, Aug 19, 2016 at 11:52 AM, Stanislav Malyshev
 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

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Stanislav Malyshev]

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
smalys...@gmail.com

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Marco Pivetta]

On Fri, Aug 19, 2016 at 8:40 PM, Stanislav Malyshev 
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/

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Stanislav Malyshev]

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
smalys...@gmail.com

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Marco Pivetta]

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

> Hey Aaron,
>
> I am currently going through the changes, and just figured that 7.1
> implements https://wiki.php.net/rfc/reflectiontypeimprovements, even
> though the RFC was declined:
>
> ./sapi/cli/php -r 'class Foo { public function bar() : ?Foo {} }
> var_dump((new ReflectionMethod("Foo", "bar"))->getReturnType());'
> object(ReflectionNamedType)#2 (0) {
> }
>
> Was there a newer RFC that I missed?
>
> Marco Pivetta
>
> http://twitter.com/Ocramius
>
> http://ocramius.github.com/
>
> On Wed, Aug 17, 2016 at 7:25 PM, Marco Pivetta  wrote:
>
>> On Wed, Aug 17, 2016 at 7:17 PM, Aaron Piotrowski 
>> wrote:
>>
>>>
>>> > On Aug 17, 2016, at 12:02 PM, Marco Pivetta 
>>> 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/
>>
>>
>

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Marco Pivetta]

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

> On Wed, Aug 17, 2016 at 7:17 PM, Aaron Piotrowski 
> wrote:
>
>>
>> > On Aug 17, 2016, at 12:02 PM, Marco Pivetta  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/
>
>

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Marco Pivetta]

On Wed, Aug 17, 2016 at 7:17 PM, Aaron Piotrowski  wrote:

>
> > On Aug 17, 2016, at 12:02 PM, Marco Pivetta  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/

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Aaron Piotrowski]

> On Aug 17, 2016, at 12:02 PM, Marco Pivetta  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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Marco Pivetta]

On Wed, Aug 17, 2016 at 6:55 PM, Aaron Piotrowski  wrote:

>
> > On Aug 17, 2016, at 11:45 AM, Marco Pivetta  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/

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Aaron Piotrowski]

> On Aug 17, 2016, at 11:45 AM, Marco Pivetta  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)
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Marco Pivetta]

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

> Marco,
>
> > On Aug 17, 2016, at 11:22 AM, Marco Pivetta > 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://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
>
>

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Aaron Piotrowski]

Marco,

> On Aug 17, 2016, at 11:22 AM, Marco Pivetta  > 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://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

Re: [PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Marco Pivetta]

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

> 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
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>

[PHP-DEV] ReflectionType::__toString() prepending \ to class names

[Aaron Piotrowski]

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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php