PHP 7.1 support by Ocramius · Pull Request #87 · zendframework/zend-code

Ocramius · 2016-08-19T18:13:27Z

This PR introduces initial support for PHP 7.1 nullable (? syntax) return and parameter hints, as well as void return hint.

Implements #84
Implements #85

This last change can probably be reversed, since we don't really use `getType()` besides for testing, while consumers may actually be doing that...~~~ (fixed, BC retained)

TODOs:
- [x] code review (ping @trowski, since you may want to adjust implementation on this)
- [x] code review (ping @nikic, since you had the idea of just dropping `ReflectionType#__toString()`)
- [x] figure out how to make this work both on 7.1 and on 7.0 (currently works only on 7.1)
- [x] figure out why PHP throws `'Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 4755801206603911168 bytes) in /Users/ocramius/Documents/Projects/zend/zend-code/vendor/sebastian/global-state/src/Snapshot.php on line 329'` and `'Process finished with exit code 139 (interrupted by signal 11: SIGSEGV)'` when dealing with `(string)` casts of a `ReflectionType` representing a `?string` type

Note: changes will need to be reflected into:
- https://github.com/doctrine/common/issues/731
- https://github.com/Ocramius/ProxyManager/pull/327
- https://github.com/Ocramius/ProxyManager/issues/314

Ocramius · 2016-08-19T18:25:20Z

@weierophinney can we drop PHP 5 at some point?

Ocramius · 2016-08-19T18:26:15Z

test/Generator/ParameterGeneratorTest.php

+            [NullableHintsClass::class, 'callableParameter', 'foo', '?callable'],
+            [NullableHintsClass::class, 'intParameter', 'foo', '?int'],
+            [NullableHintsClass::class, 'floatParameter', 'foo', '?float'],
+//            [NullableHintsClass::class, 'stringParameter', 'foo', '?string'],


Enabling this makes the test suite fail in the most weird ways (see issue description TODOs) /cc @trowski @nikic

weierophinney · 2016-08-19T18:39:13Z

can we drop PHP 5 at some point?

Eventually, but not yet. 5.6 is still supported. Once that goes off active support, we can do a new minor version that bumps minimum to a v7 release.

Ocramius · 2016-08-19T18:39:57Z

Eventually, but not yet. 5.6 is still supported. Once that goes off active support, we can do a new minor version that bumps minimum to a v7 release.

I'll start ticking off the days.

RalfEggert · 2016-08-19T19:09:29Z

Please don't drop the PHP 5.6 support too early. There are a lot of projects out there which won't be merged to PHP 7.x so quickly.

I would favour a new major release rather than a new minor release. Should be 4.x then to drop PHP 5.6 support. People tend to configure their composer.json files like this:

    "zendframework/zend-code": "^3.0",

and not like this:

    "zendframework/zend-code": "^3.0.0",

Jm2c

Ocramius · 2016-08-19T19:23:13Z

@RalfEggert this targets 4.0

RalfEggert · 2016-08-19T19:57:28Z

Great! then I misunderstood Matthews comment...

djmattyg007 · 2016-08-19T20:33:15Z

What about support for these things in the Scanner classes?

Ocramius · 2016-08-19T22:16:18Z

@djmattyg007 didn't check them at all, sorry.

djmattyg007 · 2016-08-20T01:18:45Z

So is it safe to say at this point that the Scanner classes are no longer maintained?

Ocramius · 2016-08-20T01:33:14Z

No, i simply didn't t look at them, nor will do so in this PR. You can work on them basing your work on the test assets in this PR, if you want.

Ocramius · 2016-09-13T13:57:50Z

test/Generator/ParameterGeneratorTest.php

+            [NullNullableDefaultHintsClass::class, 'callableParameter', 'foo', '?callable'],
+            [NullNullableDefaultHintsClass::class, 'intParameter', 'foo', '?int'],
+            [NullNullableDefaultHintsClass::class, 'floatParameter', 'foo', '?float'],
+//            [NullNullableDefaultHintsClass::class, 'stringParameter', 'foo', '?string'],


Same here - to be tried again with 7.1-beta

nikic · 2016-09-13T14:08:50Z

Are you still seeing the "allowed memory size" issue on the RC1 build?

Ocramius · 2016-09-13T21:18:14Z

I think that this is ready for review/merge.

Works on 5.6, 7.0, 7.1. Dropped ~~5.6~~ 5.5 because I'm a sneaky bastard.

weierophinney · 2016-09-13T21:19:51Z

