Skip to content
This repository was archived by the owner on Jan 8, 2020. It is now read-only.

RFC Send Response Workflow#3105

Merged
weierophinney merged 9 commits into
zendframework:developfrom
prolic:sendresponse
Dec 18, 2012
Merged

RFC Send Response Workflow#3105
weierophinney merged 9 commits into
zendframework:developfrom
prolic:sendresponse

Conversation

@prolic

@prolic prolic commented Nov 29, 2012

Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@weierophinney

Copy link
Copy Markdown
Member

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

- 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
@prolic

prolic commented Dec 3, 2012

Copy link
Copy Markdown
Contributor Author

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@prolic

prolic commented Dec 3, 2012

Copy link
Copy Markdown
Contributor Author

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.

@prolic

prolic commented Dec 3, 2012

Copy link
Copy Markdown
Contributor Author

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 ?

@weierophinney

Copy link
Copy Markdown
Member

@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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this needed, exactly?

@weierophinney

Copy link
Copy Markdown
Member

@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.

@prolic

prolic commented Dec 3, 2012

Copy link
Copy Markdown
Contributor Author

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

- 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
@prolic

prolic commented Dec 7, 2012

Copy link
Copy Markdown
Contributor Author

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

added default name in SendResponseEvent
removed unused factories
@prolic

prolic commented Dec 9, 2012

Copy link
Copy Markdown
Contributor Author

todo: add stream response sender, write tests

@weierophinney

Copy link
Copy Markdown
Member

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

@ghost ghost assigned weierophinney Dec 13, 2012
@prolic

prolic commented Dec 16, 2012

Copy link
Copy Markdown
Contributor Author

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants