-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
Replace request
listener with callback function
#97
Conversation
Really? Does it make sense to break the event pattern for this case specifically? Imho feels a little inconsistent. |
c0d740c
to
a79af2a
Compare
Yes it makes sense, because the event pattern itself makes no sense here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes perfect sense to replace the request
event with a single callback 👍
After all, it doesn't really make sense to add multiple listeners to this event in the first place (unlike low-level events on stream instances etc.). Also, big 👍 for using this a base to return a response object in #28.
Some minor notes below, otherwise LGTM 👍
src/Server.php
Outdated
try { | ||
$callback($request, $response); | ||
} catch (\Exception $ex) { | ||
$this->emit('error', array()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this makes sense here, can you give some usage examples? How could I possibly react to when my callback throws an exception? Can I trigger a clean shutdown or end the response etc.?
(It looks like this may be better off in a separate PR?)
tests/ServerTest.php
Outdated
$server->on('request', $this->expectCallableOnce()); | ||
$countCalled = 0; | ||
|
||
$callback = function ($request, $response) use (&$countCalled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff is unclear here, can you simplify this to keep the expectCallableOnce()
?
Updated. Checkout the newest commits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Moving this to the next major version to leave some time to discuss this further 👍 |
e09cb25
to
215c29b
Compare
README.md
Outdated
@@ -76,22 +75,18 @@ $socket = new SecureServer($socket, $loop, array( | |||
$http = new React\Http\Server($socket); | |||
``` | |||
|
|||
For each incoming connection, it emits a `request` event with the respective | |||
For each incoming connection, it executes the callback function with the respective | |||
[`Request`](#request) and [`Response`](#response) objects: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@legionth The code above does not seem to take care of this, can you update this? Also make sure the README is in line with the respective classes 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the missing parts. Also tried to make it inline with the Server
class. Have a look.
d0d122b
to
cbd602a
Compare
README.md
Outdated
@@ -51,16 +50,22 @@ See also the [examples](examples). | |||
### Server | |||
|
|||
The `Server` class is responsible for handling incoming connections and then | |||
emit a `request` event for each incoming HTTP request. | |||
execute the execute the callback function passed to the constructor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its responsibility is a bit unclear. How about just this:
…and then processing each incoming HTTP request. (or similar)
README.md
Outdated
as HTTP. | ||
|
||
For each incoming connection, it executes the callback function with the | ||
respective [`Request`](#request) and [`Response`](#response) objects: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which callback function? (see above) How about this:
For each incoming request, it executes the callback function passed to the constructor with…
README.md
Outdated
|
||
```php | ||
$socket = new React\Socket\Server(8080, $loop); | ||
|
||
$http = new React\Http\Server($socket); | ||
$http = new React\Http\Server($socket, function (Request $request, Response $response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines tend to get a bit long, how about using the local classname for all classes from this(!) package instead? (in line with Request
and Response
, see also below)
src/Server.php
Outdated
{ | ||
if (!is_callable($callback)) { | ||
throw new InvalidArgumentException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this tested? Also check the exception message.
cbd602a
to
6b12b58
Compare
6b12b58
to
19006b1
Compare
Resolved the Merge conflicts and added the requested changes Ping @clue |
Why doesn't it make sense? What if I have one callback to write to an access log and another to handle the request? |
@kelunik I think this is a very valid use case and is in fact something we do care about. Using multiple listeners in this case isn't really reliable, because only one can (reasonably) write the response anyway. In the future, we will promote and/or support middlewares for your approach that allow a more reliable way to also get access to response parameters (such as response code for logging etc.). I hope this helps 👍 |
A callback function is more useful and helps to understand better how the server works.
The
request
event isn't useful, because there is no real scenario where you need different listeners on this event.Btw.: this would make it easier to implement PSR-7 too ;)
Refs #28