Skip to content

[Messenger] Add options to FailedMessagesShowCommand #39330

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

Closed

Conversation

fguimier
Copy link

@fguimier fguimier commented Dec 5, 2020

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR symfony/symfony-docs#14689

The goal to this PR is to improve the FailedMessagesShowCommand usability by adding two options:

  • The stats option allows to display the number of messages by the class name and have a summary of all the failed messages.
  • The class-filter option is a filter to only display the messages matching with the given class name.

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@fguimier fguimier changed the title Add options to FailedMessagesShowCommand [Messenger] Add options to FailedMessagesShowCommand Dec 5, 2020
@jderusse jderusse added this to the 5.x milestone Dec 6, 2020
fguimier added a commit to fguimier/symfony-docs that referenced this pull request Dec 10, 2020
Update exemples for the messenger:failed:show command

Add documentation regarding this PR symfony/symfony#39330
fguimier added a commit to fguimier/symfony-docs that referenced this pull request Dec 10, 2020
Update exemples for the messenger:failed:show command

Add documentation regarding this PR symfony/symfony#39330
@fguimier fguimier force-pushed the messenger-enhance-failed-show-cmd branch from 70c9347 to 6d16145 Compare December 29, 2020 09:46
@OskarStark OskarStark changed the title [Messenger] Add options to FailedMessagesShowCommand [Messenger] Add options to FailedMessagesShowCommand Aug 4, 2021
@OskarStark OskarStark requested a review from jderusse August 4, 2021 20:34
@OskarStark
Copy link
Contributor

HI, it looks like your commiter email address is not associated with your Github Account:

CleanShot 2021-08-04 at 22 34 08@2x

Copy link
Member

@jderusse jderusse left a comment

Choose a reason for hiding this comment

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

I like the idea ❤️

@fguimier fguimier force-pushed the messenger-enhance-failed-show-cmd branch from 6d16145 to feba4b0 Compare August 27, 2021 13:24
@fguimier fguimier force-pushed the messenger-enhance-failed-show-cmd branch from feba4b0 to c6cdb8f Compare August 27, 2021 13:29
Co-authored-by: Alexander M. Turek <me@derrabus.de>
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

You should also revert the permission change on src/Symfony/Component/Messenger/Tests/Command/FailedMessagesShowCommandTest.php (get back to 0644)

@@ -117,7 +117,7 @@ public function testBasicRunWithServiceLocator()
Error Things are bad!
Error Code 123
Error Class Exception
Transport async
Transport async
Copy link
Member

Choose a reason for hiding this comment

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

The extra spaces should be removed

@@ -198,7 +198,7 @@ public function testMultipleRedeliveryFailsWithServiceLocator()
Error Things are bad!
Error Code 123
Error Class Exception
Transport async
Transport async
Copy link
Member

Choose a reason for hiding this comment

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

The extra spaces should be removed

@carsonbot carsonbot changed the title [Messenger] Add options to FailedMessagesShowCommand Add options to FailedMessagesShowCommand Sep 21, 2021
@fabpot fabpot modified the milestones: 5.4, 6.1 Nov 16, 2021
@carsonbot carsonbot changed the title Add options to FailedMessagesShowCommand [Messenger] Add options to FailedMessagesShowCommand Dec 29, 2021
@fabpot fabpot modified the milestones: 6.1, 6.2 May 20, 2022
@fabpot
Copy link
Member

fabpot commented Jul 21, 2022

Closing in favor of #47008, where I've made some minor changes to finish it.

@fabpot fabpot closed this Jul 21, 2022
fabpot added a commit that referenced this pull request Jul 21, 2022
… (Florian Guimier, fabpot)

This PR was merged into the 6.2 branch.

Discussion
----------

[Messenger] Add options to `FailedMessagesShowCommand`

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #39330 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#14689

See #39330

Commits
-------

e99d2ca Finish work
7f774b4 Add group and class-filter options to FailedMessagesShowCommand
javiereguiluz pushed a commit to javiereguiluz/symfony-docs that referenced this pull request Aug 3, 2022
Update exemples for the messenger:failed:show command

Add documentation regarding this PR symfony/symfony#39330
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants