10000 [Messenger] Mention the transport which failed during the setup command by thePanz · Pull Request #49044 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] Mention the transport which failed during the setup command #49044

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
Merged
merged 1 commit into from
Oct 6, 2023
Merged

[Messenger] Mention the transport which failed during the setup command #49044

merged 1 commit into from
Oct 6, 2023

Conversation

thePanz
Copy link
Contributor
@thePanz thePanz commented Jan 20, 2023 •
edited by fabpot
Loading

The transport name can help to further investigate the underlying reasons of the failure

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets none
License MIT
Doc PR -

I am not sure if this can be labelled as a new feature, but the addition from this PR can help to further investigate errors while setting up an application's transports.

At the moment, if an error occurs while setting up a transport just the exception is thrown, with no context on which transport was being setup.

@carsonbot carsonbot added this to the 6.3 milestone Jan 20, 2023
@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.3" but it seems your PR description refers to branch "6.3 for features / 5.4, 6.0, 6.1, or 6.2 for bug fixes".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@thePanz thePanz changed the base branch from 6.3 to 5.4 January 20, 2023 14:18
OskarStark
OskarStark reviewed Jan 20, 2023
View reviewed changes
Copy link
Contributor
@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

This is a new feature, so please target 6.3, thanks

@thePanz thePanz changed the base branch from 5.4 to 6.3 January 20, 2023 14:59
@thePanz thePanz requested review from OskarStark and removed request for dunglas, lyrixx, wouterj, xabbuh, yceruto and chalasr January 20, 2023 14:59
@thePanz
Copy link
Contributor Author
thePanz commented Jan 20, 2023

This is a new feature, so please target 6.3, thanks

OK, thanks for clarifying 👍 (sad that we're still on sf v5.4 😭 )

@thePanz
Copy link
Contributor Author
thePanz commented Jan 20, 2023

@OskarStark thanks for reviewing.

I guess the failed CIs are not due to this PR (pSaml and appveyor reports errors not related to the messenger component)

nicolas-grekas
nicolas-grekas reviewed Jan 26, 2023
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGMT, I just have minor notes.

@thePanz
[Messenger] Mention the transport which failed during the setup command
49c0af9 
The transport name can help to further investigate the underlying reasons of the failure
@thePanz thePanz requested review from nicolas-grekas and OskarStark and removed request for OskarStark and nicolas-grekas February 14, 2023 14:51
@thePanz thePanz requested review from nicolas-grekas and OskarStark and removed request for OskarStark and nicolas-grekas February 14, 2023 14:52
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@fabpot
Copy link
Member
fabpot commented Oct 6, 2023

Thank you @thePanz.

@fabpot fabpot mentioned this pull request Oct 6, 2023
Closed
@fabpot fabpot merged commit 8ac7385 into symfony:6.4 Oct 6, 2023
@thePanz
Copy link
Contributor Author
thePanz commented Oct 12, 2023

Thanks @fabpot for merging!

@thePanz thePanz deleted the messenger-mention-transport-failing-to-setup branch October 12, 2023 16:07
This was referenced Oct 21, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@fabpot fabpot fabpot approved these changes

@OskarStark OskarStark Awaiting requested review from OskarStark

Assignees
No one assigned
Labels
Messenger Status: Needs Review
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
5 participants
@thePanz @carsonbot @fabpot @nicolas-grekas @OskarStark
0