Skip to content
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

Add support for JSON-RPC 1.x and tweak some error messages #6

Merged
merged 14 commits into from
Feb 21, 2014
Merged

Add support for JSON-RPC 1.x and tweak some error messages #6

merged 14 commits into from
Feb 21, 2014

Conversation

scottchiefbaker
Copy link
Collaborator

No description provided.

// Check the validity of the request accoarding to RFC
if(!isset($request->jsonrpc) || $request->jsonrpc !== '2.0') {
// Check that it's a valid 1.0 or 2.0 request
if(!$this->request_version) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fail for valid 1.0 requests as those do not contain the jsonrpc attribute: http://json-rpc.org/wiki/specification#a1.1Requestmethodinvocation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This checks for the version of the request. set_version() checks for a couple of things and stores whether it's a 1.1 request or a 2.0 request and stores that number in $this->request_version.

Version 1.1 requests (in the libaries I've seen) send a "version" string instead of JSONRPC.

@scottchiefbaker
Copy link
Collaborator Author

I'm closing this pull request for now. I've made some substantial changes to the code, so the pull request is a pretty massive change at this point.

I'd be happy to discuss the changes I made and collaborate in the future if you're interested.

Provide method for setting the level of error reporting
@foglcz foglcz reopened this Feb 14, 2014
@foglcz
Copy link
Owner

foglcz commented Feb 14, 2014

HI, sorry, we've been finishing a project these past two weeks and there was zero time to respond to anything..

I've reopened the issue & granted you write permissions to manage the sourcecode if you're interested.

I will continue with merging the pull request tommorow + doing some tweaks if need be. Overall however, since you're using the library actively (I'm not at this very moment), it would be best just to update the repo directly if you're missing a function.

Thanks!

@scottchiefbaker
Copy link
Collaborator Author

On 02/14/2014 08:25 AM, Pavel Ptacek wrote:

HI, sorry, we've been finishing a project these past two weeks and there
was zero time to respond to anything..

I've reopened the issue & granted you write permissions to manage the
sourcecode if you're interested.

I will continue with merging the pull request tommorow + doing some
tweaks if need be. Overall however, since you're using the library
actively (I'm not at this very moment), it would be best just to update
the repo directly if you're missing a function.

I made some pretty substantial changes to the code. I didn't want to
step on your toes. Have you had a chance to look at what I did? I want
to respect your original work.

  • Scott

@scottchiefbaker
Copy link
Collaborator Author

I was going to push all my changes in to your repo this morning but I got a huge merge conflict. All my code is tab indented, and yours is space indented. Should I convert it back to spaces before I push?

@foglcz
Copy link
Owner

foglcz commented Feb 21, 2014

Hi,
resolve it using yours, leave the tabs option (i changed my IDE settings,
so everything should be fine :))

Thanks,
Pavel

On Thu, Feb 20, 2014 at 5:09 PM, Scott Baker notifications@github.comwrote:

I was going to push all my changes in to your repo this morning but I got
a huge merge conflict. All my code is tab indented, and yours is space
indented. Should I convert it back to spaces before I push?


Reply to this email directly or view it on GitHubhttps://github.com//pull/6#issuecomment-35637621
.

@scottchiefbaker scottchiefbaker merged commit e00f24a into foglcz:master Feb 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants