Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
Synchronized Service alternative, backwards compatible#8904
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
Some alternative names for
|
@jakzal hehe, I was also thinking about a better name, especially because the event name does not match the other ones. Naming the event So, perhaps |
I've renamed the event to |
for me the name of the events should be in past tense, an event is something happened in the past, should be also written in the doc under best practice :). |
@liuggio hmmm, an event is something that is happening right now, so present. If you have a look at the names of events for the DOM for instance, we have click, load, drag, ... |
@fabpot, I'm not convinced, because makes more sense to me the definition that is something that is happened and then is fired, and also for the Domain EventDE the event is something of the past, and make sense to me. About the DOM I suppose they have used it in favor of the name of the "Attribute"(listener)... btw good feature. |
odino commentedSep 1, 2013
+1 for the past tense |
Let me say it again, the present tense makes more sense because:
|
odino commentedSep 1, 2013
In general, my guess is that events, since they are fired after something happened, should have past tense. Googling around I also foundthis in addition of what@liuggio posted, I think we should rely on what the DDD community has already discussed - I might lack of context here, so it might be that you need consistency as you can't rename |
fmeynard commentedSep 2, 2013
You can use past or present tense for events, for me it's just depends of the event itself. For example if you want to persist entity, you could have an event named "persist" allowing the modification of the entity BEFORE the save in database and you could also have another event "persisted" which one is raised after the save. ( For the same goal doctrine use "pre" & "post" prefix to be clear ) |
Still +1 on this feature :-) |
do not rely on past tense for naming, what about irregular verbs? thereby post and pre prefixes. |
Guys, can we close the discussion about past/present tense? Symfony uses both versions, depending on circumstances. Both sides presented their views but it's unlikely that existing names change in Symfony for BC reasons. |
| public function __construct($defaultLocale = 'en', RequestContextAwareInterface $router = null) | ||
| public function __construct($defaultLocale = 'en',RequestContext $requestContext,RequestContextAwareInterface $router = null) |
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.
This BC break still need to be handled (it affects Silex for instance)
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.
this class is not@api and constructors are not part of the interface of an class in terms of API Backwards compatiblitiy in my opinion, because their construction is another concern and handled by a factory of sorts.
Same for the other constructor change below.
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.
@beberlei This is valid only if the factories are themselves part of the component and so updated at the same time. It is not the case here. Merging this as is will break Silex code while the component was marked as stable and so Silex is already using~2.3 as requirement based on this expectation.
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.
While i see the validity of this argument, this would mean that Symfonys path using DIC without factories has a flaw.
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.
This class should now work in 2.3 or 2.4+ without any change.
I still have some open questions:
|
For the RequestContext name conflict, I think that having 2 classes (RequestContext and RequestStack) can be confusing anyway and protecting users from doing stupid things is probably not needed. So, I propose to remove the ReqeustContext class in favor of only RequestStack and move it into HttpFoundation. Sounds good? |
I've done the name change and the move in the last commit. |
Documentation has been updated now. I'm ready to merge this PR. Any other thoughts? |
…om the event dispatcher at the end of the request
This PR was merged into the master branch.Discussion----------Synchronized Service alternative, backwards compatibleThis is a rebased version#7707.| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#7707| License | MIT| Doc PR |symfony/symfony-docs#2956Todo/Questions - [x] do we deprecate the synchronize feature (and removed it in 3.0)? - [x] deal with BC for listeners - [x] rename RequestContext as we already have a class with the same name in the Routing component? - [x] move RequestStack and RequestContext to HttpFoundation? - [x] update documentationPrototype for introducing a ``RequestContext`` in HttpKernel.This PR keeps the synchronized services feature, however introduces a ``RequestContext`` object additionally, that allows to avoid using synchronized service when injecting ``request_context`` instead of ``request`` into a service. The FrameworkBundle is modified such that it does not depend on synchronized services anymore. Users however can still depend on ``request``, activating the synchronized services feature.Features:* Introduced ``REQUEST_FINSHED`` (name is up for grabs) event to handle global state and environment cleanup that should not be done in ``RESPONSE``. Called in both exception or success case correctly* Changed listeners that were synchronized before to using ``onKernelRequestFinished`` and ``RequestContext`` to reset to the parent request (RouterListener + LocaleListener).* Changed ``FragmentHandler`` to use the RequestContext. Added some more tests for this class.* ``RequestStack`` is injected into the ``HttpKernel`` to handle the request finished event and push/pop the stack with requests.Todos:* RequestContext name clashes with Routing components RequestContext. Keep or make unique?* Name for Kernel Request Finished Event could be improved.Commits-------1b2ef74 [Security] made sure that the exception listener is always removed from the event dispatcher at the end of the requestb1a062d moved RequestStack to HttpFoundation and removed RequestContext93e60ea [HttpKernel] modified listeners to be BC with Symfony <2.4018b719 [HttpKernel] tweaked the codef9b10ba [HttpKernel] renamed the kernel finished eventa58a8a6 [HttpKernel] changed request_stack to a private servicec55f1ea added a RequestStack class
| * also when leaving a Request scope -- especially when they are | ||
| * nested). | ||
| * This method was used to synchronize the Request, but as the HttpKernel | ||
| * is doing that automatically now, you should never be called it directly. |
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.
Typo: This should read "* is doing that automatically now, you should never call it directly"
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.
fixed now, thanks.
| public function onKernelFinishRequest(FinishRequestEvent $event) | ||
| { | ||
| if (null === $this->requestStack) { | ||
| throw new \LogicException('You must pass a RequestStack.'); |
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.
this looks like a bc break. when a user uses this listener without passing a request stack, he will automatically get this exception because the event is subscribed automatically as well by the listener.
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.
This PR was merged into the master branch.Discussion----------[HttpKernel] made request stack feature BC| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#8904 see comments| License | MITCommits-------08a42e7 [HttpKernel] made request stack feature BC
…ion)This PR was merged into the 3.0-dev branch.Discussion----------[HttpKernel] make RequestStack parameter required| Q | A| ------------- | ---| Bug fix? | no| New feature? | no| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aContinuation of#14634,#8904Commits-------a2e154d [HttpKernel] make RequestStack parameter required for classes that need it
…(acrobat)This PR was merged into the 2.7 branch.Discussion----------Missing reference docs for kernel.finish_request event| Q | A| ------------- | ---| Doc fix? | yes| New docs? | yes - missing docs (PRsymfony/symfony#8904)| Applies to | Event was added in 2.4 so added to **2.7** docs| Fixed tickets |#6151Commits-------aad2b89 Missing reference docs for kernel.finish_request event
This is a rebased version#7707.
Todo/Questions
Prototype for introducing a
RequestContextin HttpKernel.This PR keeps the synchronized services feature, however introduces a
RequestContextobject additionally, that allows to avoid using synchronized service when injectingrequest_contextinstead ofrequestinto a service. The FrameworkBundle is modified such that it does not depend on synchronized services anymore. Users however can still depend onrequest, activating the synchronized services feature.Features:
REQUEST_FINSHED(name is up for grabs) event to handle global state and environment cleanup that should not be done inRESPONSE. Called in both exception or success case correctlyonKernelRequestFinishedandRequestContextto reset to the parent request (RouterListener + LocaleListener).FragmentHandlerto use the RequestContext. Added some more tests for this class.RequestStackis injected into theHttpKernelto handle the request finished event and push/pop the stack with requests.Todos: