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

[Messenger] Added transport agnostic exception #30375

Merged
merged 5 commits into from Mar 4, 2019

Conversation

@lolmx
Copy link
Contributor

commented Feb 25, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #30346
License MIT
Doc PR TODO

As described in #30346, client code shouldn't care about which transport is currently used by the message bus. This pr adds a new generic exception that is thrown by the AmqpSender if the message couldn't be delivered.

@fabpot

fabpot approved these changes Feb 25, 2019

@kunicmarko20

This comment has been minimized.

Copy link

commented Feb 25, 2019

Should it also be added here:

https://github.com/symfony/messenger/blob/0c0e9cfc2fd50a5b6e242204c80855f2f385f9cb/Transport/AmqpExt/AmqpReceiver.php#L70 ?

Didn't check what exception can occur here but I guess it should be consistent.

@lolmx

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

I added a catch block whenever the $this->connection->ack($AMQPEnvelope); fails in the receiver, and surrounded $this->connection->reject($AMQPEnvelope); & $this->connection->nack($AMQPEnvelope); because they can fail too.

I did not alter this throw because it's not transport-related.

Although this one is related to transport, I choose to not trow a TransportException because it's an exception manually raised by client code, thus I let client do his stuff :)

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

Can you rebase your pull request on current master? You merged master and it makes the PR non-mergeable. thank you.

@lolmx lolmx force-pushed the lolmx:master branch from 8e3980a to 7d6a3fa Mar 4, 2019

@lolmx

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

My bad, here it is :)

@lolmx

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

I checked tests errors, it's Component/Debug and Bundle/FrameworkBundle, which are failing on upstream/master too

@fabpot

fabpot approved these changes Mar 4, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Thank you @lolmx.

@fabpot fabpot merged commit 7d6a3fa into symfony:master Mar 4, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 4, 2019

feature #30375 [Messenger] Added transport agnostic exception (nikoss…
…vnk, lolmx)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Messenger] Added transport agnostic exception

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30346
| License       | MIT
| Doc PR        | TODO

As described in #30346, client code shouldn't care about which transport is currently used by the message bus. This pr adds a new generic exception that is thrown by the `AmqpSender` if the message couldn't be delivered.

Commits
-------

7d6a3fa Updated changelog to document changes in AmqpReceiver
62a08ee Updated exception message in AmqpSender, updated AmqpReceiver to throw new TransportException
b2b0640 Chain new exception with previous one
06c8404 forgot one backslash, my bad
93c1001 [Messenger] Added new TransportException which is thrown if transport could not send a message

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.