Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] MoveRejectRedeliveredMessageMiddleware to AMQP Package#41749
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Messenger] MoveRejectRedeliveredMessageMiddleware to AMQP Package#41749
Uh oh!
There was an error while loading.Please reload this page.
Conversation
88b914e toe158c56Comparexabbuh commentedJun 19, 2021
Can you show the stack trace of the error that you receive? I do not really see why the current code should trigger it. |
Warxcell commentedJun 19, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Sure,https://travis-ci.org/github/Warxcell/files/jobs/773978302#L785 If I do |
xabbuh commentedJun 19, 2021
I think we should prevent the use of |
Warxcell commentedJun 19, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Why? To me it's looks like this middleware is tighly coupled to AMQP package, why not ship it inside it and load it only when installed, rather than ship it globally in Messenger, load it always, and check on every message if class exists? |
xabbuh commentedJun 19, 2021
We can think about adding a new middleware in another namespace and deprecate the other one. But such a change must not happen in a patch release and would have to target the |
Warxcell commentedJun 20, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Ok, so I will redirect this to 5.4 and your fix will be patched in 5.2 ? |
c6a8671 to2937f33Comparenicolas-grekas commentedJun 21, 2021
Please fix commit+PR title |
2937f33 to533f5b7Compare533f5b7 toe293f17Comparekozlice commentedJun 27, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'm working on an alternative AMQP transport for messenger based onbunny library. Moving this class will complicate dependency tree. Can we choose the way@xabbuh proposes to avoid that? EDIT: This class is indeed so tightly coupled to |
RejectRedeliveredMessageMiddleware to AMQP Packagefabpot commentedAug 7, 2022
@Warxcell Looks good to me. Can you rebase on 6.2 and update the code accordingly? Thank you. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedOct 11, 2023
On closer look, I think we should close this PR. The linked issue has been fixed, which means that in practice, this is change is just a nice to have. Yes, the implementation knows only about some amqp stamp at the moment, but we could also imagine a day where it manages other kind of stamps. |
fabpot commentedOct 11, 2023
Thanks for having a look Nicolas. Let's close then. |
Uh oh!
There was an error while loading.Please reload this page.