Skip to content

Conversation

@fabpot
Copy link
Member

@fabpot fabpot commented Mar 24, 2013

Q A
Doc fix? yes
New docs? yes (symfony/symfony#7466)
Applies to master
Fixed tickets n/a
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be ConsoleEvents::COMMAND rather than the value ? (same goes for the code)

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in the code

Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be only in the code ? console.code is an implementation detail that should not be exposed to end user I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it were just me, I would remove the constants altogether. I've followed what was done in the docs for HttpKernel.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think they're nice because:

  • they allow autocompletion,
  • magic values are bad (as seen on several other PRs)

Anyway, the current documentation is a bad compromise. If I were you I would pick one or the other way, not a weird mix.

Copy link
Contributor

Choose a reason for hiding this comment

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

And of course a typo in a constant name would produce an error while a typo in a magic value would cause a sometime hard to debug issue.

fabpot added a commit to symfony/symfony that referenced this pull request Mar 25, 2013
This PR was merged into the master branch. Discussion ---------- Console dispatcher | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #3889, #6124 | License | MIT | Doc PR | symfony/symfony-docs#2352 refs #1884, #1929 This is an alternative implementation for adding events to console applications. This implementation has the following features: * Available for anyone using the Console component and it is not tied to FrameworkBundle (this is important as one thing we are trying to solve is email sending from a command, and frameworks like Silex using the Console component needs a solution too); * Non-intrusive as the current code has not been changed (except for renaming an internal variable that was wrongly named -- so that's not strictly needed for this PR) * The new DispatchableApplication class also works without a dispatcher, falling back to the regular behavior. That makes easy to create applications that can benefit from a dispatcher when available, but can still work otherwise. * Besides the *before* and *after* events, there is also an *exception* event that is dispatched whenever an exception is thrown. * Each event is quite powerful and can manipulate the input, the output, but also the command to be executed. Commits ------- 4f9a55a refactored the implementation of how a console application can handle events 4edf29d added helperSet to console event objects f224102 Added events for CLI commands
fabpot added a commit to symfony/console that referenced this pull request Mar 25, 2013
This PR was merged into the master branch. Discussion ---------- Console dispatcher | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #3889, #6124 | License | MIT | Doc PR | symfony/symfony-docs#2352 refs #1884, #1929 This is an alternative implementation for adding events to console applications. This implementation has the following features: * Available for anyone using the Console component and it is not tied to FrameworkBundle (this is important as one thing we are trying to solve is email sending from a command, and frameworks like Silex using the Console component needs a solution too); * Non-intrusive as the current code has not been changed (except for renaming an internal variable that was wrongly named -- so that's not strictly needed for this PR) * The new DispatchableApplication class also works without a dispatcher, falling back to the regular behavior. That makes easy to create applications that can benefit from a dispatcher when available, but can still work otherwise. * Besides the *before* and *after* events, there is also an *exception* event that is dispatched whenever an exception is thrown. * Each event is quite powerful and can manipulate the input, the output, but also the command to be executed. Commits ------- 4f9a55a refactored the implementation of how a console application can handle events 4edf29d added helperSet to console event objects f224102 Added events for CLI commands
@wouterj
Copy link
Member

wouterj commented Mar 25, 2013

@weaverryan this is ready for merging as the code is merged

@fabpot
Copy link
Member Author

fabpot commented Mar 26, 2013

I've just changed all event name to use constants. Mergeable now.

weaverryan added a commit that referenced this pull request Mar 30, 2013
added documentation for the dispatchable console application
@weaverryan weaverryan merged commit c7c215f into symfony:master Mar 30, 2013
@weaverryan
Copy link
Member

Great new feature and well-explained!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants