marcelklehr / tivoka Goto Github PK

When creating the headers passed to curl_setopt($ch, CURLOPT_HTTPHEADER, $headers), new lines are appended to each header value. This was introduced in commit 2caa759 in Http.php on line 96 when building support for curl requests:

foreach($this->headers as $label => $value) {
  $headers[] = $label . ": " . $value . "\r\n";
}

According to the curl_setopt() documentation for option CURLOPT_HTTPHEADER at http://php.net/manual/en/function.curl-setopt.php, the array values should not include the trailing new line.

The effect is that the http body is prepended with \r\n which modifies the body content, which can be fatal and very hard to find when debugging.

Removing the "\r\n" should do it and has been confirmed by our own testing. The correct line should look like:

$headers[] = $label . ": " . $value;

The last version on pearhub (http://pearhub.org/projects/tivoka) is 3.0

Something broke on building 3.0.1 see:
http://pearhub.org/projects/tivoka/releases

This will require the package to be PSR-0-compliant. And this in turn will break compatibility with php < 5.3

Although the currrent API, introduced in 2.0.0b, was intended to be intuitive and simple/short to write and read, it failed at least one of these.

Proposal:
Make a Tivoka Class (I think there already is one) containing factory methods for all classes.

Tivoka::notification($method, $params);
Tivoka::request($method, $params);

Also Request->send should be renamed to

Tivoka::request($method, $params)->sendTo('http://example.com/json-rpc');

I get this warning for some methods called via Client::sendRequest():

Notice: Undefined index: label in [...]/tivoka/lib/Tivoka/Client/Request.php on line 99
Notice: Undefined index: body in [...]/tivoka/lib/Tivoka/Client/Request.php on line 99

The code removes the first line of the $headers parameter and assumes the rest will match. However, the offending input looks like this:

array (size=7)
  0 => string 'HTTP/1.1 100 Continue' (length=21)
  1 => string 'HTTP/1.1 200 OK' (length=15)
  2 => string 'Date: Mon, 21 Nov 2016 11:44:46 GMT' (length=35)
  3 => string 'Server: Apache/2.4.10 (Debian)' (length=30)
  4 => string 'Content-Length: 144' (length=19)
  5 => string 'Connection: close' (length=17)
  6 => string 'Content-Type: application/json' (length=30)

The code needs a better detection of status lines to handle the case of the 100/200 status.

For a library it's quite cumbersome to be GPL-licensed. I belive, it'd be better to license it under terms of MIT license.

The native client interface is cool, but only allows positional arguments in the method implementation. I prefer named parameters (more flexible and readable) which are not supported by the native client interface.

To get the best of both worlds, it would be nice to have a switch that would let __call() interpret only 1 argument which would be an array with all the parameters.

Hi there!
I met a problem a little time ago: while Request executes json_encode function and get an error, there is no ability get to know what kind of error it is. I wasted too much time to understand what sort of error was on my production environment.
Hope, it's a helpful improvement, as it's for me.

Hi, I'd like to integrate tivoka into the FreeBSD ports tree, but I'd need you to provide a downloadable package with a pre-determinable checksum (which is not the case with tarballs created automatically by github). Would you mind to provide such a package for your official releases?

When working on #35 I encountered several problems with passing encoder instance around. I found it quite difficult to track all the cases.

I think the structure of main request/response handling classes should be refactored to better separate logic of certain layers:

• request and response classes should not contain any transport-related logic, they should act just as a containers and possibly contain basic processing routines (error response handling, JSON-RPC error codes maybe etc)
• server handler should not call die() after sending response
• connection handler should handle serialization of data and sending them

As an example of mess we have now just look at BatchRequest::getRequest() - it serializes data, that is product of un-serialization of previously serialized single requests.

I guess this could be done as start of 4.x branch since it will result in huge rewrite :).

• encourage people to share feedback
• strip out license text (just write license name)
• Download link to the top
• add link to json-rpc spec
• add some teaser paragraph

There must be a way to set a preference for stdClass(es) instead of assoc arrays, just like in PHP json_decode function, which actually defaults to stdClass.

My procedures return array of objects but instead I get array of assoc arrays, which, actually, is a show-stopper for my particular application.

This would be very cool, and leaves only by-order calls with a $params array.

http://de.php.net/manual/de/class.reflectionfunction.php

fsockopen($this->host, $this->port, $errno, $errstr, $this->timeout);

stream_set_timeout($this->socket, $this->timeout);

fsockopen timeout in seconds, stream_set_timeout in micro seconds

Somehow json-rpc version handling has to be implemented.
I'm not sure how to do this, yet.

<?php
$result = $request->params[0] - $request->params[1]
$request->returnResult($result);
return TRUE;
?>

This is mad enough without the return TRUE statement. Why not make returnResult return TRUE (and returnError, return FALSE)?

<?php
function(r) {
  $result = $request->params[0] - $request->params[1];
  return $r->returnResult($result);
}
?>

...consistently!
(I somehow have to figure out, how to convert the existing files...)

My initial PR for cURL support set CURLOPT_FOLLOWLOCATION. This is a leftover from testing and should be removed as it may cause various problems:

1. It generates a warning if open_basedir is set, eventually garbling client output.
2. It does not work as expected anyway: The redirection is always done as a GET request, making this mechanism unsuitable for JSON-RPC requests.

In this case, the server sets the original request as the error message of the response, which makes it invalid, since errmsg mustbe a string...

Currently the validation part looks like this

 if(isset($assoc['jsonrpc'], $assoc['result'], $assoc['id']) === FALSE) return FALSE

...which is not very obvious. This issue is a personal reminder for me to reimplement this like so:

    if(!isset($assoc[jsonrpc])) return false
    // ...

(i'm aware this no valid php...)

How to fix it when trying to get data from javascript

if you connect via tcp and sending notification client will be waiting for response - is WRONG!

in file "lib/Tivoka/Client/Connection/Tcp.php" before read response (line 139) you must write this code:
if(!$request->id) { return false; }

After that fix - your connection will not waiting response on notification

Something like a native php object, fetching all method calls with __call() and sending them to the server returning the result.

People are going to build this anyway, so why not provide it?

Don't repeat license statement in each file!
Make shure every method and every property is documented!

http://www.jsonrpc.org/specification

JSON-RPC 2.0 Specification

4.2 Parameter Structures
....
by-name: params MUST be an Object, with member names that match the Server expected parameter names. The absence of expected names MAY result in an error being generated. The names MUST match exactly, including case, to the method's expected parameters.

$rpcConnection = Tivoka\Client::connect('url');
$rpcConnection->sendRequest('init', array('param1' => 'value1', 'param2' => 'value2'));

correct work with server side function init($param1, $param2)
but don't work with function init($param2, $param1)

Please fix it! Thanks.

For what reason fo you need to specifically set the id of a request?
It's not really interesting to the server, therefore it's unnessecery to have the user set up some id value everytime they want to send a request.

-1 : This would bump the project to v3.0.0, bco major API changes...

It's ugly to have only one Tivoka_Eception class with varying error codes. (This prevents people from catching specificly!)

Currently I see that the code expects the server to use UTF-8 (based on the json_encode and json_decode calls. Please make the server code have an option to set an alternative encoding. You can then either leave the default as UTF-8 or guess it from ini_get('default_charset'). Perhaps making protected json_encode and json_decode methods is a good idea so that users can customize the encoding/decoding by overriding them.

Below are some of my alternative json encode/decode functions that use iconv_mixed to help you. You can of course set the default to a different value.

Thanks.

/**
* The same as json_decode() but allows the specification of the character encoding of the result.
*
* @param string $json
* @param boolean $assoc, optional, default = false
* @param string $charset optional, default = 'cp1252'
* @return object|array|null
*/
function json_dec($json, $assoc = false, $charset = 'cp1252') {
    if (!$charset || (strtoupper($charset) == 'UTF-8')) {
        return json_decode($value, $assoc);
    }
    else {
        return iconv_mixed('UTF-8', $charset, json_decode($json, $assoc), true);
    }
}


/**
* The same as json_encode() but allows the specification of the character encoding of $value.
*
* @param mixed $value
* @param string $charset optional, default = 'cp1252'
* @return string
*/
function json_enc($value, $charset = 'cp1252') {
    if (!$charset || (strtoupper($charset) == 'UTF-8')) {
        return json_encode($value);
    }
    else {
        $value = iconv_mixed($charset, 'UTF-8', $value, true);
        return json_encode($value);
    }
}


/**
* Similar to iconv(), but works on (nested) arrays and objects as well.
*
* @param string $in_charset
* @param string $out_charset
* @param mixed $data
* @return mixed
*/
function iconv_mixed($in_charset, $out_charset, $data, $encode_keys=false) {
    if (empty($data)) {
        return $data;
    }
    $result = null;
    if (is_string($data)) {
        $result = iconv($in_charset, $out_charset, $data);
    }
    elseif (is_array($data)) {
        $result = array();
        foreach($data as $k => $v) {
            if ($encode_keys) {
                $k = iconv($in_charset, $out_charset, $k);
                if ($k === false) {
                    return false;
                }
            }
            if (is_scalar($v) && !is_string($v)) {
                $result[$k] = $v; // because $v can be false and that's not an error.
            }
            else {
                $v = iconv_mixed($in_charset, $out_charset, $v, $encode_keys);
                if ($v === false) {
                    return false;
                }
                $result[$k] = $v;
            }
        }
    }
    elseif (is_object($data)) {
        $vars = get_object_vars($data); // public variables.
        $result = $encode_keys && (get_class($data) == 'stdClass') ? new stdClass() : $data;
        foreach($vars as $k => $v) {
            if ($encode_keys) {
                $k = iconv($in_charset, $out_charset, $k);
                if ($k === false) {
                    return false;
                }
            }
            if (is_scalar($v) && !is_string($v)) {
                $result->$k = $v; // because $v can be false and that's not an error.
            }
            else {
                $v = iconv_mixed($in_charset, $out_charset, $v, $encode_keys);
                if ($v === false) {
                    return false;
                }
                $result->$k = $v;
            }
        }
    }
    else {
        $result = $data;
    }
    return $result;
}

Abstract the transport part on client side, so different protocols can be used (which was the original intention of the spec!).

I can't use this package as is, because my server requires a User-Agent header and returns "403 Forbidden" if it's not set. It would be nice if this could be done: $request->connection()->setRequestHeader('User-Agent', 'Foo/1.0'); or $request->setHeader('User-Agent', 'Foo/1.0');

... because the client expects a batch response, but the server replies directly to the one request it received (notifications don't trigger a response...)

Think of some way to allow the server to handle both v1.0 and v2.0 specs

_
... at once! (I think that was my intention, when submitting this)

I created a method called 'register' and it was not working properly probably because of the conflict with system method that registers user-defined methods.

Conflict case:
$methods = array(
'register' => function($params) {
list($email, $username, $pass) = $params;
return $email;
}
)

After renaming the method to 'user_register', it started to work.