RFC Send Response Workflow #3105

Merged
merged 9 commits into from Dec 18, 2012

Conversation

Projects
None yet
3 participants
@@ -30,16 +30,6 @@ class Response extends HttpResponse
@weierophinney

weierophinney Dec 3, 2012

Owner

The changes in this file are backwards incompatible and cannot be made in ZF2.

+ /**
+ * @return bool
+ */
+ public function headersSent()
@weierophinney

weierophinney Dec 3, 2012

Owner

This would be better to remain in Zend\Http\PhpEnvironment\Response, as that class is meant to be an encapsulation of the current HTTP response environment within PHP -- in other words, that class should be checking for whether or not headers are sent already, etc. The response sender would simply check that information in the response object.

+ public function sendHeaders()
+ {
+ if ($this->headersSent()) {
+ return $this;
@weierophinney

weierophinney Dec 3, 2012

Owner

I'm wondering if we shouldn't trigger an event in this situation, as we potentially might end up omitting some headers if we detect headers have already been sent. A separate flag could track whether this class has sent them, vs. whether the response reports having sent them (via headers_sent).

@weierophinney

weierophinney Dec 3, 2012

Owner

Also, this implementation is naive, as it does not check headers_sent(). While I advocate moving that check back to the response object, nevertheless, we need to check it somewhere, and this class does not check it all.

@prolic

prolic Dec 3, 2012

Contributor

@weierophinney how do you think we should handle this? After calling "header($someHeaderString)" there are still no headers sent and "headers_sent()" returns false. Headers are sent, when content gets sent, too. When we change the implementation to use "headers_sent()", it would no longer mean, that "header($someHeaderString)" was called.

}
+
+ $responseSender = $this->getServiceLocator()->get($this->options[$responseType]);
@weierophinney

weierophinney Dec 3, 2012

Owner

Why make this class service locator aware? If it is to have a factory in the ServiceManager, why not simply inject the response object instead of pulling it from the SM?

Owner

weierophinney commented Dec 3, 2012

I've also left comments on the RFC, @prolic

updated send response workflow
- implemented changes proposed by weierophinney
- reverted changes in Console\Response and PhpEnvironment\Response objects
- added Zend\Mvc\View\SendResponseListener extending the new class
- added ResponseListenerFactory
- Response sender triggers events
Contributor

prolic commented Dec 3, 2012

This also fixes a so far unknown issue: If your controller returns a custom response object (e.g. return new StreamResponse), this response object was not set in Zend\Mvc\Application and also not updated in Zend\ServiceManager\ServiceManager. When you call $serviceManager->get('Response') after returning a custom response in controller, you will now get the correct response object. (see changes in Zend\Mvc\Application)

+ return $this;
+ }
+ echo $response->getContent();
+ $response->setContentSent(true);
@weierophinney

weierophinney Dec 3, 2012

Owner

I wouldn't track this in the response. I'd instead do a hash map of splobjectid => flag -- you can then check the given $response object against that to see if it has been sent.

+ }
+
+ echo $response->getContent();
+ $response->setContentSent(true);
@weierophinney

weierophinney Dec 3, 2012

Owner

I wouldn't track this in the response. I'd instead do a hash map of splobjectid => flag -- you can then check the given $response object against that to see if it has been sent.

+ * @param MvcEvent $e
+ * @return void
+ */
+ public function injectResponseSender(MvcEvent $e)
@weierophinney

weierophinney Dec 3, 2012

Owner

This method does not make sense to me. Why not simply register the ResponseSender, trigger it, and if no response is present, return early? Or is there some advantage to registering late?

@prolic

prolic Dec 3, 2012

Contributor

When the SendResponseListener gets instantiated, the response object given could change, e.g. the controller returns a newly created response object. The concrete response sender is selected, based on the classname of the current response object. If you return a custom response in your controller, the send response listener would not be able to send the correct response.
Alternative implementation would be: make this class ServiceLocatorAware!

@weierophinney

weierophinney Dec 3, 2012

Owner

The MvcEvent has the current response object -- if the controller returned one, then that's what will be in the Event at this point. As such, instead of setting the Response as a property of the ResponseSender, you'd simply pass it as an argument to the ResponseSender's sendResponse() method.

I guess my point is: shouldn't a typical ResponseSender implementation be able to handle any response type, or at least raise an error if it cannot? And if you will be using a custom type, you could potentially inject the SendResponseListener manually with an approriate ResponseSender; we could even create a controller plugin and/or application listeners around this.

library/Zend/Mvc/Application.php
+ $serviceManager = $this->getServiceManager();
+ $serviceManager->setAllowOverride(true);
+ $serviceManager->setService('Response', $response);
+ $serviceManager->setAllowOverride(false);
@weierophinney

weierophinney Dec 3, 2012

Owner

Why are the above lines necessary?

@prolic

prolic Dec 3, 2012

Contributor

If the controller returns a custom response object (e.g. return new StreamResponse), the wrong (old one) response object is registered in Zend\Mvc\Application and Zend\ServiceManager\ServiceManager.
When I try to create the correct response sender object, it would get the wrong response object injected.

