Skip to content
This repository has been archived by the owner on Feb 6, 2020. It is now read-only.

Fix problem with string callables failing in PHP 5.6. #206

Merged
merged 2 commits into from
Nov 27, 2017
+22 −0
Merged

Fix problem with string callables failing in PHP 5.6. #206

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b96db8
Fix problem with string callables failing in PHP 5.6.
demiankatz Nov 3, 2017
8d84c79
Response to review suggestions.
demiankatz Nov 20, 2017
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
4 changes: 4 additions & 0 deletions src/ServiceManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,10 @@ private function getFactory($name)
if ($lazyLoaded) {
$this->factories[$name] = $factory;
}
// PHP 5.6 fails on 'class::method' callables unless we explode them:
if (is_string($factory) && strpos($factory, '::') !== false) {
Copy link
Member

Choose a reason for hiding this comment

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

If this is PHP 5.6-specific, let's add a check for being run under PHP 5.6, so that we can remove this once we drop PHP 5 support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I've made the change using the PHP_MAJOR_VERSION constant; please let me know if you have a different preferred way of doing version checking in the code.

$factory = explode('::', $factory);
}
return $factory;
}

Expand Down
16 changes: 16 additions & 0 deletions test/ServiceManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,4 +267,20 @@ public function testSetAliasShouldWorkWithRecursiveAlias()
$this->assertSame($service, $alias);
$this->assertSame($service, $headAlias);
}

public static function sampleFactory()
{
return new stdClass();
}

public function testStaticCallable()
Copy link
Member

Choose a reason for hiding this comment

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

Please provide a more descriptive name, something like testMapsStaticCallableMethodsAsArrayCallablesUnderPHP56. Also, limit this test to PHP 5.6.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is not actually testing anything PHP 5.6-specific. It is simply testing whether a service manager can accept a callable string of the format class::method to use a static method as a factory. As such, I have given the method a more descriptive name but have not made it a version-specific test; this is a behavior of the service manager that some people depend on, so I think it's worth testing for future regressions in case something different causes this behavior to break. Of course, I'll happily change this if you still feel it is necessary -- but I wrote the test with the intention of exercising a generic behavior, not trying to capture the implementation details of my specific fix.

Copy link
Contributor Author

@demiankatz demiankatz Nov 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It also occurs to me that we could, if you think it worthwhile, add a complementary testFactoryMayBeStaticMethodDescribedByArray test to demonstrate that a configuration using an array instead of a delimited string does exactly the same thing. I'll happily add this if you think it's useful, but I'm holding off for now in the interest of avoiding adding unnecessary noise to the PR.

{
$config = [
'factories' => [
stdClass::class => 'ZendTest\ServiceManager\ServiceManagerTest::sampleFactory',
]
];
$serviceManager = new SimpleServiceManager($config);
$this->assertEquals(stdClass::class, get_class($serviceManager->get(stdClass::class)));
}
}