Added php-amqplib support #24
Conversation
| @@ -14,7 +14,8 @@ | |||
| "swarrot/swarrot": "~1.2", | |||
| "psr/log": "~1.0", | |||
| "symfony/framework-bundle": "~2.4", | |||
| "symfony/console": "~2.4" | |||
| "symfony/console": "~2.4", | |||
| "videlalvaro/php-amqplib": "2.4.*" | |||
There was a problem hiding this comment.
Please don't make it a hard dependency. People using the PECL extension don't need it
|
If it was up to me, I'd try to introduce a tag based detection as I proposed back in #2 , only simpler (just handling the provider in fact, rather than everything else) |
|
Why not for something like #2. :) Otherwise, @ThisIsAreku if you don't want do it yourself, something like this is enough to start : $id = 'swarrot.factory.'.$config['provider'];
if (!$container->has($id)) {
// throw exception
} |
| } else { | ||
| throw new \InvalidArgumentException('Only pecl is supported for now'); | ||
| $id = 'swarrot.factory.'.$config['provider']; | ||
| if (!$container->has($id)) { |
There was a problem hiding this comment.
note that this will still not allow other bundles to provide providers though. So it does not solve the case requested by #2
There was a problem hiding this comment.
I'm quite new to symfony stuff, i don't know how to do this :/
There was a problem hiding this comment.
meh, I think this is gonna be fine for now, as it means that at least all the providers supported by swarrot bundle (and also those registered as swarrot.factory.<something>, even in other bundles) can be easily registered.
the thing with the tag is just a little more advanced and does not limit the other bundles to use specifically this syntax. Hence, I think you can leave it like that, as it should be fine for now.
There was a problem hiding this comment.
and also those registered as swarrot.factory., even in other bundles
Precisely what is not working. The DI extension receives a temporary container, not the main one. So the only available services are the services defined by the DI extension itself (by loading the XML file 2 lines above)
There was a problem hiding this comment.
It's just to merge this PR quickly, I'm agree that the solution #2 is better. :)
There was a problem hiding this comment.
Oh, I thought it was the real container, which doesn't seem to be the case. My bad then.
There was a problem hiding this comment.
It's just to merge this PR quickly, I'm agree that the solution #2 is better. :)
Meh, I'll try to refactor it (simplifying, updating, ...) when I'll have a little bit of time then (except if somebody else wants to do it). :)
There was a problem hiding this comment.
Well, #2 moving all the DIC configuration to a compiler pass was not clean. There are probably better ways to provide such hook for external bundles
There was a problem hiding this comment.
Yup, I agree, it was just way too overkill ; But I think that at least handling the provider in a compiler pass should do just fine (especially since the introduction of the factories)
|
Can I merge this PR ? |
|
@odolbeau IMO, you can merge it like that. Providing better hooks is a separate feature |
👍 |
This fixes a mistakes introduced in swarrot#24. Closes swarrot#25
I've added support of https://github.com/videlalvaro/php-amqplib, and also config for ssl. Note that at the moment, pecl-amqp didn't support ssl