NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Messenger] Add debug:messenger CLI command#26803

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

Jump to bottom

Merged

sroze merged 3 commits intosymfony:4.1fromro0NL:debug-messenger

May 9, 2018

Merged

[Messenger] Add debug:messenger CLI command#26803

sroze merged 3 commits intosymfony:4.1fromro0NL:debug-messenger

May 9, 2018

Conversation

Copy link

Contributor

ro0NL commentedApr 4, 2018

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#...
License	MIT
Doc PR	symfony/symfony-docs#...

Adds adebug:messenger CLI command to expose message classes that can be dispatched. Heavily inspired bydebug:autowiring.

carsonbot added Status: Needs Review Feature labels

Apr 4, 2018

Copy link

Contributor

ogizanagi commentedApr 4, 2018

One less PR to do on my list :)

The tactician debug command looks like this:

Tactician routing=================Bus: default------------ ----------------------------------------------------- -----------------   Command                                               Handler Service   ----------------------------------------------------- -----------------   League\Tactician\Bundle\Tests\Fake\FakeCommand        a.handler          League\Tactician\Bundle\Tests\Fake\OtherFakeCommand   b.handler         ----------------------------------------------------- -----------------

https://github.com/thephpleague/tactician-bundle/blob/master/tests/Integration/DebugCommandTest.php#L37-L48

Would be great to have messages <-> handlers too here.

ogizanagi reviewed

Apr 4, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/DebugMessengerCommand.php Outdated

		{
		$this
		->setDefinition(array(
		newInputArgument('search', InputArgument::OPTIONAL,'A search filter'),

Copy link

Contributor

ogizanagiApr 4, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Not convinced this is useful until we have more info to show about a message. A simplegrep is enough for this for now.

Copy link

ContributorAuthor

ro0NL commentedApr 4, 2018•
edited
Loading

A message can have multiple handlers .. 🤔

Also the intend was concerning end users who mostly care about$bus->dispatch($whatCanIPutHere).

Copy link

Contributor

ogizanagi commentedApr 4, 2018

A message can have multiple handlers ..

Wasn't the case with tactician, but it's not a problem here. Just show all associated handlers.

Also the intend was concerning end users who mostly care about $bus->dispatch($whatCanIPutHere).

Would still be useful in this way :)

ogizanagi added the Messenger label

Apr 4, 2018

nicolas-grekas added this to the4.1 milestone

Apr 5, 2018

carsonbot removed the Status: Needs Review label

Apr 6, 2018

fabpot previously requested changes

Apr 6, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/DebugMessengerCommand.php Outdated

		* file that was distributed with this source code.
		*/

		namespaceSymfony\Bundle\FrameworkBundle\Command;

Copy link

Member

fabpotApr 6, 2018

Choose a reason for hiding this comment

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

Should be moved to the component like the other Messenger commands.

Copy link

ContributorAuthor

ro0NLApr 8, 2018

Choose a reason for hiding this comment

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

Done.

carsonbot added the Status: Needs Work label

Apr 6, 2018

Copy link

ContributorAuthor

ro0NL commentedApr 8, 2018

Latest version

sroze suggested changes

Apr 8, 2018

View reviewed changes

src/Symfony/Component/Messenger/Command/DebugMessengerCommand.php

		$tableRows =array();
		foreach ($this->mappingas$message =>$handlers) {
		$tableRows[] =array(sprintf('<fg=cyan>%s</fg=cyan>',$message));
		foreach ($handlersas$handler) {

Copy link

Contributor

srozeApr 8, 2018

Choose a reason for hiding this comment

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

I think that having the list of handlers in another column would make more sense and be more coherent with the rest of thedebug: commands as well.

Copy link

ContributorAuthor

ro0NLApr 22, 2018

Choose a reason for hiding this comment

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

well.. i tookdebug:autowring for inspiration. IMHO that one reads well :)

Also with multiple columns we probably run into a spacing issue, assuming all service IDs are FQCN.

src/Symfony/Component/Messenger/Command/DebugMessengerCommand.php Outdated

		$io =newSymfonyStyle($input,$output);

		$io->title('Message Bus');
		$io->text('The following classes can be dispatch:');

Copy link

Contributor

srozeApr 8, 2018

Choose a reason for hiding this comment

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

The following messages can be dispatched:

src/Symfony/Component/Messenger/Command/DebugMessengerCommand.php Outdated

		{
		$io =newSymfonyStyle($input,$output);

		$io->title('Message Bus');

Copy link

Contributor

srozeApr 8, 2018

Choose a reason for hiding this comment

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

Messenger

Copy link

Member

weaverryan commentedApr 21, 2018

Ido like the table layout mentioned above#26803 (comment) - it's just a bit easier to read.

I realize that it's possible for a message to have several handlers. This is an edge-case, and I think we could handle that like this:

 ----------------------------------------------------- -----------------   Command                                               Handler Service   ----------------------------------------------------- -----------------   League\Tactician\Bundle\Tests\Fake\FakeCommand        a.handler                                                                c.handler          League\Tactician\Bundle\Tests\Fake\OtherFakeCommand   b.handler         ----------------------------------------------------- -----------------

Copy link

Contributor

sroze commentedApr 25, 2018

This PR needs to be updated following#26864 :)

Copy link

ContributorAuthor

ro0NL commentedApr 28, 2018

Rebased.

@sroze we cant group messages per bus as there's no such concept, so it's still a single list of messages.

@weaverryan can you update the table example with handler services likeLeague\Tactician\Bundle\Tests\Fake\FakeCommandHandler and then tell me if 2 column layout works for you :)

Copy link

Member

weaverryan commentedApr 28, 2018

As we talked about, the table layout I suggested won’t work - class names are too long. So, the original format should be used, but probably with some added coloration to make it nice and clear :)

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Apr 30, 2018

sroze approved these changes

Apr 30, 2018

View reviewed changes

Copy link

Contributor

sroze left a comment

Choose a reason for hiding this comment

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

Looks good. Just two minor comments.

src/Symfony/Bundle/FrameworkBundle/Resources/config/console.xml Outdated

		<tagname="console.command"command="messenger:consume-messages" />
		</service>

		<serviceid="console.command.messenger_debug"class="Symfony\Component\Messenger\Command\DebugMessengerCommand">

Copy link

Contributor

srozeApr 30, 2018

Choose a reason for hiding this comment

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

All the commands are namedNameDebugCommand, notDebugNameCommand. Could you change it to be something consistent with the rest?

Copy link

ContributorAuthor

ro0NLMay 1, 2018

Choose a reason for hiding this comment

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

All the commands exceptDebugAutowiringCommand.. which i took as a template 😅 now updated.

src/Symfony/Component/Messenger/Command/DebugMessengerCommand.php Outdated

		protectedfunctionconfigure()
		{
		$this
		->setDescription('Lists classes you can dispatch using the message bus')

Copy link

Contributor

srozeApr 30, 2018

Choose a reason for hiding this comment

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

I suggest "message classes" or "messages" (as you say "The following messages [...]" in the output)

chalasr approved these changes

May 3, 2018

View reviewed changes

Copy link

Member

chalasr left a comment

Choose a reason for hiding this comment

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

Looks good. A functional test case would have been nice, maybe could you just share a screenshot of the final output?

Copy link

Member

weaverryan commentedMay 3, 2018

Here's a current screenshot, which I think looks quite good :).

We could perhaps do it in a different PR, but what about showing where each message is routed?

weaverryan previously requested changes

May 3, 2018

View reviewed changes

src/Symfony/Component/Messenger/Command/MessengerDebugCommand.php Outdated

		$tableRows[] =array(sprintf(' handled by %s',$handler));
		}
		}
		$io->table(array(),$tableRows);

Copy link

Member

weaverryanMay 3, 2018

Choose a reason for hiding this comment

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

Minor, but we should print some message here if there are NO messages, like:

No messages were found that have valid handlers.

Copy link

Contributor

srozeMay 7, 2018

Choose a reason for hiding this comment

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

@ro0NL can you update this PR at some point? Will be for beta2 now but can still be in 4.1 :)

Copy link

ContributorAuthor

ro0NLMay 8, 2018

Choose a reason for hiding this comment

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

Done.

carsonbot added Status: Needs Work and removed Status: Reviewed labels

May 3, 2018

sroze dismissedfabpot’sstale review

May 3, 2018 13:15

Changes have been made 20+ days ago :)

nicolas-grekas changed the base branch frommaster to4.1

May 7, 2018 15:06

yceruto reviewed

May 8, 2018

View reviewed changes

Copy link

Member

yceruto left a comment

Choose a reason for hiding this comment

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

👍

src/Symfony/Component/Messenger/Command/MessengerDebugCommand.php Outdated

		*
		* @experimental in 4.1
		*/
		class MessengerDebugCommandextends Command

Copy link

Member

ycerutoMay 8, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't it be namedDebugCommand like debug commands in other components?

Copy link

Contributor

srozeMay 8, 2018

Choose a reason for hiding this comment

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

Good point@yceruto, you are right. Commands within components' namespace are not prefixed with the component name, let's keep it this way :)

Copy link

Contributor

srozeMay 9, 2018

Choose a reason for hiding this comment

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

Renamed 👍

ro0NLand others added2 commits

May 9, 2018 09:27

[Messenger] Add debug:messenger CLI command

9847b83

Rename the commandDebugCommand

290c7eb

sroze dismissedweaverryan’sstale review

May 9, 2018 08:34

Change have been made

carsonbot added Status: Reviewed and removed Status: Needs Work labels

May 9, 2018

sroze approved these changes

May 9, 2018

View reviewed changes

Copy link

ContributorAuthor

ro0NL commentedMay 9, 2018

Thx@sroze .. still no clue about failing test :(

Copy link

Contributor

ogizanagi commentedMay 9, 2018•
edited
Loading

@ro0NL :deps=high failure expected. Fordeps=low, try updating the Framework bundlecomposer.json file to requiresymfony/messenger~4.1-beta2

Copy link

Contributor

ogizanagi commentedMay 9, 2018•
edited
Loading

The-beta2 part is important, asprefer-stable andprefer-lowest is used, so it selects the released beta1 first when using just@beta

Copy link

ContributorAuthor

ro0NL commentedMay 9, 2018

your're right. switched it to@dev. Not surebeta2 will resolve... :S should we try? 😓

Copy link

Contributor

ogizanagi commentedMay 9, 2018

That's usually what is done, so unless I'm missing something it should work:e81005e. It should resolve as far asminimum-stability allows it (which does)

fix deps

7f87309

Copy link

Contributor

sroze commentedMay 9, 2018

Yay, and tests are all green now. Thanks@ogizanagi for the hint.

Copy link

Contributor

sroze commentedMay 9, 2018

Thanks@ro0NL for working on this feature, this is much appreciated.

sroze merged commit7f87309 intosymfony:4.1

May 9, 2018

sroze added a commit that referenced this pull request

May 9, 2018

feature#26803[Messenger] Add debug:messenger CLI command (ro0NL, sr…

6207d70

…oze)This PR was merged into the 4.1 branch.Discussion----------[Messenger] Add debug:messenger CLI command| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->Adds a `debug:messenger` CLI command to expose message classes that can be dispatched. Heavily inspired by `debug:autowiring`.Commits-------7f87309 fix deps290c7eb Rename the command `DebugCommand`9847b83 [Messenger] Add debug:messenger CLI command