Skip to content

Commit

Permalink
#137 - stops dice breaking under some conditions when scalar type hin…
Browse files Browse the repository at this point in the history
…ts are present
  • Loading branch information
TRPB committed Mar 5, 2018
1 parent 1b50d6a commit c3a480e
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 34 deletions.
8 changes: 5 additions & 3 deletions Dice.php
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ private function getClosure($name, array $rule) {
// Generate the method arguments using getParams() and call the returned closure (in php7 will be ()() rather than __invoke)
$params = $this->getParams($class->getMethod($call[0]), ['shareInstances' => isset($rule['shareInstances']) ? $rule['shareInstances'] : [] ])->__invoke($this->expand(isset($call[1]) ? $call[1] : []));
$return = $object->{$call[0]}(...$params);
if (isset($call[2]) && is_callable($call[2])) call_user_func($call[2], $return);
if (isset($call[2]) && is_callable($call[2])) call_user_func($call[2], $return);
}
return $object;
} : $closure;
Expand Down Expand Up @@ -178,7 +178,7 @@ private function getParams(\ReflectionMethod $method, array $rule) {
}
}
// When nothing from $args matches but a class is type hinted, create an instance to use, using a substitution if set
if ($class) try {
if ($class) try {
$parameters[] = $sub ? $this->expand($rule['substitutions'][$class], $share, true) : $this->create($class, [], $share);
}
catch (\InvalidArgumentException $e) {
Expand All @@ -187,7 +187,9 @@ private function getParams(\ReflectionMethod $method, array $rule) {
// For variadic parameters, provide remaining $args
else if ($param->isVariadic()) $parameters = array_merge($parameters, $args);
// There is no type hint, take the next available value from $args (and remove it from $args to stop it being reused)
else if ($args) $parameters[] = $this->expand(array_shift($args));
// Support PHP 7 scalar type hinting, is_a('string', 'foo') doesn't work so this a
// is hacky AF workaround: call_user_func('is_' . $type, '')
else if ($args && (!$param->getType() || call_user_func('is_' . $param->getType()->getName(), $args[0]))) $parameters[] = $this->expand(array_shift($args));

This comment has been minimized.

Copy link
@solleer

solleer Mar 14, 2018

Contributor

@TomBZombie This causes mine to break on php7

This comment has been minimized.

Copy link
@TRPB

TRPB Mar 18, 2018

Author Member

can you elaborate on "break"?

This comment has been minimized.

Copy link
@solleer

solleer Mar 18, 2018

Contributor

It says the method getType doesn't exist

This comment has been minimized.

Copy link
@TRPB

TRPB Mar 19, 2018

Author Member

Strange, http://php.net/manual/en/reflectionparameter.gettype.php says that it's available from PHP7.

I'll grab a php7.0 virtual machine and see if it exists.

If it's not available in php 7.0, I'll move this fix to Dice 3.0 only and tag set minimum PHP version to 7.1 (or 7.2 if that's where getType was added)

This comment has been minimized.

Copy link
@TRPB

TRPB Mar 19, 2018

Author Member

Aha, it appears that getType()->getName() does not work in PHP 7.0, but getType()->__toString() works consistently from 7.0.

This comment has been minimized.

Copy link
@solleer

solleer Mar 19, 2018

Contributor

Switching to that should work

// There's no type hint and nothing left in $args, provide the default value or null
else $parameters[] = $param->isDefaultValueAvailable() ? $param->getDefaultValue() : null;
}
Expand Down
66 changes: 38 additions & 28 deletions tests/BasicTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,17 @@ public function testCreateInvalid() {
throw new ErrorException('Error occurred');
}
}

public function testNoConstructor() {
$a = $this->dice->create('NoConstructor');
$a = $this->dice->create('NoConstructor');
$this->assertInstanceOf('NoConstructor', $a);
}


public function testSetDefaultRule() {
$defaultBehaviour = [];
$defaultBehaviour['shared'] = true;
$this->dice->addRule('*', $defaultBehaviour);
$this->dice->addRule('*', $defaultBehaviour);

$rule = $this->dice->getRule('*');
foreach ($defaultBehaviour as $name => $value) {
Expand All @@ -47,24 +47,24 @@ public function testSetDefaultRule() {
public function testDefaultRuleWorks() {
$defaultBehaviour = [];
$defaultBehaviour['shared'] = true;

$this->dice->addRule('*', $defaultBehaviour);

$rule = $this->dice->getRule('A');

$this->assertTrue($rule['shared']);

$a1 = $this->dice->create('A');
$a2 = $this->dice->create('A');

$this->assertSame($a1, $a2);
}


/*
* Object graph creation cannot be tested with mocks because the constructor need to be tested.
* You can't set 'expects' on the objects which are created making them redundant for that as well
* Need real classes to test with unfortunately.
* Need real classes to test with unfortunately.
*/
public function testObjectGraphCreation() {
$a = $this->dice->create('A');
Expand All @@ -73,15 +73,15 @@ public function testObjectGraphCreation() {
$this->assertInstanceOf('D', $a->b->c->d);
$this->assertInstanceOf('E', $a->b->c->e);
$this->assertInstanceOf('F', $a->b->c->e->f);
}
}

public function testSharedNamed() {
$rule = [];
$rule['shared'] = true;
$rule['instanceOf'] = 'A';

$this->dice->addRule('[A]', $rule);

$a1 = $this->dice->create('[A]');
$a2 = $this->dice->create('[A]');
$this->assertSame($a1, $a2);
Expand All @@ -90,55 +90,55 @@ public function testSharedNamed() {
public function testSharedRule() {
$shared = [];
$shared['shared'] = true;

$this->dice->addRule('MyObj', $shared);

$obj = $this->dice->create('MyObj');
$this->assertInstanceOf('MyObj', $obj);

$obj2 = $this->dice->create('MyObj');
$this->assertInstanceOf('MyObj', $obj2);

$this->assertSame($obj, $obj2);


//This check isn't strictly needed but it's nice to have that safety measure!
$obj->setFoo('bar');
$this->assertEquals($obj->getFoo(), $obj2->getFoo());
$this->assertEquals($obj->getFoo(), 'bar');
$this->assertEquals($obj2->getFoo(), 'bar');
}


public function testInterfaceRule() {
$rule = [];

$rule['shared'] = true;
$this->dice->addRule('interfaceTest', $rule);

$one = $this->dice->create('InterfaceTestClass');
$two = $this->dice->create('InterfaceTestClass');
$this->assertSame($one, $two);
$two = $this->dice->create('InterfaceTestClass');

$this->assertSame($one, $two);
}

public function testCyclicReferences() {
$rule = [];
$rule['shared'] = true;

$this->dice->addRule('CyclicB', $rule);

$a = $this->dice->create('CyclicA');

$this->assertInstanceOf('CyclicB', $a->b);
$this->assertInstanceOf('CyclicA', $a->b->a);

$this->assertSame($a->b, $a->b->a->b);
}

public function testInherit() {
$rule = ['shared' => true, 'inherit' => false];

$this->dice->addRule('ParentClass', $rule);
$obj1 = $this->dice->create('Child');
$obj2 = $this->dice->create('Child');
Expand All @@ -165,4 +165,14 @@ public function testOptionalInterface() {

$this->assertEquals(null, $optionalInterface->obj);
}


public function testScalarTypeHintWithShareInstances() {

$this->dice->addRule('ScalarTypeHint', ['shareInstances' => ['A']]);

$obj = $this->dice->create('ScalarTypeHint');

$this->assertInstanceOf('ScalarTypeHint', $obj);
}
}
13 changes: 10 additions & 3 deletions tests/TestData/Basic.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@ class NoConstructor {

class CyclicA {
public $b;

public function __construct(CyclicB $b) {
$this->b = $b;
}
}

class CyclicB {
public $a;

public function __construct(CyclicA $a) {
$this->a = $a;
}
Expand Down Expand Up @@ -91,7 +91,7 @@ public function getFoo() {
class MethodWithDefaultValue {
public $a;
public $foo;

public function __construct(A $a, $foo = 'bar') {
$this->a = $a;
$this->foo = $foo;
Expand Down Expand Up @@ -126,4 +126,11 @@ class OptionalInterface {
public function __construct(InterfaceTest $obj = null) {
$this->obj = $obj;
}
}


class ScalarTypeHint {
public function __construct(string $a = null) {

}
}

1 comment on commit c3a480e

@ishegg
Copy link

@ishegg ishegg commented on c3a480e Apr 6, 2018

Choose a reason for hiding this comment

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

@TomBZombie this fixes the problem described in issue #95. I'm on version 7.1.6. I can finally migrate to PHP7+ on my production servers. Thanks!

Please sign in to comment.