Introduce PHP7.2 `object` type hint by shulard · Pull Request #112 · zendframework/zend-code

shulard · 2017-07-03T10:24:23Z

Hello,

This PR is about introducing the PHP7.2 object type hint recently merged in the PHP core as requested in #110.

Ocramius · 2017-07-03T10:55:45Z

     */
-    private static $internalPhpTypes = ['void', 'int', 'float', 'string', 'bool', 'array', 'callable', 'iterable'];
+    private static $internalPhpTypes = ['void', 'int', 'float', 'string', 'bool', 'array', 'callable', 'iterable',
+        'object'];


Alignment is broken here

What's the right alignment here ? I try to find a "too" long array property but can't find one in the project...

Is this ok?

private static $internalPhpTypes = [ 'void', 'int', 'float', 'string', 'bool', 'array', 'callable', 'iterable', 'object' ];

Ocramius · 2017-07-03T10:56:31Z

                    || (
                        false === strpos($parameter[2], '?')
-                        && ! in_array(strtolower($parameter[2]), ['void', 'iterable'])
+                        && ! in_array(strtolower($parameter[2]), ['void', 'iterable', 'object'])


Careful about the php version comparison: should be 70200

Ocramius · 2017-07-03T10:57:06Z

        $compatibleParameters = array_filter(
            $parameters,
            function (array $parameter) {
                return PHP_VERSION_ID >= 70100


Same as above: different conditionals for 70200

Ocramius · 2017-07-03T10:58:02Z

            [IterableHintsClass::class, 'iterableParameter', 'foo', 'iterable'],
            [IterableHintsClass::class, 'nullableIterableParameter', 'foo', '?iterable'],
            [IterableHintsClass::class, 'nullDefaultIterableParameter', 'foo', '?iterable'],
+            [ObjectHintsClass::class, 'objectParameter', 'foo', 'object'],


These should only be added to the data providrr if the php version is 70200 or greater

Ocramius · 2017-07-03T10:58:12Z

            [NullableReturnTypeHintedClass::class, 'otherClassReturn', '?\\' . InternalHintsClass::class],
            [IterableHintsClass::class, 'iterableReturnValue', 'iterable'],
            [IterableHintsClass::class, 'nullableIterableReturnValue', '?iterable'],
+            [ObjectHintsClass::class, 'objectReturnValue', 'object'],


These should only be added to the data providrr if the php version is 70200 or greater

I've updated the tests regarding PHP version.

Ocramius · 2017-07-03T11:07:30Z

Yup

…

On 3 Jul 2017 13:04, "Stéphane HULARD" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Generator/TypeGenerator.php <#112 (comment)> : > @@ -33,7 +33,8 @@ * * @link http://php.net/manual/en/functions.arguments.php#functions.arguments.type-declaration */ - private static $internalPhpTypes = ['void', 'int', 'float', 'string', 'bool', 'array', 'callable', 'iterable']; + private static $internalPhpTypes = ['void', 'int', 'float', 'string', 'bool', 'array', 'callable', 'iterable', + 'object']; What's the right alignment here ? I try to find a "too" long array property but can't find one in the project... Is this ok? private static $internalPhpTypes = [ 'void', 'int', 'float', 'string', 'bool', 'array', 'callable', 'iterable', 'object' ]; — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#112 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakOMzTKQhdY0auFDbefLE21Kdylqlks5sKMqsgaJpZM4OMGre> .

Ocramius · 2017-07-03T17:06:23Z

Couple things here:

Patch target should be changed to develop
Bumped PHP to 7.1 and updated PHPUnit to ^6.2.2 #114 was just merged, and that may cause rebase conflicts

Allow the TypeGenerator to recognize this new PHP7.2 internal type. # Conflicts: # src/Generator/TypeGenerator.php

shulard · 2017-07-03T18:00:34Z

Hello,

I've updated the target branch and rebased on the develop branch 😄.

Ocramius

LGTM! Great work, thanks!

michalbundyra · 2017-07-03T18:04:38Z

+                    || (
+                        false === strpos($parameter[2], 'object')
+                    ))
+                    && (PHP_VERSION_ID >= 70100


We don't need this part of condition, as now develop supports only PHP 7.1+

michalbundyra · 2017-07-03T18:04:47Z

+                    || (
+                        false === strpos($parameter[3], 'object')
+                    ))
+                    && (PHP_VERSION_ID >= 70100


michalbundyra · 2017-07-03T18:05:37Z

@Ocramius tests still are failing !

Ocramius · 2017-07-03T18:05:39Z

Hmm, can you check up on the test failures?

shulard · 2017-07-03T18:06:46Z

I've just removed the PHP 7.1 conditions in the tests...

michalbundyra

@shulard Please see my comments.

michalbundyra · 2017-07-03T18:18:28Z

+            [ObjectHintsClass::class, 'nullDefaultObjectParameter', 'foo', '?object'],
        ];

+        $compatibleParameters = array_filter(


You have to use this variable in line 487 and 489.

michalbundyra · 2017-07-03T18:18:38Z

+        $compatibleParameters = array_filter(
+            $parameters,
+            function (array $parameter) {
+


Please remove empty line above.

michalbundyra · 2017-07-03T18:19:32Z

            ['\\ITERABLE'],
+            ['\\object'],
+            ['\\Object'],
+            ['\\OBJECT'],


Here you added these to invalidTypeProvider, don't we need remove 'object' from validTypeProvider?

Introduce a new TestAsset\ObjectHintsClass as a sample to be used in test. Also update generator tests to check for object type.

shulard · 2017-07-03T18:58:38Z

Hello @webimpress,

I've updated the code with your comments and fixed the test suite. I've made a mistake in the "validTypeProvider" method...

michalbundyra

LGTM 👍 And all green now. Thanks @shulard !

Ocramius · 2017-07-03T23:36:37Z

🚢

Ocramius suggested changes Jul 3, 2017

View reviewed changes

shulard force-pushed the feature/object-type-hint branch 2 times, most recently from fb4277e to b95a4e4 Compare July 3, 2017 11:19

Ocramius added the enhancement label Jul 3, 2017

Ocramius added this to the 3.2.0 milestone Jul 3, 2017

shulard changed the base branch from master to develop July 3, 2017 17:56

Add the new object internal type.

ae7c4b6

Allow the TypeGenerator to recognize this new PHP7.2 internal type. # Conflicts: # src/Generator/TypeGenerator.php

shulard force-pushed the feature/object-type-hint branch from b95a4e4 to 754faf1 Compare July 3, 2017 17:59

Ocramius approved these changes Jul 3, 2017

View reviewed changes

michalbundyra reviewed Jul 3, 2017

View reviewed changes

shulard force-pushed the feature/object-type-hint branch from 754faf1 to c7b1922 Compare July 3, 2017 18:06

michalbundyra suggested changes Jul 3, 2017

View reviewed changes

shulard force-pushed the feature/object-type-hint branch from c7b1922 to a8acf4e Compare July 3, 2017 18:56

Update test suite to check object internal type.

b959211

Introduce a new TestAsset\ObjectHintsClass as a sample to be used in test. Also update generator tests to check for object type.

shulard force-pushed the feature/object-type-hint branch from a8acf4e to b959211 Compare July 3, 2017 18:57

michalbundyra approved these changes Jul 3, 2017

View reviewed changes

Ocramius merged commit 9f6eb29 into zendframework:develop Jul 3, 2017

Ocramius self-assigned this Jul 3, 2017

Ocramius mentioned this pull request Jul 23, 2017

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

Closed

Conversation

shulard commented Jul 3, 2017

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 Jul 3, 2017 via email

Uh oh!

Ocramius commented Jul 3, 2017

Uh oh!

shulard commented Jul 3, 2017

Uh oh!

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

michalbundyra commented Jul 3, 2017

Uh oh!

Ocramius commented Jul 3, 2017

Uh oh!

shulard commented Jul 3, 2017

Uh oh!

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

shulard commented Jul 3, 2017

Uh oh!

michalbundyra left a comment

Choose a reason for hiding this comment

Uh oh!

Ocramius commented Jul 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants