Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only add Ampq transport factories when packages are found #241

Merged
merged 1 commit into from Oct 23, 2017
Merged

Only add Ampq transport factories when packages are found #241

merged 1 commit into from Oct 23, 2017

Conversation

jverdeyen
Copy link
Contributor

No description provided.

@makasim
Copy link
Member

makasim commented Oct 22, 2017

AmqpTransportFactory is in enqueue/enqueue package which is always required by the bundle.
The factory inside does class_exists checks and uses only what is available.

@jverdeyen
Copy link
Contributor Author

But the RabbitMqAmqpTransportFactory uses DelayStrategyTransportFactoryTrait which I am no using with the following reqs:

"enqueue/enqueue-bundle": "<=1.0.0",
"enqueue/job-queue": "<=1.0.0",
"enqueue/sqs": "<=1.0.0",
"enqueue/redis": "<=1.0.0",
"enqueue/dbal": "<=1.0.0",

Should I always require enqueue/enqueue also ?

@makasim
Copy link
Member

makasim commented Oct 22, 2017

You shouldn't, the bundle heavily rely on it and hence require it itself

@makasim
Copy link
Member

makasim commented Oct 22, 2017

Are there any errors you face?

@jverdeyen
Copy link
Contributor Author

The DelayStrategyTransportFactoryTrait is from the ampq-tools package. Which isn't a requirement for enqueue/enqueue nor for the enqueue-bundle. Or am I missing something? I get an error that the Trait can't be found. Since the bundle tries to load the RabbitMqAmqpTransportFactory which uses the DelayStrategyTransportFactoryTrait.

@makasim
Copy link
Member

makasim commented Oct 23, 2017

Okay, let's merge this then. Could you please CS issues please?

@jverdeyen
Copy link
Contributor Author

This is the correct check to avoid adding the TransportFactories?

https://github.com/php-enqueue/enqueue-dev/pull/241/files#diff-21599724c6f8c888b53103436890a85fR60

@makasim
Copy link
Member

makasim commented Oct 23, 2017

This is the correct check to avoid adding the TransportFactories?

yeap

@makasim makasim merged commit 0a25800 into php-enqueue:master Oct 23, 2017
@makasim
Copy link
Member

makasim commented Oct 30, 2017

Commit contains better fix for this issue 031380e. I kept both changes for now, though ones you added are not needed any more

ASKozienko pushed a commit that referenced this pull request Nov 2, 2018
Only add Ampq transport factories when packages are found
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants