Skip to content

Address deprecation of ReflectionType::getClass() #176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cdosoftei wants to merge 8 commits into from
Closed

Address deprecation of ReflectionType::getClass() #176

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2ed085d
php8: address ReflectionParameter::getClass deprecation
cdosoftei Aug 11, 2020
8a8150d
Adjustments for 7.1, support for union types, test suite updates
cdosoftei Aug 13, 2020
fa620fa
Reinstated previous PHPUnit XML config (dupe of #177)
cdosoftei Aug 18, 2020
0f27f37
Code tweaks and test adjustments
cdosoftei Aug 18, 2020
d9bd281
Test tweaks
cdosoftei Aug 19, 2020
f9e0ab7
_checkType tests cleanup
cdosoftei Aug 20, 2020
c65c2d9
php8: address ReflectionParameter::getClass deprecation
cdosoftei Aug 11, 2020
479be38
Merge branch 'master' of github.com:singlecomm/promise
cdosoftei Aug 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions src/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,11 +342,33 @@ function _checkTypehint(callable $callback, \Throwable $reason): bool
return true;
}

$expectedClass = $parameters[0]->getClass();
$type = $parameters[0]->getType();

if (!$expectedClass) {
if (!$type) {
return true;
}

return $expectedClass->isInstance($reason);
$types = [$type];

if ($type instanceof \ReflectionUnionType) {
$types = $type->getTypes();
}

$mismatched = false;

foreach ($types as $type) {
if (!$type || $type->isBuiltin()) {
continue;
}

$expectedClass = $type->getName();

if ($reason instanceof $expectedClass) {
return true;
}

$mismatched = true;
}

return !$mismatched;
}
4 changes: 2 additions & 2 deletions tests/ErrorCollector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ public function start()
{
$errors = [];

set_error_handler(function ($errno, $errstr, $errfile, $errline, $errcontext) use (&$errors) {
$errors[] = compact('errno', 'errstr', 'errfile', 'errline', 'errcontext');
set_error_handler(function ($errno, $errstr, $errfile, $errline) use (&$errors) {
$errors[] = compact('errno', 'errstr', 'errfile', 'errline');
});

$this->errors = &$errors;
Expand Down
72 changes: 71 additions & 1 deletion tests/FunctionCheckTypehintTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
use Exception;
use InvalidArgumentException;

define('UNION_TYPE_TESTS_ENABLED', defined('PHP_MAJOR_VERSION') && (PHP_MAJOR_VERSION >= 8));
Copy link
Member

@clue clue Aug 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be replaced with @requires PHP 8 annotations for each test à la

promise/tests/PromiseTest.php

Line 135 in 12dcf28

* @requires PHP 7
.

Copy link
Contributor Author

@cdosoftei cdosoftei Aug 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, this is incredibly helpful for writing cleaner tests and better contributions in general!

I do have one more, likely final, follow up:

(1) While the annotation does the job nicely for the test methods, I am not sure of a similar/cleaner approach for the eval piece that defines the class with union type argument methods:

https://github.com/singlecomm/promise/blob/d9bd281fffeb7c1dcc997f177e97dd28bb1b7ca9/tests/FunctionCheckTypehintTest.php#L143-L162

(2) When running the test suite against PHP 7.x, the PHPUnit summary accounts the PHP 8 specific tests as being skipped (albeit it does allow the CI to exit successfully). I believe a @doesNotPerformAssertions annotation (or the prior self::expectNotToPerformAssertions() approach) would be conflicting but I am wondering if there are other options.

https://travis-ci.org/github/reactphp/promise/jobs/719402155

Thank you!

Copy link
Member

@clue clue Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for keeping up with this and the excellent communication!

(1) While the annotation does the job nicely for the test methods, I am not sure of a similar/cleaner approach for the eval piece that defines the class with union type argument methods:

https://github.com/singlecomm/promise/blob/d9bd281fffeb7c1dcc997f177e97dd28bb1b7ca9/tests/FunctionCheckTypehintTest.php#L143-L162

The suggested changes LGTM as-is, but I understand where you're coming from 👍 As an alternative, we should also be able to move this class to tests/fixtures and rely on autoloading to only parse this file for PHP 8 from your specific tests. In this case, I would argue this whole file could use a cleanup and all its fixtures should be moved accordingly. Is this something you can look into?

(2) When running the test suite against PHP 7.x, the PHPUnit summary accounts the PHP 8 specific tests as being skipped (albeit it does allow the CI to exit successfully). I believe a @doesNotPerformAssertions annotation (or the prior self::expectNotToPerformAssertions() approach) would be conflicting but I am wondering if there are other options.

https://travis-ci.org/github/reactphp/promise/jobs/719402155

IMHO skipping tests that are unsupported on certain platforms is the best solution for this situation (clear communication, matches semantically and allows showing more details with --verbose). I agree that the PHPUnit summary (yellow) looks perhaps a bit "unhelpful". In the past we've tried this circumvent this to show a "green" output on your "average PHP installation", but this turned out to be hard to sustain/maintain with multiple PHP environments. Accordingly, we're also having to skip some tests for our other components, e.g. https://travis-ci.org/github/reactphp/event-loop/jobs/709667085, https://travis-ci.org/github/reactphp/socket/jobs/713968611 and others.

Copy link
Contributor Author

@cdosoftei cdosoftei Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementing the fixture classes appropriately renders a much nicer test suite structure indeed!

I believe all validations are satisfied at this point; I've messed up the git/branching strategy on my end -- do you mind squashing the commits at merging time? Thanks!


class FunctionCheckTypehintTest extends TestCase
{
/** @test */
Expand Down Expand Up @@ -42,6 +44,53 @@ public function shouldAcceptStaticClassCallbackWithTypehint()
self::assertFalse(_checkTypehint([TestCallbackWithTypehintClass::class, 'testCallbackStatic'], new Exception()));
}

/** @test */
public function shouldAcceptClosureCallbackWithUnionTypehint()
{
if (UNION_TYPE_TESTS_ENABLED) {
eval(
'namespace React\Promise;' .
'self::assertTrue(_checkTypehint(function (\RuntimeException|\InvalidArgumentException $e) {}, new \InvalidArgumentException()));' .
'self::assertFalse(_checkTypehint(function (\RuntimeException|\InvalidArgumentException $e) {}, new \Exception()));'
);
} else {
self::expectNotToPerformAssertions();
}
}

/** @test */
public function shouldAcceptInvokableObjectCallbackWithUnionTypehint()
{
if (UNION_TYPE_TESTS_ENABLED) {
self::assertTrue(_checkTypehint(new TestCallbackWithUnionTypehintClass(), new InvalidArgumentException()));
self::assertFalse(_checkTypehint(new TestCallbackWithUnionTypehintClass(), new Exception()));
} else {
self::expectNotToPerformAssertions();
}
}

/** @test */
public function shouldAcceptObjectMethodCallbackWithUnionTypehint()
{
if (UNION_TYPE_TESTS_ENABLED) {
self::assertTrue(_checkTypehint([new TestCallbackWithUnionTypehintClass(), 'testCallback'], new InvalidArgumentException()));
self::assertFalse(_checkTypehint([new TestCallbackWithUnionTypehintClass(), 'testCallback'], new Exception()));
} else {
self::expectNotToPerformAssertions();
}
}

/** @test */
public function shouldAcceptStaticClassCallbackWithUnionTypehint()
{
if (UNION_TYPE_TESTS_ENABLED) {
self::assertTrue(_checkTypehint([TestCallbackWithUnionTypehintClass::class, 'testCallbackStatic'], new InvalidArgumentException()));
self::assertFalse(_checkTypehint([TestCallbackWithUnionTypehintClass::class, 'testCallbackStatic'], new Exception()));
} else {
self::expectNotToPerformAssertions();
}
}

/** @test */
public function shouldAcceptClosureCallbackWithoutTypehint()
{
Expand All @@ -52,7 +101,7 @@ public function shouldAcceptClosureCallbackWithoutTypehint()
/** @test */
public function shouldAcceptFunctionStringCallbackWithoutTypehint()
{
self::assertTrue(_checkTypehint(new TestCallbackWithTypehintClass(), new InvalidArgumentException()));
self::assertTrue(_checkTypehint(new TestCallbackWithoutTypehintClass(), new InvalidArgumentException()));
}

/** @test */
Expand Down Expand Up @@ -97,6 +146,27 @@ public static function testCallbackStatic(InvalidArgumentException $e)
}
}

if (UNION_TYPE_TESTS_ENABLED) {
eval(<<<EOT
namespace React\Promise;
class TestCallbackWithUnionTypehintClass
{
public function __invoke(\RuntimeException|\InvalidArgumentException \$e)
{
}

public function testCallback(\RuntimeException|\InvalidArgumentException \$e)
{
}

public static function testCallbackStatic(\RuntimeException|\InvalidArgumentException \$e)
{
}
}
EOT
);
}

class TestCallbackWithoutTypehintClass
{
public function __invoke()
Expand Down