Dropped 5.6 because I'm a sneaky bastard.

You mean you dropped 5.5. Which is perfectly acceptable for the next minor release. 😄

Ocramius · 2016-09-13T21:20:12Z

Yep, typo, sorry :-)

Ocramius · 2016-09-13T21:23:15Z

Are you still seeing the "allowed memory size" issue on the RC1 build?

@nikic that's gone, thanks! :-)

nikic · 2016-09-13T21:42:38Z

src/Generator/TypeGenerator.php

     *
     * @link http://php.net/manual/en/functions.arguments.php#functions.arguments.type-declaration
     */
-    private static $internalPhpTypes = ['int', 'float', 'string', 'bool', 'array', 'callable'];


What about iterable?

Wait, that passed? :O

Alright, to be added (as separate test asset, since iterable can be a class/interface in 5.6 and 7.0)

Definitely want iterable as part of the final changes. 😄

weierophinney

Looks sane! The primary concerns I have are noted in other review comments, but include:

Should static and/or $this be considered for type hint generation if generating types from docblocks?
License docblocks in test assets.
Adding support for the iterable type.

Thanks, @Ocramius !

weierophinney · 2016-09-15T18:00:25Z

.travis.yml

-        - EXECUTE_CS_CHECK=true
    - php: 5.6
      env:
+        - EXECUTE_CS_CHECK=true


Let's move this into the PHP 7 target, so that we can get results earlier, please. (That's what we've done in other components as well.)

Just verified: can't do, since the lock file was generated with 5.x deps

weierophinney · 2016-09-15T18:04:39Z

src/Generator/MethodGenerator.php

+     *
+     * @return string
+     */
+    private static function expandLiteralType($literalReturnType, ReflectionMethod $methodReflection)


