Fix problem with string callables failing in PHP 5.6. #206
Conversation
|
@Xerkus, I received a notification that you had left a comment here, but I no longer see it. Does that mean you were able to reproduce the problem successfully? |
|
Yes, I made wrong test case at first https://3v4l.org/h8MEs |
|
Thanks! |
src/ServiceManager.php
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
test/ServiceManagerTest.php
Outdated
| return new stdClass(); | ||
| } | ||
|
|
||
| public function testStaticCallable() |
There was a problem hiding this comment.
Please provide a more descriptive name, something like testMapsStaticCallableMethodsAsArrayCallablesUnderPHP56. Also, limit this test to PHP 5.6.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Fix problem with string callables failing in PHP 5.6.
|
Thanks, @demiankatz |
ServiceManager v2 supported configuring factories to callable strings of the form
'Class::Method', as a concise alternative to['Class', 'Method'].ServiceManager v3 continues to support this for PHP 7, but for some reason, the feature breaks in PHP 5.6.
This pull request includes a possible fix as well as a test that demonstrates the problem; without my proposed code changes to the ServiceManager, the test dies with a fatal error in PHP 5.6, but passes in PHP 7.
If there is a more elegant solution, I would welcome feedback. If this support is being dropped intentionally, we should at least find a way to fail more gracefully.