ProxyGenerator: Added support for PHP 7.1 features #734

Majkl578 · 2016-08-16T02:28:31Z

This was supposed to work without any change in the generator, but it's unfortunately not the case. There are multiple problems:

generating nullable type with FQCN is broken - generated code results in parse error;
reflection with nullable FQCN is broken - unable to generate proxy;
it's impossible to distinguish nullable type with null default value and non-nullable type with null default value - may change function signature in the proxy.

Forwarded upstream for more discussion to the PR where the changes were originally discussed and where you can also find more discussion with some other concerns: php/php-src#2068 (comment)
In case of no activity there I'll forward it to the bugs tracker soon or eventually php.internals.

(Travis still has old PHP 7.1 version without the changes, they'll appear in beta3.)

Majkl578 · 2016-08-31T20:00:04Z

Updated for PHP 7.1.0RC1 (probably some refactoring would be good), but the Reflection API is still a mess IMHO. 😟
cc @Ocramius @trowski

Majkl578 · 2016-08-31T22:50:49Z

Added cd7fbb4 to show the inconsistent signature change for parameter without nullable type, but with null as default value. How should we handle this? I consider it a bug since the signature is different, but there is no easy way to support it properly (allowsNull returns true when default value is NULL).

Ocramius · 2016-09-01T06:23:52Z

Note: I stopped working on this unit 7.1 is in RC phase, since the
ReflectionType API is basically useless in its current state. Posted about
it on thr mailing list multiple times, btw.

On 1 Sep 2016 00:50, "Michael Moravec" [email protected] wrote:

Added cd7fbb4
cd7fbb4
to show the inconsistent signature change for parameter without nullable
type, but with null as default value. How should we handle this? I consider
it a bug since the signature is different, but there is no easy way to
support it properly (allowsNull returns true when default value is NULL).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#734 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAJakJ9SCkeHwEuYuyPXnRwq7APGYP2Xks5qlgVKgaJpZM4Jk9-T
.

Ocramius · 2016-09-16T13:44:37Z

Finished working on zendframework/zend-code#87 - should be stable enough to be usable :-)

Ocramius · 2016-09-16T14:06:40Z

Need checking of:

public function foo(int $bar = null) => public function foo(?int $bar = null)
public function foo(iterable $bar)
public function foo($bar) : iterable
public function foo($bar) : void

Majkl578 · 2016-09-21T13:21:04Z

Although 7.1 is RC2 already, Reflection is still a mess. Let's just wait for the resolution of php/php-src#2136 and see what happens. It may probably also fix the nonsense with broken allowsNull. 🙏

Majkl578 · 2016-09-30T14:08:21Z

PR updated, now also includes support for void return type and iterable pseudotype. Tested against PHP 7.1 RC3.

Ocramius · 2016-09-30T14:31:14Z

@Majkl578 amazing! Reviewing now.

Ocramius · 2016-09-30T14:31:37Z

lib/Doctrine/Common/Proxy/ProxyGenerator.php

        }

