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

[#4231] Clarify that only the main command triggers events #4966

Merged

Conversation

riperez
Copy link
Contributor

@riperez riperez commented Feb 6, 2015

Q A
Doc fix? yes
New docs? no
Applies to all
Fixed tickets #4231

@riperez
Copy link
Contributor Author

riperez commented Feb 6, 2015

This is my first contribution so please be kind to me ;)

@@ -20,6 +20,11 @@ the wheel, it uses the Symfony EventDispatcher component to do the work::
$application->setDispatcher($dispatcher);
$application->run();

.. caution::

Console application lifecycle events are only triggered by the main command being executed. Inner or nested
Copy link
Member

Choose a reason for hiding this comment

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

I think we should call them "Console events" (just like we did in the versionadded directive).

@wouterj
Copy link
Member

wouterj commented Feb 6, 2015

It's a very good start! Correct branch, fixing a ticket, using correct syntax, etc. Great!

Besides my two pretty minor comments above, you should add a line break after the first word that crosses the 72th character. This does include the 4 spaces of indent.

@riperez riperez force-pushed the clarify_who_triggers_events_on_commands branch from 3f18fac to 3f5ce4d Compare February 6, 2015 23:37
@riperez
Copy link
Contributor Author

riperez commented Feb 6, 2015

Second version following @wouterj advice. Thanks for the help!

@wouterj
Copy link
Member

wouterj commented Feb 7, 2015

👍 from me

@xabbuh
Copy link
Member

xabbuh commented Feb 8, 2015

@riperez Looks great to me too. 👍

@weaverryan
Copy link
Member

Looks perfect to me! Thanks Richard for the PR. Always even better to have a PR that closes an issue too :). Cheers!

@weaverryan weaverryan merged commit 3f5ce4d into symfony:2.3 Feb 24, 2015
weaverryan added a commit that referenced this pull request Feb 24, 2015
…nts (riperez)

This PR was merged into the 2.3 branch.

Discussion
----------

[#4231] Clarify that only the main command triggers events

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | all
| Fixed tickets | #4231

Commits
-------

3f5ce4d [#4231] Clarify that only the main command triggers events
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.

4 participants