Am I reading this correctly in that it pulls the type from reflecting the docblock in this case? If so, should it also consider static and $this? (If it's strictly from declared return type, ignore.)

If so, should it also consider static and $this? (If it's strictly from declared return type, ignore.)

static is not a valid type hint (for good reasons). static is used when resolving methods to call inside an instance, but it is not possible to use it as a hint due to its natural non-LSP-compliance.

$this is not a type

In general, reflecting from docblocks is now a deprecated feature (there are explicit tests that verify that docblocks aren't used for reflecting anymore)

weierophinney · 2016-09-15T18:05:11Z

src/Generator/ParameterGenerator.php

+     * @param ReflectionParameter $reflectionParameter
+     *
+     * @return string
+     */


Same comment as with MethodGenerator.

Same comment as with MethodGenerator

weierophinney · 2016-09-15T18:09:20Z

test/Generator/ParameterGeneratorTest.php

+            [ClassTypeHintedClass::class, 'classParameter', 'foo', '\\' . ClassTypeHintedClass::class],
+            [ClassTypeHintedClass::class, 'otherClassParameter', 'foo', '\\' . InternalHintsClass::class],
+            [ClassTypeHintedClass::class, 'closureParameter', 'foo', '\\' . \Closure::class],
+            [ClassTypeHintedClass::class, 'importedClosureParameter', 'foo', '\\' . \Closure::class],


Out of curiousity... why are the Null* cases prior to the others, which are all in alpha order by class name otherwise? (I ask, because I thought at first you'd removed the ClassTypeHintedClass cases until I discovered they'd been shifted down...)

I added the assets in the order in which I copied them and adapted them. That's why they don't appear at the end of the list

weierophinney · 2016-09-15T18:11:14Z

test/TestAsset/NullDefaultHintsClass.php

@@ -0,0 +1,38 @@
+<?php
+


EmptyClass includes the license docblock, but this and those following do not; should they? or are they used for comparison purposes?

Docblocks are not important here, and may actually confuse the reader of the assets, so maybe I should remove it from EmptyClass as well.

Ocramius · 2016-09-16T09:49:53Z

@weierophinney implemented iterable support, removed useless docblock on EmptyClass

Ocramius · 2016-09-16T13:36:51Z

@weierophinney this is ready for merge

…parent class hint

…ameter class

…nt class FQCN

… confusing the reader)

…ould be rendered by `TypeGenerator`

…cial" in parameter generation code

…cial" in return type generation code

@weierophinney

review completed - discussed directly on IRC with @weierophinney

Ocramius · 2016-10-24T13:06:32Z

Merging. The failure is caused by a CS check that is fixed in master, and will be fixed here when merging develop into master.

Ocramius added enhancement BC Break labels Aug 19, 2016

Ocramius added this to the 4.0.0 milestone Aug 19, 2016

Ocramius reviewed Aug 19, 2016
View reviewed changes

Ocramius changed the title ~~Feature/php 7 1 support~~ PHP 7.1 support Sep 13, 2016

Ocramius reviewed Sep 13, 2016
View reviewed changes

Ocramius removed the BC Break label Sep 13, 2016

Ocramius modified the milestones: 3.1.0, 4.0.0 Sep 13, 2016

nikic reviewed Sep 13, 2016
View reviewed changes

weierophinney previously requested changes Sep 15, 2016

View reviewed changes

Ocramius force-pushed the feature/php-7-1-support branch from b0617d6 to 46baca8 Compare September 16, 2016 13:15

Ocramius added 21 commits October 24, 2016 15:00

Adding parent nullable return type hint

80efdfc

Expecting parent nullable return hint to be expanded to the actual …

bba27ef

…parent class hint

Expanding also parent type when looking at method return types

6bb8fcd

Nullable parent hint test asset

9e90219

Testing against the parentParameter nullable hint

23eb0f5

Testing against the parentParameter nullable hint on the parent par…

69892eb

…ameter class

Nullable defaulted parent parameter should be expaneded to the pare…

1baa95f

…nt class FQCN

Adding simple parent hint expansion scenarios

8b339a2

Expanding parent parameter hint to the FQCN of the parent class

cf1f743

Minor CS fix - excessive spacing

b12085a

Minor CS fix - excessive line length

1d518df

Removing class docblock (not needed in this test asset, and genuinely…

264bea4

… confusing the reader)

Test asset with the iterable hints (7.1 feature)

8495179

The iterable PHP 7.1 built-in union-type should not be namespaced

5a5a441

Expectations around how the iterable PHP 7.1 built-in union type sh…

7f5c35a

…ould be rendered by `TypeGenerator`

Adding iterable to the internal types recognized by TypeGenerator

b40bbd1

iterable (PHP 7.1 internal union type) should be recognized as "spe…

657e57a

…cial" in parameter generation code

iterable (PHP 7.1 internal union type) should be recognized as "spe…

aa8e947

…cial" in return type generation code

Tests around iterable to be skipped pre-PHP-7.1

bda0512

Adding PHP 7.1 to the build matrix

db63773

xdebug.ini may not exist in all matrix entries

c682384

Ocramius force-pushed the feature/php-7-1-support branch from f23f389 to c682384 Compare October 24, 2016 13:00

Ocramius self-assigned this Oct 24, 2016

Ocramius merged commit 3449c7c into zendframework:develop Oct 24, 2016

Ocramius deleted the feature/php-7-1-support branch October 24, 2016 13:06

mwgamble mentioned this pull request Feb 27, 2017

PHP7 return types incompatibile with generate code magento/magento2#6106

Closed

Ocramius mentioned this pull request Jun 26, 2017

Add support for PHP 7.2's object type hint #110

Closed

michalbundyra mentioned this pull request Aug 29, 2019

Support PHP 7.1 void return type #84

Closed

Conversation

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

Uh oh!

Uh oh!

Ocramius commented Aug 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weierophinney commented Aug 19, 2016

Uh oh!

Ocramius commented Aug 19, 2016

Uh oh!

RalfEggert commented Aug 19, 2016

Uh oh!

Ocramius commented Aug 19, 2016

Uh oh!

RalfEggert commented Aug 19, 2016

Uh oh!

djmattyg007 commented Aug 19, 2016

Uh oh!

Ocramius commented Aug 19, 2016

Uh oh!

djmattyg007 commented Aug 20, 2016

Uh oh!

Ocramius commented Aug 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Sep 13, 2016

Uh oh!

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

Uh oh!

Uh oh!

weierophinney commented Sep 13, 2016

Uh oh!

Ocramius commented Sep 13, 2016

Uh oh!

Ocramius commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ocramius Sep 13, 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!

weierophinney left a comment

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!

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!

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

Uh oh!

Uh oh!

Ocramius commented Sep 16, 2016

Uh oh!

Ocramius commented Oct 24, 2016

Uh oh!

Reviewers

Ocramius commented Aug 19, 2016 •

edited

Loading

Ocramius commented Sep 13, 2016 •

edited

Loading

Ocramius Sep 13, 2016 •

edited

Loading

Ocramius commented Sep 16, 2016 •

edited

Loading