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.

Sign up for GitHub

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

Jump to bottom

[Messenger] Move RejectRedeliveredMessageMiddleware to AMQP Package #41749

Closed
wants to merge 2 commits into from
Closed

[Messenger] Move RejectRedeliveredMessageMiddleware to AMQP Package #41749

wants to merge 2 commits into from

Conversation

Warxcell
Copy link
Contributor

@Warxcell Warxcell commented Jun 19, 2021 •
edited by nicolas-grekas

Q A
Branch? 5.4
Bug fix? no
New feature? no
Deprecations? yes
Tickets Fix #41748
License MIT
Doc PR

@Warxcell Warxcell requested a review from sroze as a code owner June 19, 2021 16:07
@Warxcell Warxcell changed the base branch from 5.4 to 5.2 June 19, 2021 16:07
@Warxcell Warxcell force-pushed the fix_reject_redelivered_message_middleware branch from 88b914e to e158c56 Compare June 19, 2021 16:37
@xabbuh
Copy link
Member

xabbuh commented Jun 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
Copy link
Contributor Author

Warxcell commented Jun 19, 2021 •
edited

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 do composer require symfony/amqp-messenger - it's working again. But I don't really need that package.

@xabbuh
Copy link
Member

xabbuh commented Jun 19, 2021

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

@Warxcell
Copy link
Contributor Author

Warxcell commented Jun 19, 2021 •
edited

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

xabbuh commented Jun 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 5.4 branch.

@Warxcell
Copy link
Contributor Author

Warxcell commented Jun 20, 2021 •
edited

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

@Warxcell Warxcell changed the base branch from 5.2 to 5.4 June 20, 2021 06:51
@Warxcell Warxcell force-pushed the fix_reject_redelivered_message_middleware branch 2 times, most recently from c6a8671 to 2937f33 Compare June 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.

@nicolas-grekas nicolas-grekas added this to the 5.4 milestone Jun 21, 2021
@carsonbot carsonbot changed the title Fix Class AmqpReceivedStamp does not exist [Messenger] Fix Class AmqpReceivedStamp does not exist Jun 21, 2021
@Warxcell Warxcell force-pushed the fix_reject_redelivered_message_middleware branch from 2937f33 to 533f5b7 Compare June 21, 2021 21:13
@Warxcell Warxcell changed the title [Messenger] Fix Class AmqpReceivedStamp does not exist [Messenger] Move RejectRedeliveredMessageMiddleware to AMQP Package. Jun 21, 2021
@Warxcell Warxcell force-pushed the fix_reject_redelivered_message_middleware branch from 533f5b7 to e293f17 Compare June 21, 2021 21:15
@kozlice
Copy link
Contributor

kozlice commented Jun 27, 2021 •
edited

I'm working on an alternative AMQP transport for messenger based on bunny 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 symfony/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.

@OskarStark OskarStark changed the title [Messenger] Move RejectRedeliveredMessageMiddleware to AMQP Package. [Messenger] Move RejectRedeliveredMessageMiddleware to AMQP Package Aug 1, 2021
@fabpot fabpot modified the milestones: 5.4, 6.1 Nov 16, 2021
@chrisguitarguy chrisguitarguy mentioned this pull request Jan 12, 2022
Closed
@fabpot fabpot modified the milestones: 6.1, 6.2 May 20, 2022
@fabpot
Copy link
Member

fabpot commented Aug 7, 2022

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

@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
fabpot
fabpot requested changes Oct 11, 2023
View reviewed changes
UPGRADE-5.4.md Show resolved Hide resolved
UPGRADE-5.4.md Show resolved Hide resolved
UPGRADE-6.0.md Show resolved Hide resolved
UPGRADE-5.4.md Show resolved Hide resolved
@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

fabpot commented Oct 11, 2023

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

@fabpot fabpot closed this Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot requested changes

@sroze sroze Awaiting requested review from sroze

Assignees
No one assigned
Labels
Deprecation Messenger Status: Needs Work
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