Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

add SimpleStreamResponseSender + Tests #3343

Merged
merged 10 commits into from Jan 7, 2013

Conversation

Projects
None yet
2 participants
Contributor

prolic commented Jan 4, 2013

No description provided.

@weierophinney weierophinney commented on the diff Jan 4, 2013

...end/Mvc/ResponseSender/SimpleStreamResponseSender.php
@@ -0,0 +1,70 @@
+<?php
+
@weierophinney

weierophinney Jan 4, 2013

Owner

Needs file-level docblock

@weierophinney weierophinney commented on an outdated diff Jan 4, 2013

...end/Mvc/ResponseSender/SimpleStreamResponseSender.php
@@ -0,0 +1,70 @@
+<?php
+
+namespace Zend\Mvc\ResponseSender;
+
+use Zend\Http\Header\MultipleHeaderInterface;
+use Zend\Http\Response\Stream;
+
+class SimpleStreamResponseSender implements ResponseSenderInterface
@weierophinney

weierophinney Jan 4, 2013

Owner

Needs class-level docblock

@weierophinney weierophinney commented on an outdated diff Jan 4, 2013

...end/Mvc/ResponseSender/SimpleStreamResponseSender.php
+<?php
+
+namespace Zend\Mvc\ResponseSender;
+
+use Zend\Http\Header\MultipleHeaderInterface;
+use Zend\Http\Response\Stream;
+
+class SimpleStreamResponseSender implements ResponseSenderInterface
+{
+
+ /**
+ * Send headers
+ *
+ * @param SendResponseEvent $event
+ */
+ public function sendHeaders(SendResponseEvent $event)
@weierophinney

weierophinney Jan 4, 2013

Owner

I'm thinking we should probably have an AbstractResponseSender that contains this method, as it doesn't vary significantly between the implementations; right now, we're getting some code duplication.

Also, note that your implementation is missing checks that the PhpEnvironmentResponseSender has, making this one slightly less robust.

@weierophinney weierophinney commented on the diff Jan 4, 2013

library/Zend/Mvc/SendResponseListener.php
@@ -139,6 +140,7 @@ protected function attachDefaultListeners()
$events = $this->getEventManager();
$events->attach(SendResponseEvent::EVENT_SEND_RESPONSE, new PhpEnvironmentResponseSender(), -1000);
$events->attach(SendResponseEvent::EVENT_SEND_RESPONSE, new ConsoleResponseSender(), -2000);
+ $events->attach(SendResponseEvent::EVENT_SEND_RESPONSE, new SimpleStreamResponseSender(), -3000);
@weierophinney

weierophinney Jan 4, 2013

Owner

Shouldn't this be at the same priority as the PhpEnvironmentResponseSender? or possibly even higher priority?

@prolic

prolic Jan 4, 2013

Contributor

This is made special this way. Let me try to explain my thoughts about this:
Most response objects the framework will send, are PhpEnvironmentResponses, so this one has the highest priority. However the priority is low, so if you attach a custom response listener without priority, this one will be used first.
Second is the ConsoleResponseSender, which will obviously only be used for console responses, but I would expect having a couple of them in an zf app.
Last the (Simple) Sream response sender, which will (in most apps) not used, because not every application delivers stream responses to the end user. So as the stream response sender is not important in most apps, it has the lowest priority.
The reason why I chose different levels of priorities is, that you can attach a custom response sender with higher or lower priority respective to whatver base response sender is available.

@weierophinney

weierophinney Jan 4, 2013

Owner

That makes sense -- maybe detail that logic in the method docblock, for posterity.

Owner

weierophinney commented Jan 4, 2013

Implementation looks mostly reasonable -- address the code duplication, and add tests, and I'll review again for merge.

@ghost ghost assigned weierophinney Jan 4, 2013

prolic added some commits Jan 4, 2013

added abstract response sender
send Headers not uses "headers_sent()" directly
Contributor

prolic commented Jan 4, 2013

Todo:

  • add tests for all response senders
  • add tests for the send response listener
  • add priority description to method docblock
Owner

weierophinney commented Jan 4, 2013

I added the "[WIP]" notation to the PR title; edit and remove when it's ready. Thanks, @prolic !

Contributor

prolic commented Jan 4, 2013

Additional question: I am planning a more complex Stream Response sender, that supports download resume and limit download speed. That would need additional configuration (limit download speed (yes, no), download speed in kb/s, and enable / disable resume support). Is it best to bring this up as module so users can configure it how they want it to be, or should we provide such an implementation in zf2 directly? If so, how should it be configured?

Owner

weierophinney commented Jan 4, 2013

@prolic Hmm... considering that would likely work in conjunction with things such as Zend\ProgressBar, I'd argue a module may be better suited for that functionality.

prolic added some commits Jan 5, 2013

added get/setEvent method in SendResponseListener
added unit test for send response listener
optimized SendResponseEvent
added ConsoleResponseSenderTest
added SendResponseEventTest
added phpenrivonment response sender test
improved console response sender test
added additional test
trying to send the headers multiple times results in headers only being sent once
Contributor

prolic commented Jan 6, 2013

I created a repo for the upcoming complex stream response sender as module: https://github.com/prolic/HumusStreamResponseSender
currently there is no code, but it's coming very very soon!

This PR is ready for review.

Contributor

prolic commented Jan 6, 2013

Note: Travis build failed because of trailing spaces in Zend\Db, no issue about this PR.

@weierophinney weierophinney commented on the diff Jan 7, 2013

...ry/Zend/Mvc/ResponseSender/AbstractResponseSender.php
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2012 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Mvc
+ */
+
+namespace Zend\Mvc\ResponseSender;
+
+use Zend\Http\Header\MultipleHeaderInterface;
+
+/**
+ * @category Zend
+ * @package Zend_Mvc
+ * @subpackage ResponseSender
+ */
+abstract class AbstractResponseSender
@weierophinney

weierophinney Jan 7, 2013

Owner

Have this implement ResponseSenderInterface -- that way the specific implementations only need to extend this, and then add any missing methods.

weierophinney added a commit that referenced this pull request Jan 7, 2013

[#3343] CS and test fixes
- Minor CS fixes
- Moved code from conditionals into method body where possible
- Fixed AbstractResponseSenderTest to account for xdebug headers, if present

@weierophinney weierophinney merged commit 13e3c73 into zendframework:develop Jan 7, 2013

1 check failed

default The Travis build failed
Details
Owner

weierophinney commented Jan 7, 2013

Made requested change to AbstractResponseSender on merge.

@prolic prolic deleted the prolic:streamresponsesender branch Jan 7, 2013

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