-        if (method_exists($parameter, 'hasType') && $parameter->hasType() && $parameter->getType()->isBuiltin()) {
-            return (string) $parameter->getType();
+        if (method_exists($parameter, 'hasType')) {


This block is to be moved to a private method

Ocramius · 2016-09-30T14:31:55Z

lib/Doctrine/Common/Proxy/ProxyGenerator.php

@@ -935,25 +939,49 @@ private function buildParametersString(ClassMetadata $class, \ReflectionMethod $
     */
    private function getParameterType(ClassMetadata $class, \ReflectionMethod $method, \ReflectionParameter $parameter)
    {
+        $typeDeclaration = '';


This assignment and subsequent block is to be moved to a private method

Ocramius · 2016-09-30T14:32:20Z

lib/Doctrine/Common/Proxy/ProxyGenerator.php

@@ -1018,20 +1046,43 @@ private function getMethodReturnType(\ReflectionMethod $method)

        $returnType = $method->getReturnType();

+        $returnDeclaration = ': ';


To be moved to a private method (probably same method as block above)

Ocramius · 2016-09-30T14:35:03Z

tests/Doctrine/Tests/Common/Proxy/ProxyGeneratorTest.php

+
+        $classCode = file_get_contents(__DIR__ . '/generated/__CG__DoctrineTestsCommonProxyVoidReturnTypeClass.php');
+
+        $this->assertEquals(1, substr_count($classCode, 'function returnsVoid(): void'));


We should also verify that calling the method produces no return value. Without actually evaluating the code, we can't be sure that we support void correctly. Not sure if we should put it here or in https://github.com/doctrine/common/blob/670d814b6c8b5e8f39a2764d090463d39b3502c9/tests/Doctrine/Tests/Common/Proxy/ProxyLogicTest.php

What exactly do you mean? Since we can't put any return value into the method that is to be proxied (otherwise it's a fatal error), just testing that such method returns NULL, i.e. assertNull($proxy->returnsVoid())?

Something like $proxy->increment(10); and then self::assertSame(10, $proxy->counter). Simply verifies that we're passing parameters.

Majkl578

Updated, refactored most of the code to separate private method and added few more tests.

Majkl578 · 2016-10-03T18:25:22Z

tests/Doctrine/Tests/Common/Proxy/NullableTypeHintsClass.php

+    {
+    }
+
+    public function nullableTypeHintWithDefaultNull(?int $param = null)


There is still no way to detect this correctly at runtime (makes one test fail).

Majkl578 · 2016-10-03T18:26:53Z

tests/Doctrine/Tests/Common/Proxy/ProxyGeneratorTest.php

+        $this->assertEquals(1, substr_count($classCode, 'function nullableTypeHintObject(?\stdClass $param)'));
+        $this->assertEquals(1, substr_count($classCode, 'function nullableTypeHintSelf(?\\' . $className . ' $param)'));
+        $this->assertEquals(1, substr_count($classCode, 'function nullableTypeHintWithDefault(?int $param = 123)'));
+        $this->assertEquals(1, substr_count($classCode, 'function nullableTypeHintWithDefaultNull(?int $param = NULL)'));


This fails in 7.1 because Reflection API still doesn't provide proper way to detect this case (see method declaration above) and I don't see any straightforward workaround that wouldn't break generated code in 7.0 (allowsNull exists there too so method_exists doesn't help).
Any advice how to handle this?

ReflectionParameter::isOptional() and ReflectionParameter::getDefaultValue()

@trowski Thanks, unfortunately that's not enough either, I used isDefaultValueAvailable which should be the same. See https://github.com/doctrine/common/pull/734/files#diff-ce03ab9b396edcbb313c54234c20e0deR1081

The following:

public function foo(int $param = null)

would still generate:

public function foo(?int $param = null)

which is not correct (at least in <7.1). And since we don't want any version checks, it's tricky...

Is it needed for 7.1 to generate 7.0 compatible code? If so, you could special case nullable parameters with a null default value to always omit the preceding ?. It may not be exactly as written, but it will be compatible.

@trowski That's a good question, probably not, but then someone could use 7.1 on CI server to warm-up cache (and generate proxies) and then 7.0 in production...

I already added special case for null default value, but then it has opposite effect.
I actually thought that this code would throw an error due to incompatible signatures:

class Foo { function x(?int $x = NULL) {} } class Bar extends Foo { function x(int $x = NULL) {} }

But surprisingly it doesn't, even though subclass removes nullability (question mark) from the type, something I'd not really expect. This is super-confusing, really something not well-thought-out in my opinion... 😳

Bar::x(int $x = null) is not is not removing nullability from $x, but rather just using older syntax to declare nullability. ?int $x = null is semantically identical to int $x = null, so the definitions are compatible.

…, iterable pseudotype

Majkl578 · 2016-10-05T04:23:02Z

Updated:

When default value is null, question mark is not added to ensure PHP 7.0 compatibility (see inline discussion above).
Added test for void and calling parent with/without parameters.
Hopefully fixed most of the CS issues (no code sniffer here to rescue me).

Ocramius · 2016-10-06T00:45:55Z

tests/Doctrine/Tests/Common/Proxy/ScalarTypeHintsClass.php

@@ -22,4 +22,12 @@ public function combinationOfTypeHintsAndNormal(\stdClass $a, $b, int $c)
    public function typeHintsWithVariadic(int ...$foo)
    {
    }
+
+    public function withDefaultValue(int $foo = 123)


Oooh, shiny! I'll have to test this on my libs too.

Ocramius · 2016-10-06T00:50:44Z

@Majkl578 excellent work! The only blocker for getting this merged is having 7.1.0-RC3 on travis, since there were some changes in the API there.

Ocramius · 2016-10-06T00:52:02Z

Actually, never mind, tested locally, and everything works! Merging, thanks!

…pehints Fix parameter/return type validation for interfaces (introduced in #734)

Majkl578 force-pushed the proxygenerator-php7.1 branch from 6a77931 to 8080153 Compare August 16, 2016 02:37

Majkl578 force-pushed the proxygenerator-php7.1 branch from 8080153 to c58633f Compare August 31, 2016 19:57

Majkl578 mentioned this pull request Sep 2, 2016

'returns void' causing proxy generation errors. doctrine/orm#6000

Closed

Majkl578 force-pushed the proxygenerator-php7.1 branch from cd7fbb4 to 7818108 Compare September 30, 2016 14:02

Majkl578 changed the title ~~Added tests for nullable types (PHP 7.1)~~ ProxyGenerator: Added support for PHP 7.1 features Sep 30, 2016

Enable PHP 7.1 on Travis

e096321

Majkl578 force-pushed the proxygenerator-php7.1 branch from 7818108 to 8e3b4d4 Compare September 30, 2016 14:06

Ocramius suggested changes Sep 30, 2016

View reviewed changes

Majkl578 force-pushed the proxygenerator-php7.1 branch 2 times, most recently from 3032832 to fbf9791 Compare October 3, 2016 18:24

Majkl578 commented Oct 3, 2016

View reviewed changes

Majkl578 force-pushed the proxygenerator-php7.1 branch 2 times, most recently from 4e46fbb to 464507f Compare October 5, 2016 03:58

ProxyGenerator: Support for PHP 7.1: nullable types, void return type…

87dc24e

…, iterable pseudotype

Majkl578 force-pushed the proxygenerator-php7.1 branch from 464507f to 87dc24e Compare October 5, 2016 04:20

Ocramius reviewed Oct 6, 2016

View reviewed changes

Ocramius added this to the 2.7.0 milestone Oct 6, 2016

Ocramius self-assigned this Oct 6, 2016

Ocramius added the Improvement label Oct 6, 2016

Ocramius merged commit 5086700 into doctrine:master Oct 6, 2016

Majkl578 mentioned this pull request Oct 6, 2016

Fix parameter/return type validation for interfaces (introduced in #734) #743

Merged

Ocramius added a commit that referenced this pull request Oct 6, 2016

Merge pull request #743 from Majkl578/proxygenerator-fix-interface-ty…

f74f44d

…pehints Fix parameter/return type validation for interfaces (introduced in #734)

Majkl578 deleted the proxygenerator-php7.1 branch October 6, 2016 03:54

This was referenced Oct 6, 2016

Proxies / PHP7 return types: unexpected '\' (T_NS_SEPARATOR) #736

Closed

Please add support nullable types (PHP 7.1) in ProxyGenerator #731

Closed

Mocking methods with scalar type hinting and optional parameters fails with PHP 7.1 mockery/mockery#623

Closed

Majkl578 mentioned this pull request Nov 29, 2016

PHP7.1 nullable types not supported in doctrine:schema:update #750

Closed

alcaeus mentioned this pull request Dec 7, 2016

[PersistentCollection] Fix compatibility with PHP7 return types doctrine/mongodb-odm#1532

Closed

teohhanhui mentioned this pull request Dec 8, 2018

Tighten CS without BC breaks api-platform/core#2372

Merged

		@@ -1018,20 +1046,43 @@ private function getMethodReturnType(\ReflectionMethod $method)

		$returnType = $method->getReturnType();

		$returnDeclaration = ': ';


		$classCode = file_get_contents(__DIR__ . '/generated/__CG__DoctrineTestsCommonProxyVoidReturnTypeClass.php');

		$this->assertEquals(1, substr_count($classCode, 'function returnsVoid(): void'));

Uh oh!

ProxyGenerator: Added support for PHP 7.1 features #734

ProxyGenerator: Added support for PHP 7.1 features #734

Uh oh!

Conversation

Majkl578 commented Aug 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Majkl578 commented Aug 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Majkl578 commented Aug 31, 2016

Uh oh!

Ocramius commented Sep 1, 2016

Uh oh!

Ocramius commented Sep 16, 2016

Uh oh!

Ocramius commented Sep 16, 2016

Uh oh!

Majkl578 commented Sep 21, 2016

Uh oh!

Majkl578 commented Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ocramius commented Sep 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Majkl578 Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Majkl578 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Majkl578 Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Majkl578 commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ocramius commented Oct 6, 2016

Uh oh!

Ocramius commented Oct 6, 2016

Uh oh!

Uh oh!

Majkl578 commented Aug 16, 2016 •

edited

Loading

Majkl578 commented Aug 31, 2016 •

edited

Loading

Majkl578 commented Sep 30, 2016 •

edited

Loading

Majkl578 Oct 3, 2016 •

edited

Loading

Majkl578 Oct 3, 2016 •

edited

Loading

Majkl578 commented Oct 5, 2016 •

edited

Loading