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

Remove messenger transport service conditionally #1602

Closed
wants to merge 1 commit into from
Closed

Remove messenger transport service conditionally #1602

wants to merge 1 commit into from

Conversation

gndk
Copy link
Contributor

@gndk gndk commented Dec 29, 2022

Fixes #1601

@gndk gndk marked this pull request as draft December 29, 2022 13:31
@gndk gndk marked this pull request as ready for review December 29, 2022 13:53
@gndk gndk changed the title Only wire messenger if transport available Only wire messenger transport if available Dec 29, 2022
@gndk gndk changed the title Only wire messenger transport if available Only tag messenger transport if available Dec 29, 2022
@gndk
Copy link
Contributor Author

gndk commented Dec 29, 2022

Not sure about the best way to test based on optional dependencies, so I manually composer required/removed symfony/messenger and symfony/doctrine-messenger to verify the new tests locally. Should they be added to require-dev like doctrine/orm for some of the other tests in DoctrineExtensionTest?

Copy link
Member

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

Not sure about the best way to test based on optional dependencies, so I manually composer required/removed symfony/messenger and symfony/doctrine-messenger to verify the new tests locally. Should they be added to require-dev like doctrine/orm for some of the other tests in DoctrineExtensionTest?

The way we do it is that we manually require symfony/messenger in CI already (for some jobs)

- name: "Require symfony/messenger"
so I think the way you did it is sufficient

Resources/config/messenger.xml Outdated Show resolved Hide resolved
DependencyInjection/DoctrineExtension.php Outdated Show resolved Hide resolved
@ostrolucky ostrolucky added the Bug label Dec 30, 2022
@gndk
Copy link
Contributor Author

gndk commented Dec 30, 2022

Not sure about the best way to test based on optional dependencies, so I manually composer required/removed symfony/messenger and symfony/doctrine-messenger to verify the new tests locally. Should they be added to require-dev like doctrine/orm for some of the other tests in DoctrineExtensionTest?

The way we do it is that we manually require symfony/messenger in CI already (for some jobs)

- name: "Require symfony/messenger"

so I think the way you did it is sufficient

Maybe it would make sense to add the new combinations (symfony/doctrine-messenger require on/off) to the matrix then?

@ostrolucky
Copy link
Member

I don't think adding another combination to matrix has enough ROI, so no need to do something around that.

@gndk gndk changed the title Only tag messenger transport if available Remove messenger transport service conditionally Dec 30, 2022
@ostrolucky
Copy link
Member

Thx, merged manually

@ostrolucky ostrolucky closed this Dec 30, 2022
@gndk gndk deleted the fix-messenger branch December 30, 2022 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[2.8.0] Container compilation fails if symfony/doctrine-messenger is not used
3 participants