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

only set the master request on request_aware services #55

Closed
wants to merge 1 commit into from

Conversation

dbu
Copy link
Member

@dbu dbu commented Jun 19, 2013

Q A
Bug fix? yes
New feature? no
BC breaks? not sure
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

i had problems that my current menu item voter did not see the main request but got it overwritten with some subrequest. i found some code in the locale bundle - not 100% sure if this is all sane.

@@ -23,6 +23,10 @@ class RequestAwareListener implements EventSubscriberInterface

public function onKernelRequest(GetResponseEvent $event)
{
if ($event->getRequestType() !== HttpKernelInterface::MASTER_REQUEST) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@lunetics additionally checks for the request being xmlhttprequest - not sure why as i would expect an ajax request to be considered a master request too. https://github.com/lunetics/LocaleBundle/blob/master/EventListener/LocaleListener.php#L93

@lsmith77
Copy link
Member

uhm .. but why do you want the master request? as always its dangerous for caching to rely on the availability of a master request instance within a subrequest.

@dbu
Copy link
Member Author

dbu commented Jun 19, 2013

the problem i faced was that the sub request was overwriting the master request. when we return from the sub request we do not get the request event again. as the menu is built after a subrequest has been triggered and resolved, i had the wrong request in the current menu voter.

maybe we should cache the requests in this listener and listen to response as well and switch back to the parent request? that would probably be the clean thing to do. does that make sense?

@lsmith77
Copy link
Member

hmm .. for this use case we then need to use synchronized services which are only available in 2.3 :(

@dbu
Copy link
Member Author

dbu commented Jun 19, 2013

if you have time on thursday, i could come to the liip office a bit earlier to discuss. not sure what synchronized services is or why we would need them here.

@lsmith77
Copy link
Member

see symfony/symfony#7007

@lsmith77
Copy link
Member

Alright .. so the plan would be to merge this and use it for 2.2 only and switching all other use cases to synchronized services instead for 2.3

@dbu
Copy link
Member Author

dbu commented Jun 20, 2013

i wonder if i should do a small bundle that just provides symfony 2.2 synchronized services using a tag. the problem is that for our cmf bundles we want to support both 2.2 and 2.3 so we need a compiler pass that in symfony 2.3 makes the servcies synchronized and in 2.2 registers this listener and the services to that listener.

@dbu
Copy link
Member Author

dbu commented Jun 20, 2013

i created #56 to convert cmf_request_aware to synchronized services in sf 2.3 and properly stack requests in 2.2

@dbu dbu closed this Jun 20, 2013
@dbu dbu deleted the request-aware-master-request branch June 20, 2013 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants