- Download Diff

Summary:

[Network] add stream socket client class

Review Request #10 — Created Nov. 5, 2012 and discarded Jan. 7, 2013, 7:03 p.m.

Information
Owner:	olivier
Repository:	Lunr
Branch:	personal/olivier/streamclient
Bugs:	12249
Depends On:
Reviewers
Groups:	lunr
People:

Description

Add StreamSocketClient class extending StreamSocket

implements all abstract methods from StreamSocket

Testing Done

PHPUnit tests passed

Live test for push to APNS

Issues

1
0
8
0
All Issues: 9

Description	From	Last Updated
Not in Core	pprkut	Nov. 7, 2012, 2:59 p.m.
ditto	pprkut	Nov. 7, 2012, 2:59 p.m.
This needs to implement HttpRequestInterface	pprkut	Nov. 9, 2012, 6:33 p.m.
why would I be interested in this?	pprkut	Nov. 15, 2012, 6:36 p.m.
come again?	pprkut	Nov. 15, 2012, 6:58 p.m.
we don't need the line breaking here	pprkut	Nov. 15, 2012, 6:59 p.m.
Instead of checking whether this is a handle, you can just introduce a boolean and check whether that is TRUE ...	pprkut	Nov. 15, 2012, 7:27 p.m.
superfluous, look at parse_url()	pprkut	Nov. 15, 2012, 7:28 p.m.
I don't like this line, there's way too much going on	pprkut	Nov. 12, 2012, 5:57 p.m.

You currently require that open() and close() are called explicitely, while in other classes in Lunr we do it transparently.
If you do it transparently you can also do the is open/closed checks a bit easier.
For an example of what I'm talking about check the database connection classes.

system/libraries/network/class.streamsocketclient.inc.php (Diff revision 1)
Show all issues
```
Not in Core
```
system/libraries/network/class.streamsocketclient.inc.php (Diff revision 1)
Show all issues
```
ditto
```
system/libraries/network/class.streamsocketclient.inc.php (Diff revision 1)
Show all issues
```
This needs to implement HttpRequestInterface
```

Change Summary:

adjust summary

Summary:

-	[lunr] add stream socket client class
+	[Network] add stream socket client class

Change Summary:

Implements the HttpRequest interface in StreamSocketclient class

Live test made but no unit test written for the moment.

Need advice for:

1-Now the class handles http, maybe moving all the parameters of constructor to setters with fluid interface
would be relevant. In such case, while using http post or get, the remote socket value could be generated 
from the uri given as parameter to the methods involved

2-Despite the fact as far as I have seen, I put the response header data in an array and makes the key and values lower case
doesn't really matter but I had like you to be aware of that.

3-I forced the http header parameter to close (Connection: close) for both post and get requests.
This is probably no good design but the keep-alive value generates time out or other weird behaviours
probably because rejected by remote servers.

4-I intend to add a last method to the class: 

a method that would register a libevent event wrapper to the stream
It would consist in simply making the StreamSocketClient class provide its handle to the wrapping event class
This is better practice I think than just adding a get_handler method to the class.
this would involve the fact that extra classes should be created (event wrapper and base wrapper).

Enjoy!

Diff:

Revision 2 (+3403 -10)

Show changes

	system/libraries/network/class.streamsocket.inc.php
	system/libraries/network/class.streamsocketclient.inc.php
	tests/phpunit.xml
	tests/system/libraries/network/class.curl.base.test.php
	tests/system/libraries/network/class.curl.execute.test.php
	tests/system/libraries/network/class.curl.request.test.php
	tests/system/libraries/network/class.curl.setget.test.php
	tests/system/libraries/network/class.curl.test.php
	10 more

Change Summary:

Just fixed a potential bug in chunk support for transfer-encoding in parse_http_response_data.

For commenting please see remarks of previous diff.

Diff:

Revision 3 (+3403 -10)

Show changes

	system/libraries/network/class.streamsocket.inc.php
	system/libraries/network/class.streamsocketclient.inc.php
	tests/phpunit.xml
	tests/system/libraries/network/class.curl.base.test.php
	tests/system/libraries/network/class.curl.execute.test.php
	tests/system/libraries/network/class.curl.request.test.php
	tests/system/libraries/network/class.curl.setget.test.php
	tests/system/libraries/network/class.curl.test.php
	10 more

There's changes in this review that are already present in other review requests. Please rediff properly.

olivier Nov. 12, 2012, 10:40 a.m.
```
Oops sorry
```

Change Summary:

rediffed referring to the fact that parent review is stream socket.

For commenting please refer to diff r2 comments.

Diff:

Revision 4 (+1550 -5)

Show changes

	system/libraries/network/class.streamsocket.inc.php
	system/libraries/network/class.streamsocketclient.inc.php
	tests/phpunit.xml
	tests/system/libraries/network/class.streamsocket.base.test.php
	tests/system/libraries/network/class.streamsocket.state.test.php
	tests/system/libraries/network/class.streamsocket.test.php
	tests/system/libraries/network/class.streamsocketclient.base.test.php
	tests/system/libraries/network/class.streamsocketclient.flags.test.php
	3 more

Fix it!

system/libraries/network/class.streamsocketclient.inc.php (Diff revision 4)

Show all issues

why would I be interested in this?

olivier Nov. 13, 2012, 11 a.m.

right, only the http_response header is interesting

pprkut Nov. 13, 2012, 12:23 p.m.

I doubt I'm even interested in that. Why would I want to have access to low level http stuff. That's what I have the library for to not have to care

olivier Nov. 15, 2012, 6:32 p.m.

For this one I insist can be usefull for debug purpose

system/libraries/network/class.streamsocketclient.inc.php (Diff revision 4)

Show all issues

come again?

Felipe Nov. 13, 2012, 11:55 a.m.

Binary OR applied to flags array to construct the flags property... Is there an easier way to do  it??

pprkut Nov. 13, 2012, 12:23 p.m.

Ok, after reading the docs I can understand what you want to do. The foreach is fine, but

$flags = 0;

before is confusing. Use the proper default constant there (STREAM_CLIENT_CONNECT).

olivier Nov. 13, 2012, 1:29 p.m.

The flag you mention is added in the constructor of the class.

Initing to zero is made for allowing the possibility to remove that flag
using the method remove_flag.

pprkut Nov. 13, 2012, 8:27 p.m.

Right, I didn't see that. But that's confusing to the user. Why should I have to remove something I never added?
I would drop remove_flag(), detect whether flags are set or not. If not use STREAM_CLIENT_CONNECT, if they are use array_reduce() in combination with a lambda function (should be fairly small) to create the flags parameter.

olivier Nov. 14, 2012, 10:13 a.m.

Ok so, then the default flag will be added if no flag present, remove_flag method will be suppressed.

pprkut Nov. 14, 2012, 10:58 a.m.
```
yes
```

system/libraries/network/class.streamsocketclient.inc.php (Diff revision 4)

Show all issues

we don't need the line breaking here

olivier Nov. 13, 2012, 11 a.m.
```
will be removed then
```

Felipe Nov. 13, 2012, 11:55 a.m.

I like the line breaking in array generation, makes things more clear... and my auto formatter supports it! :P

pprkut Nov. 13, 2012, 12:23 p.m.

We can keep the line breaking, but then it needs to be proper:

$this->handle = stream_socket_client(
    $this->remote_socket,
    $this->errno,
    $this->errmsg,
    $this->init_timeout,
    $flags,
    $this->context
);

Felipe Nov. 13, 2012, 12:37 p.m.
```
Agree!
```
olivier Nov. 15, 2012, 7:27 p.m.
```
I'll do but I dont like it!
```

system/libraries/network/class.streamsocketclient.inc.php (Diff revision 4)

Show all issues

Instead of checking whether this is a handle, you can just introduce a boolean and check whether that is TRUE or FALSE

olivier Nov. 13, 2012, 11 a.m.
```
you mean  if($this->handle) ?
```

pprkut Nov. 13, 2012, 12:23 p.m.

I mean if ($this->open) or if ($this->connected) or if ($this->started) ....

olivier Nov. 13, 2012, 1:29 p.m.

Ok I get it now, that flag would be then set in the create handle method.
Cleaner design.

I propose to use the open name(started not relevant, connected depends on server side).

system/libraries/network/class.streamsocketclient.inc.php (Diff revision 4)

Show all issues

superfluous, look at parse_url()

olivier Nov. 13, 2012, 11 a.m.
```
Ok will be made so
```

system/libraries/network/class.streamsocketclient.inc.php (Diff revision 4)

Show all issues

I don't like this line, there's way too much going on

olivier Nov. 13, 2012, 11 a.m.

In fact I caught the function from the documentation,
it's a quick alternative to implementing PECL.
I will have a look for making the function more readable.

http://php.net/manual/fr/function.http-chunked-decode.php

Felipe Nov. 13, 2012, 11:55 a.m.

- Please try to do so
- Please don't link to french documentation :P

Status: Discarded

Change Summary:

replaced by http://reviews.lunr.nl/r/36/

You have a pending review.

Review Board 3.0.24

[Network] add stream socket client class

Change Summary:

Summary:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Discarded

Change Summary: