Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.2k
GetResponse*Events stop after a response was set#4532
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This alsoapplies to the kernel.request event, doesn't it? |
@xabbuh you're right, I looked for the uses of the subclasses but not of the main class, good catch. I'll update this PR in no time. |
| .. note:: | ||
| When setting a response for the ``kernel.view`` event, the propagation | ||
| is stopped, so the lower priority listeners on that event don't get called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this might cause some confusion. What about: "When setting a response for thekernel.view event, the propagation is stopped. This means listeners with lower priority won't be executed."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A full stop makes more sense to separate the two concepts. I'll change it.
| @@ -391,6 +396,11 @@ At this stage, if no listener sets a response on the event, then an exception | |||
| is thrown: either the controller *or* one of the view listeners must always | |||
| return a ``Response``. | |||
| .. note:: | |||
| When setting a response for the ``kernel.view`` event, the propagation | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
2 times the same note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's for different events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
https://github.com/symfony/symfony-docs/pull/4532/files#diff-e15ca2d264f8727e6c744c50d918aecaR172
https://github.com/symfony/symfony-docs/pull/4532/files#diff-e15ca2d264f8727e6c744c50d918aecaR401
2 times " When setting a response for thekernel.view event, the propagation"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh yes you're right. Should bekernel.request above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're right, it's fixed now 😄
| @@ -57,6 +57,12 @@ event is just one of the core kernel events:: | |||
| the ``kernel.exception`` event, it is :class:`Symfony\\Component\\HttpKernel\\Event\\GetResponseForExceptionEvent`. | |||
| To see what type of object each event listener receives, see :class:`Symfony\\Component\\HttpKernel\\KernelEvents`. | |||
| .. note:: | |||
| When setting a response for the ``kernel.request``, ``kernel.view`` and | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldnt this "or" instead oft "and"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In fact if you set it for kernel.request kernel.view won't even be triggered, and kernel.exception is exclusive with kernel.view (and is triggered after kernel.request) so I thought and was better.
Also, I'm refering to the set of the 3 events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe it's due to how you would write this in german by I agree with@timglabisch here that "or" sounds more correct.
What do you think about rewording this a bit to make it more clear (sadly, I don't have a good idea right now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yeah, in german "or" would be more natural.
`kernel.request``, ``kernel.view`` andyou dont have to set kernel.request AND kernel.view AND ...., it's more like this is true for kernel.request || kernel.view | | ... :)
but "and" sounds ok for me, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It should indeed be OR. The event has to be either kernel.request, kernel.view or kernel.exception. It can't be kernel.request AND kernel.view AND kernel.exception at the same time, it's one of them. (but well, this is a very high technical view on this topic, I think both are ok to use)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree - should be OR - I've got it changed at sha:5842f5c
Thanks!
Excellent addition indeed - thanks so much@Lumbendil! |
…ndil)This PR was merged into the 2.3 branch.Discussion----------GetResponse*Events stop after a response was set| Q | A| ------------- | ---| Doc fix? | yes| New docs? | no| Applies to | All versions| Fixed tickets |#4516The kernel.view and kernel.exception events stop propagation when a response is set, and it wasn't noted on the documentation.Commits-------bebce0e Fix incorrect event name.e729750 Changed phrasing to explain the effects off propagation stopping.d13943a Add missing info about kernel.request event.25e1069 Added notes specifying the propagation behaviour for kernel.view and kernel.exception.
* 2.3: (27 commits) Update forms.rst link to the API documentation Re-wording parameters.yml section and removing note about vendor#4608 [#4608] Using #. for numbered bullets Changing and -> or based on conversation in#4532 Changed to definition lists from Book section Changed to definition lists Fixed heading capitalization to follow the standards Changed paragraph to heading Changed unordered list to definition list Spelling mistake tens to tons Update controllers.rst Update the_controller.rst replace Symfony2 with Symfony Linked the PDO/DBAL Session article from the Doctrine category Fix indentation of YAML example Fixed some code indentation Fixed section headers Minor grammar fix Restored the original section title ...Conflicts:book/forms.rstbook/internals.rstcomponents/http_foundation/introduction.rstcookbook/map.rst.inc
* 2.5: (30 commits) Update forms.rst link to the API documentation Re-wording parameters.yml section and removing note about vendor#4608 [#4608] Using #. for numbered bullets Changing and -> or based on conversation in#4532 Changed to definition lists from Book section Changed to definition lists Fixed heading capitalization to follow the standards Changed paragraph to heading Changed unordered list to definition list Spelling mistake tens to tons Removed double `firewall_restriction` entry Normalize the method listings on version 2.5 Update controllers.rst Update the_controller.rst replace Symfony2 with Symfony Linked the PDO/DBAL Session article from the Doctrine category Fix indentation of YAML example Fixed some code indentation Fixed section headers ...
* 2.7: (35 commits) Update forms.rst link to the API documentation Re-wording parameters.yml section and removing note about vendor#4608 [#4608] Using #. for numbered bullets Changing and -> or based on conversation in#4532 Changed to definition lists from Book section Changed to definition lists Fixed heading capitalization to follow the standards Changed paragraph to heading Changed unordered list to definition list Spelling mistake tens to tons Removed double `firewall_restriction` entry Normalize the method listings on version 2.5 Update controllers.rst Update the_controller.rst replace Symfony2 with Symfony Linked the PDO/DBAL Session article from the Doctrine category Fix indentation of YAML example Fixed some code indentation Matching up the index position with the map ...
The kernel.view and kernel.exception events stop propagation when a response is set, and it wasn't noted on the documentation.