Added ServiceManager v3 and EventManager v3 support #37
Conversation
| * | ||
| * @return CorsService | ||
| */ | ||
| public function createService(ServiceLocatorInterface $container) |
There was a problem hiding this comment.
Remove Factorinterface from the class and just use __invoke instead.
There was a problem hiding this comment.
Sm 2.7 provides compatibility with __invoke?
There was a problem hiding this comment.
Remove Factorinterface from the class and just use __invoke instead.
@macnibblet this would be be BC break.
Sm 2.7 provides compatibility with __invoke?
@bakura10 No. At least I don't know this and this is not mentioned in migration guide, also tested with @macnibblet suggestion as I thought does not work.
There was a problem hiding this comment.
Using __invoke makes this class callable, same as a closure or anonymous function so yes it will work.
There was a problem hiding this comment.
@macnibblet maybe I don't get what you want because I already added __invoke to support zend-servicemanager v3, but the second part where you suggesting remove Factorinterface and createService this is BC break.
I don't understand what changes you want I think I did everything mentioned here: http://zendframework.github.io/zend-servicemanager/migration/#factoryinterface
There was a problem hiding this comment.
Using __invoke makes this class callable, same as a closure or anonymous function so yes it will work.
@macnibblet as I said I tested your suggestion by removing FactoryInterface and createService() and tests failed. So if you think that I should change something please explain more about what and why should be changed.
There was a problem hiding this comment.
@svycka Can you point to the test failures? I could look at them to give guidance.
What @macnibblet is suggesting is the route we went with factories in Apigility itself: they do not implement FactoryInterface, but instead are simply callable; additionally, we don't typehint the container. This provides forwards compatibility out-of-the-box. As such, I'd like to see the test failures to understand what exactly is failing; is it tests of the factories themselves? or some integration test?
(Side note: The only place we'll likely have issues in Apigility is where we define abstract factories, and those will need to be updated.)
There was a problem hiding this comment.
@weierophinney I don't know apigility and didn't used it but..
What @macnibblet is suggesting is the route we went with factories in Apigility itself: they do not implement FactoryInterface, but instead are simply callable; additionally, we don't typehint the container. This provides forwards compatibility out-of-the-box.
not related, but you forgot about plugin managers and I guess factories implementing MutableCreationOptionsInterface they also will not work out-of-the-box.
I did those changes the way documented in migration documentation but I guess some modules does things differently. :D
|
👍 |
|
Looks pretty good. @weierophinney can you do a review as Apigility depends on this as well? |
|
@bakura10 is there anything I could do to speed up this? I can even change what @macnibblet said but removing public methods would mean BC break and failing tests, but yes that probably would work and I could fix tests if that would speed up new release. |
|
I'm currently abroad until 16th of April so won't be able to do much. However I think current code works well and is compatible with all SM versions. However as said I'd love to have @weierophinney a last shot as this is the recommended library for CORS in Apigility. If he does not give any feedback I'll merge and tag when I come back from holidays. Sorry :). |
| @@ -39,4 +39,13 @@ public function testCanCreateOptions() | |||
|
|
|||
| $this->assertInstanceOf('ZfrCors\Options\CorsOptions', $options); | |||
| } | |||
|
|
|||
| public function testCanCreateOptionsWithServiceManagerV2() | |||
There was a problem hiding this comment.
If this and the other "v2" tests are what are failing, my point above is that they're unnecessary if you're using invokables without implementing FactoryInterface.
There was a problem hiding this comment.
@weierophinney updated factories like this:
class CorsOptionsFactory
{
public function __invoke(ContainerInterface $container, $requestedName, array $options = null)
{
//...
}
}
and run tests after composer update --prefer-lowest
$ php vendor/bin/phpunit
PHPUnit v1.2.1-2-g1176b18 by Sebastian Bergmann and contributors.
Runtime: PHP 5.6.15-1+deb.sury.org~trusty+1 with Xdebug 2.3.3
Configuration: /vagrant/projects/zfr-cors/phpunit.xml.dist
EEE..............................
Time: 23.92 seconds, Memory: 10.50Mb
There were 3 errors:
1) ZfrCorsTest\Factory\CorsOptionsFactoryTest::testCanCreateOptions
Zend\ServiceManager\Exception\ServiceNotCreatedException: An exception was raised while creating "ZfrCors\Options\CorsOptions"; no instance returned
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:943
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:1096
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:636
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:599
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:532
/vagrant/projects/zfr-cors/tests/ZfrCorsTest/Factory/CorsOptionsFactoryTest.php:38
Caused by
PHPUnit_Framework_Error: Argument 3 passed to ZfrCors\Factory\CorsOptionsFactory::__invoke() must be of the type array, string given
/vagrant/projects/zfr-cors/src/ZfrCors/Factory/CorsOptionsFactory.php:39
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:936
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:1096
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:636
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:599
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:532
/vagrant/projects/zfr-cors/tests/ZfrCorsTest/Factory/CorsOptionsFactoryTest.php:38
2) ZfrCorsTest\Factory\CorsRequestListenerFactoryTest::testCanCreateCorsRequestListener
Zend\ServiceManager\Exception\ServiceNotCreatedException: An exception was raised while creating "ZfrCors\Mvc\CorsRequestListener"; no instance returned
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:943
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:1096
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:636
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:599
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:532
/vagrant/projects/zfr-cors/tests/ZfrCorsTest/Factory/CorsRequestListenerFactoryTest.php:38
Caused by
PHPUnit_Framework_Error: Argument 3 passed to ZfrCors\Factory\CorsRequestListenerFactory::__invoke() must be of the type array, string given
/vagrant/projects/zfr-cors/src/ZfrCors/Factory/CorsRequestListenerFactory.php:40
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:936
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:1096
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:636
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:599
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:532
/vagrant/projects/zfr-cors/tests/ZfrCorsTest/Factory/CorsRequestListenerFactoryTest.php:38
3) ZfrCorsTest\Factory\CorsServiceFactoryTest::testCanCreateCorsService
Zend\ServiceManager\Exception\ServiceNotCreatedException: An exception was raised while creating "ZfrCors\Service\CorsService"; no instance returned
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:943
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:1096
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:636
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:599
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:532
/vagrant/projects/zfr-cors/tests/ZfrCorsTest/Factory/CorsServiceFactoryTest.php:38
Caused by
PHPUnit_Framework_Error: Argument 3 passed to ZfrCors\Factory\CorsServiceFactory::__invoke() must be of the type array, string given
/vagrant/projects/zfr-cors/src/ZfrCors/Factory/CorsServiceFactory.php:39
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:936
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:1096
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:636
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:599
/vagrant/projects/zfr-cors/vendor/zendframework/zend-servicemanager/src/ServiceManager.php:532
/vagrant/projects/zfr-cors/tests/ZfrCorsTest/Factory/CorsServiceFactoryTest.php:38
FAILURES!
Tests: 33, Assertions: 59, Errors: 3.
so problem in V2 is here:
$instance = call_user_func($callable, $this, $cName, $rName);
third argument is string while V3 FactoryInterface expects array of options. So the simple fix would be ignoring this and removing array from __invoke signature like this:
public function __invoke(ContainerInterface $container, $requestedName, $options = null)
then tests pass. I don't know if this is ok I can change it like this.
|
@weierophinney I did not removed container typehint because in composer we require zend-servicemanager:^2.7.5 with supports this so I don't see why remove it. If you like I can remove this also. |
|
also if we are not going implement interfaces then maybe change signature like this: |
| @@ -29,16 +28,17 @@ | |||
| * @license MIT | |||
| * @author Florent Blaison <[email protected]> | |||
| */ | |||
| class CorsServiceFactory implements FactoryInterface | |||
| class CorsServiceFactory | |||
| { | |||
| /** | |||
| * {@inheritDoc} | |||
There was a problem hiding this comment.
@weierophinney I guess this is not good anymore, but what I should add here if typehint to ContainerInterface will be removed? ServiceLocatorInterface|ContainerInterface?
|
Hows this PR going? Looking at the latest code, the factories seem to be wrong if it was to service both zf2 and zf3 ServiceManager's?? Replacing
With
would break v2 implementation. It should be an addition to the existing implementation. Should it not be like this |
|
@scottasmith you are wrong service manager v2 also supports this syntax except $options parameter, but it is optional so it works for both SMv2 and SMv3 just not sure about defining them because we don't use and haven't added interface. |
|
@scottasmith I think that you're correct, actually. A straight replacement of I think that they're doing something different with these factories, though. They're not following the "recommended" migration guide (https://zendframework.github.io/zend-servicemanager/migration/#factories). They're dropping at least one type hint and removing a reference to @weierophinney usually has a sufficiently detailed/understandable explanation for decisions, but this is one of the few times I'm not seeing much detail or understanding exactly what he's saying here. Granted, he's not said much and I only looked through here quickly. Personally, I'd like to get a better understanding for why the migration guides recommendations weren't followed. It may be that "it works", but it seems odd to have interfaces that are not being used in their traditional/expected manner. P.S. Please...I hope my tone doesn't sound mean/accusatory. In this world of PC-gone-mad and hyper-sensitivity, I feel compelled to say that I'm simply confused and believe the current solution to be less than it otherwise could be. I'm often wrong or slow to understand, so I'm writing this to voice my humble opinion and to ask for some help understanding. If I'm right, great. If I'm not, that's fine, too. No worries...and no offense(s) intended. :-) |
|
@scottasmith and @telkins your suggestion was my original commit, but I cahnged that because I was told:
|
|
@svycka Thanks for pointing out those comments. Looks like I missed them earlier because they were in one of github's nearly-hidden outdated comments lines. I've found/expanded them and found what you quoted. I'll read through them later, though, since the day's wearing on and I haven't yet eaten breakfast. ;-) |
|
@svycka Fair enough, I've not seen the latest implementation until now. As far as the $requestedName and $options are concerned. SM v2 would push a string through anyway and not an array for $options and it would make it more confusing to the community on what the factory is trying to do or being passed. |
|
@svycka OK. I have just read through those comments a bit more thoroughly. I can see that the migration guide was followed initially, which makes good sense. While I have a better understanding of why @weierophinney suggested that the migration guide should not be followed in this case, I'm a bit uncomfortable with it. Here's a comment from @weierophinney that seems to get to the crux of it:
So, as I said, I understand the main argument, but struggle with the fact that it seems that changes to zfr-cors are being made based on how Apigility is designed to work. This seems backward to me. I'm an Apigility user and want zfr-cors to be read/up-to-date ASAP, but it seems like making changes in this module for the needs of a single consumer isn't the best way to go. Then, again...I just re-read the rest of the same post from @weierophinney:
So...it's possible that @weierophinney was just explaining what @macnibblet was thinking and seems to allude to the fact that it is Apigility that should be changed/updated. This is more along the lines of what I would expect, to be honest....and I hope that's the case. As usual, I know it's possible that I'm missing something here. I don't want to be a stick in the mud, but if @weierophinney or someone could help me to see why zfr-cors is being designed to Apigility instead of the other way around, I'd appreciate it. Thanks in advance. :-) |
|
I would rather see how the factory is expected to work by simply reading the class code rather than seeing a class that does something on A developer should not have to delve deep into the ServiceManager code to see how things are implemented in the factory. Thats what the interfaces are good for (As well as catching developer mistakes later on). Does it really matter that all the factory is designed like the official migration guide if it works in both SM v2 and v3? |
|
@scottasmith I take a different approach. I like that an interface provides the information that is needed for implementation. This makes it less necessary to read the class code. The choice of Of course, I agree with what you wrote here, so perhaps I've misunderstood something:
You then seem to make a contradictory question/statement:
The SM v2 and v3 FactoryInterface interfaces do seem clean and understandable (to me) and are typehinted (except for v3's |
|
@telkins Sorry we seem to have crossed wires. Maybe because I didn't write the last comment clearly ;) I indeed am the same as you and like having a small interface tell me how i should code a factory/anything. What I meant to say is why should you not code as to guidelines on the migration page. I am not against typehint. I am the complete opposite and love the new implementation over the By clean understandable code, I mean that I would much rather glance at a class and instantly see what exactly its supposed to do by looking at its interfaces and typehints. The interfaces are clean and very understandable. In contrast, this does not tell me much about what it is supposed to do: Yes it creates a new instance of a class. But is this for service manager or some ad-hock implementation? Without reading the ServiceManager code, who knows. |
|
@scottasmith I think you're right and we crossed wires. It does seem that we're more or less on the same page. :-) I agree that we should "code as to guidelines on the migration page", which appears to be what @svycka did the first time around. I'm sure it's frustrating for him to be doing this work and trying to follow the suggestions @macnibblet and @weierophinney have made and then have people like me come in and question his actions/choice. Sorry, @svycka...! No harm intended. :-) I should also note that I'm very wary of contradicting something @weierophinney says. I have a great deal of respect for his knowledge, experience, and way of thinking. In the end, though, I'm not so sure that he's recommending that the "guidelines on the migration page" be ignored simply to suit some need in Apigility or if he's simply explaining @macnibblet's reasoning. I hope it's the latter, anyway, and that he'll jump in here at some point and clarify his position. Thanks for the clarification, btw. :-) |
|
ping @bakura10 |
|
Oops sorry @svycka I should have read the comments before commenting :p. It seems that v2 handle the invoke here: https://github.com/zendframework/zend-servicemanager/blob/release-2.7/src/ServiceManager.php#L922 |
|
In all cases, I'll likely tag this as 2.0.0. Would it be ok for everyone guys? But if we go that way, what about simply dropping support for v2 components? @weierophinney will Apigility 2 supports v2 components of ZF ? |
|
@bakura10 why you want to tag this as 2.0 this does not have BC break? |
|
Hmmm yeah you're correct. I'm just a bit afraid as this package is used quite a lot. Did you have the opportunity to try it successfully on an app using v2.7 components? |
|
@bakura10 for now apigility components supports both v2 and v3, but of course when this module(possible some others also) does not support v3 it is impossible to update to v3 |
|
tests passed with v2 and v3 worked :D also not much changed only factories and event manager attach methods, but they are covered with tests I think. |
svycka
deleted the
improvement/servicemanager-v3-and-eventmanager-v3-support
branch
|
Merged. I'll wait a few days to give people time to try the dev-master in their app :). |
|
ok :D only zfr-rbac module left what holds me from upgrading :D |
I hope we can rise php required version to 5.5 no?