@weierophinney

weierophinney Dec 3, 2012

Owner

How?

The point is that the response composed in the event is what should be sent, not the one in the application object.

@weierophinney

weierophinney Dec 3, 2012

Owner

In fact, a number of PRs were merged for 2.0.4 for precisely that reason: to ensure that the Response object composed in the MvcEvent object is the one returned by a listener. That object is the correct one to introspect at any given time, not the one in the service manager or the application instance. (I'd argue that we likely should not register the Request/Response objects in those classes.)

+ {
+ $this->contentSent = (bool) $flag;
+ return $this;
+ }
@weierophinney

weierophinney Dec 3, 2012

Owner

Per previous comments, track this in the SendResponseListener.

@prolic

prolic Dec 3, 2012

Contributor

Really in SendResponseListener and not in ResponseSender?

@weierophinney

weierophinney Dec 3, 2012

Owner

Sorry, meant the ResponseSender itself.

library/Zend/Console/Response.php
+ {
+ $this->contentSent = (bool) $flag;
+ return $this;
+ }
@weierophinney

weierophinney Dec 3, 2012

Owner

Per previous comments, track this in the SendResponse listener.

Contributor

prolic commented Dec 3, 2012

About headers_sent():

@weierophinney how do you think we should handle this? After calling "header($someHeaderString)" there are still no headers sent and "headers_sent()" returns false. Headers are sent, when content gets sent, too. When we change the implementation to use "headers_sent()", it would no longer mean, that "header($someHeaderString)" was called.

Contributor

prolic commented Dec 3, 2012

RFC document at http://framework.zend.com/wiki/display/ZFDEV2/RFC+-+Send+response+workflow needs to be updated. However the open questions remain:

  • How and should we use headers_sent() ?
  • Should we also be able to send Zend\Http\Response or only support sending of Zend\Http\PhpEnvironment\Response ?
Owner

weierophinney commented Dec 3, 2012

@prolic re: headers_sent() -- I'd use a two-step approach:

  1. First, check the status of headers_sent()
  2. Second, check if we've sent the headers from this specific Response object before (this is the bit where I recommended keeping a hash map of the SplObject ID/sent status in a previous comment).

Basically, headers_sent() only returns true if we have also started sending content. So the first check is for that -- because you'll get a PHP notice if that's the case, and we don't want that. The second check is to ensure that if the same object is passed to the routine again, we don't try and send the headers a second time.

+ */
+ public function sendContent()
+ {
+ $this->getEventManager()->trigger(self::EVENT_SEND_CONTENT, $this);
@weierophinney

weierophinney Dec 3, 2012

Owner

Why is this needed, exactly?

+ */
+ public function sendResponse()
+ {
+ $this->getEventManager()->trigger(self::EVENT_SEND_RESPONSE, $this);
@weierophinney

weierophinney Dec 3, 2012

Owner

What use cases do you have for this event?

@prolic

prolic Dec 3, 2012

Contributor

Why should we then even trigger the SEND_HEADERS event? We can also register a listener on MVCs EVENT_FINISH and forget about SEND_HEADERS event. You can also remove there unneeded/ unwished headers.

Owner

weierophinney commented Dec 3, 2012

@prolic So, only one question really remains in your list: if we should allow sending alternate Response types. My answer is yes. There are a couple ways to manage this:

  1. We have the ResponseSender delegate to different methods based on Response type
  2. We use an event, and allow attaching to that event. This way, we can provide some standard listeners for known Response types -- Console, PhpEnvironment, Stream -- and the first one to report having sent the response causes the event to stop propagation.

The first is easier and faster to implement, but less flexible. The second will take more time, and require more objects, but be completely flexible.

Contributor

prolic commented Dec 3, 2012

@weierophinney thanks for your input. I will update this PR and the RFC document tomorrow.

refactored send response workflow
- added SendResponseEvent
- ResponseSender listen for an event
- first response sender that can send the response, stops propagation
- send response listener just triggers event, sends no response
- SendResponseEvent tracks which response headers and content were sent
- send response listener attaches default listeners (phpenvironmentresponse and consoleresponse, stream response sender is in progress)
- removed AbstractResponseSender
Contributor

prolic commented Dec 7, 2012

updated worflow, document still out of date, see last commit message

Should be EVENT_SEND_RESPONSE to be consistent with the MvcEvent::EVENT_ constants.

added phpdoc
added default name in SendResponseEvent
removed unused factories
Contributor

prolic commented Dec 9, 2012

todo: add stream response sender, write tests

Owner

weierophinney commented Dec 13, 2012

@prolic Let me know when this is ready to review again.

@ghost ghost assigned weierophinney Dec 13, 2012

Contributor

prolic commented Dec 16, 2012

I will add the stream response sender in a separate PR, as there is probably more to discuss. All tests are running again. Ready for review.

@weierophinney weierophinney merged commit a18deed into zendframework:develop Dec 18, 2012

cgmartin pushed a commit to cgmartin/zf2 that referenced this pull request Dec 18, 2012

weierophinney added a commit to zendframework/zend-http that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-console that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment