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

Fix issue #49. Add tests. #58

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Fix issue #49. Add tests. #58

wants to merge 3 commits into from

Conversation

skybardpf
Copy link
Contributor

  • Include PhpUnit into project
  • Tests for Server\MethodWrapper
  • Constructor Server\Server taskes MethodWrapper or array methods.
  • Fix issue Method names conflict #49

@marcelklehr Your variant for solving the problem #49 the wrong. Example:

$methods = array(
  '__register' => function($params) {}
);
$server = new Server\Server($methods);
$server->input = '{"jsonrpc": "2.0", "method": "__register", "params": [], "id": 1}';
$server->dispatch(); // Call method "__register" of class MethodWrapper

@@ -71,22 +71,25 @@ class Server

/**
* Construct a Server object
* @param object $host An object whose methods will be provided for invokation
* @param MethodWrapper|array $host An object whose methods will be provided for invocation
Copy link
Owner

Choose a reason for hiding this comment

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

Originally the idea was that the server should be able to provide the methods of any object (e,g, it would be able to infer the parameter order via reflection etc.), MethodWrapper is just a utility to get things going quickly. Perhaps we should remove it entirely, instead of making it the only choice, because in the end, I think Classes are a better way to write an RPC API than arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is, to make the host an array? Format:

class Foo() 
{
  public function bar() {}
}

$foo = new Foo();

$methods = array(
  'method1' => function() {}, // anonymous function
  'method2' => array($foo, 'bar'), // Method of class
)

new Server($methods);

Declaration:

public function __construct(array $host)

Example:
https://github.com/marcelklehr/tivoka/pull/58/files/710ee26cd637d2df40c83eb2e16364a809e75654#diff-5994b67b081aa8d862f75ca5742fb841R23

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or I misunderstood? 😄

Copy link
Owner

Choose a reason for hiding this comment

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

No, I think it would be better to stop supporting passing hosted methods as an array:

class Foo() 
{
  public function bar() {}
}

$foo = new Foo();

-$methods = array(
-  'method1' => function() {}, // anonymous function
-  'method2' => array($foo, 'bar'), // Method of class
-)

-new Server($methods);
+new Server($foo)

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.

None yet

2 participants