add cmf_request_aware tag #37
Conversation
|
i think this leads to the services being instantiated on each request. unless there is some event on service instantiation that we can listen to, i see no way we can avoid that however. maybe we need to document to only use this tag for basic things that are needed in most requests anyways. otherwise one can always revert to inject the container and get the request from it... |
| } | ||
|
|
||
| foreach ($this->services as $service) { | ||
| if (method_exists($service, 'setRequest')) { |
There was a problem hiding this comment.
i dont think that we need this check
There was a problem hiding this comment.
copied like this from https://github.com/KnpLabs/KnpMenuBundle/blob/master/EventListener/VoterInitializerListener.php - the problem is if its not there, we get a fatal error. but chances are we get a fatal error anyways later when the service tries to access a request that is null. will remove it.
|
yes .. this listener only makes sense for things that are needed in every (sub)request anyway |
|
|
||
| public function onKernelRequest(GetResponseEvent $event) | ||
| { | ||
| if (HttpKernelInterface::MASTER_REQUEST !== $event->getRequestType()) { |
There was a problem hiding this comment.
don't we also need to do this for subrequests?
There was a problem hiding this comment.
@stof does it like this in https://github.com/KnpLabs/KnpMenuBundle/blob/master/EventListener/VoterInitializerListener.php - i am not sure what makes sense. our listeners so far want to look into the mainContent of the request - i guess that is not there on sub requests?
There was a problem hiding this comment.
well the key question is if this affects any services that run in a subrequest .. aren't menu's rendered in subrequests?
There was a problem hiding this comment.
no they are rendered with a twig function. but one way to work around performance issues would be to make that piece an ESI block to cache it, so its definitely a scenario. but then i don't know how it would work in a sub request. not sure if its even possible to have the master request and a sub request in the same context or if a new one is started anyways.
we could just drop the check here and say if a service only wants the master request, it could make its setter not overwrite the request if its already set. i guess its safe to assume the master request is always the first one...
There was a problem hiding this comment.
hmm but we cannot rely on the menu to be rendered in the master request .. but yeah running this for every subrequest might be too much .. :(
|
at least the sandbox still works when removing the check if its the master request, so i guess its good for now. |
|
maybe this could be a use case for the super lazy service instantiation thing added in symfony 2.3... but lets optimize later. |
|
I want to say "tests" here, should this stuff really be exempt because 3 classes are DI and the other one is so simple? |
|
imo unit tests make no sense here, but functional tests would make a lot of sense for this. |
|
From my pov unit tests always make sense because there part of my development process, I write the test as I write the code to ensure that the code works, often without testing in the browser at all. If I were to work on this listener, I would write a test for it to prove that it worked. Having tests for all classes also ensures that people who contribute to the same classes in the future also have to expand the existing test, proving that their change works too. It also helps to focus on architecture and to generally be more mindful of the code. Sorry if that sounds like a rant, its not, I'm just +1 for testing all classes in general. Its debatable in this case I know. |
|
you take TDD seriously i see. i will try to find some time to do this. |
|
Not sure if I do TDD, rather test aided development :) Don't go out of your way to do it on my account, as you say we will go over all the bundles before 1.0 anyway. |
once this is good, we can fix #35