Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Conversation

@Warxcell
Copy link
Contributor

@WarxcellWarxcell commentedJun 19, 2021
edited by nicolas-grekas
Loading

QA
Branch?5.4
Bug fix?no
New feature?no
Deprecations?yes
TicketsFix#41748
LicenseMIT
Doc PR

@WarxcellWarxcell requested a review fromsroze as acode ownerJune 19, 2021 16:07
@WarxcellWarxcell changed the base branch from5.4 to5.2June 19, 2021 16:07
@WarxcellWarxcellforce-pushed thefix_reject_redelivered_message_middleware branch from88b914e toe158c56CompareJune 19, 2021 16:37
@xabbuh
Copy link
Member

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
Copy link
ContributorAuthor

Warxcell commentedJun 19, 2021
edited
Loading

Can you show the stack trace of the error that you receive? I do not really see why the current code should trigger it.

Sure,https://travis-ci.org/github/Warxcell/files/jobs/773978302#L785

If I docomposer require symfony/amqp-messenger - it's working again. But I don't really need that package.

@xabbuh
Copy link
Member

I think we should prevent the use ofReflectionClass instead if the given class does not exist:#41751

@Warxcell
Copy link
ContributorAuthor

Warxcell commentedJun 19, 2021
edited
Loading

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
Copy link
Member

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 the5.4 branch.

@Warxcell
Copy link
ContributorAuthor

Warxcell commentedJun 20, 2021
edited
Loading

Ok, so I will redirect this to 5.4 and your fix will be patched in 5.2 ?

@WarxcellWarxcell changed the base branch from5.2 to5.4June 20, 2021 06:51
@WarxcellWarxcellforce-pushed thefix_reject_redelivered_message_middleware branch 2 times, most recently fromc6a8671 to2937f33CompareJune 20, 2021 06:52
@nicolas-grekas
Copy link
Member

Please fix commit+PR title
and add a line in the changelog of the component, and in UPGRADE files for 5.4 and 6.0.

Warxcell reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas added this to the5.4 milestoneJun 21, 2021
@carsonbotcarsonbot changed the titleFix Class AmqpReceivedStamp does not exist[Messenger] Fix Class AmqpReceivedStamp does not existJun 21, 2021
@WarxcellWarxcellforce-pushed thefix_reject_redelivered_message_middleware branch from2937f33 to533f5b7CompareJune 21, 2021 21:13
@WarxcellWarxcell changed the title[Messenger] Fix Class AmqpReceivedStamp does not exist[Messenger] Move RejectRedeliveredMessageMiddleware to AMQP Package.Jun 21, 2021
@WarxcellWarxcellforce-pushed thefix_reject_redelivered_message_middleware branch from533f5b7 toe293f17CompareJune 21, 2021 21:15
@kozlice
Copy link

kozlice commentedJun 27, 2021
edited
Loading

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 tosymfony/amqp-messenger it has no business staying at the component level. Please disregard what I said, I'll find a way to make it work for another transport.

@OskarStarkOskarStark changed the title[Messenger] Move RejectRedeliveredMessageMiddleware to AMQP Package.[Messenger] MoveRejectRedeliveredMessageMiddleware to AMQP PackageAug 1, 2021
@fabpotfabpot modified the milestones:5.4,6.1Nov 16, 2021
@fabpotfabpot modified the milestones:6.1,6.2May 20, 2022
@fabpot
Copy link
Member

@Warxcell Looks good to me. Can you rebase on 6.2 and update the code accordingly? Thank you.

@nicolas-grekasnicolas-grekas modified the milestones:6.2,6.3Nov 5, 2022
@nicolas-grekasnicolas-grekas modified the milestones:6.3,6.4May 23, 2023
@nicolas-grekas
Copy link
Member

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
Copy link
Member

Thanks for having a look Nicolas. Let's close then.

@fabpotfabpot closed thisOct 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot requested changes

@srozesrozeAwaiting requested review from sroze

Assignees

No one assigned

Projects

None yet

Milestone

6.4

Development

Successfully merging this pull request may close these issues.

[Serializer] Class "Symfony\Component\Messenger\Bridge\Amqp\Transport\AmqpReceivedStamp" does not exist

6 participants

@Warxcell@xabbuh@nicolas-grekas@kozlice@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp