search

ghedipunk / PHP-Websockets

A Websockets server written in PHP.
BSD 3-Clause "New" or "Revised" License
913 stars 375 forks source link

Major revision #33

Closed Xaraknid closed 9 years ago

Xaraknid commented 9 years ago

WARNING : This revision use 'trait' it's now required php >= 5.4. Those who still use php < 5.4 should continue using the former version of php-websocket. I think before merging it you should save the current master into another branch for those still using php < 5.4.

The list of modification in this revision and future tought :

    Server          $headerProtocolRequired   client                result
    ws_withoutprotocol  false               clientwithoutprotocol       Will work
    ws_withoutprotocol  false               clientwithprotocol          Client will close the connection
    ws_withprotocol     false               clientwithoutprotocol       Will work
    ws_withprotocol     false               clientwithprotocol          Will work
    ws_withprotocol     true                clientwithoutprotocol       Server will close the connection
    ws_withprotocol     true                clientwithprotocol          Will work
ghedipunk commented 9 years ago

Merged into the Development branch without testing.

ghedipunk commented 9 years ago

A few thoughts right away, on my first look:

Rather than using an underscore in the autoload to specify the path to the file, I'd rather use namespaces. This way, the actual class name stays short, and if we set up an autoloader properly, it can co-exist with other autoloading methods out there like the short-sighted PSR-0 scheme.

Second, I'm trying to move away from having the user logic extend the core server. Instead, I plan on having the core server be a stand alone object created by the application object. This will isolate classes to allow unit testing, prevent naive overwriting of properties or methods already within the core server, and help the system become more modular and easier to maintain. Thus, rather than extending an abstract class, we'll be enforcing the availability of certain methods through implementing interfaces.

Great work in the right direction. I love the traits. I'm going to use this as the basis of continuing forward.

Xaraknid commented 9 years ago

Just let you know google search you are third behind a tutorial where you are first within :). So you are the second choice, continue the good work.

This aside : Yeah indeed using namespace should be great.

Personally I really love the way it is now. For me it's by far the easier one to use. If you worried that some internal method can be naively overwrited we can simply use FINAL on that method preventing naive overwriting as when the user run the program it will crash anyway so he could use another name for is function and if he indeed want to overwrite a function he could by removing FINAL but that way he do it in purpose.

Also I think extending the core server is a great way. That way only the usefull method for user logic are used. Keep code clean for user without all behind the scene code.

I'm not sure interface is usefull for your project, seem to me a layer of complexity as it only purpose is to declaration of function to be implemented in another class.

For me why i choose this project over ratchet and wrench is I did not need 3 hours to retrace one simple function because it's delocalized, is in multiple place an extend of an extend over multiple folder where the whole simple one function is a meshed up of multiple little function all in different folder.

Anyway I'm not sure where you want to go. The only advice I will give you is keep it simple and do not end up like another ratchet or wrench for me it's strong point is it's simplicity.

Continue your good work !!!

Xaraknid commented 9 years ago

A little question : what prevent you to implement unit test with the current design ?

ghedipunk commented 9 years ago

I'm actually looking into what it would take to start up some unit testing right now. The main pain point with unit testing is that, if using a test framework like PHPUnit, you can't make a mock-up class of all of the functionality if you're extending a base class rather than using a separation of concerns system such a dependency injection... It would be testing with a full featured server running, which may make the tests less than reliable.

After reading your comments, I've decided that it's time for me to start up a new project that uses Websockets again. (The last project I used it for was a work project for a company that I'm no longer with.) I need to stay grounded with what the pain points are in this project.

Xaraknid commented 9 years ago

I actually can't help you with PHPunit i'm clueless with that :). But I found someone in same trouble as you http://stackoverflow.com/questions/18595755/phpunit-and-extended-classes if that can help you in any way.

The reasonning why I like this design I mean by that : application method being upfront ( onmessage,onclose,onopen ). and core application behind the scene.

Is that only usefull method for dev are easily available within testwebsocket.php.

Unfortunately websocket server are an active library by that I mean unlike any other library you can use like DB, cache, etc. These are passive library they do nothing by their own they wai passively that you call them for a function.

In our concern a server work on it's own and we are the one waiting that the server call himself it's own function. More precisely the hearthbeat here was (onmessage,onclose,onopen). So any DB or interaction with the server are made on those function.

There is a solution if you really want to get rid of extending the core function is instead of having upfront those function within testwebsocket or ws_withprotocol we move those behind the scene. Maybe in Application folder.

So the upfront file testwebsockets will only have 

$echo = new websockets("0.0.0.0","8080");
some way to specify the application to use
$echo->run();

So user will end up coding is server in phpwebsocket/application/myapplication.php

Personally I prefer having upfront the heartbeat function. Keep phpwebsocket cleaner in my own opinion. But maybe there is a way to have this function available without using extend that way it'll keep those function upfront and will be easier for you to implement PHPUnit.

It seem Ratchet use this method and wrench use behind the scene application.

ghedipunk commented 9 years ago

What I'm looking to do is more along the lines of

#!/usr/bin/env php
<?php
require_once('core/bootstrap.php'); // Includes the absolutely required core files, autoloader, and verifies the sanity of the environment.

class MyChatApplication 
{
    private $server;

    function __construct()
    {
        $this->server = new \Phpws\Core\Websockets($this);

        // The only actually required handler
        $this->server->registerHandler('onMessage', array($this, 'myMessageHandler'));

        // All below are optional
        $this->server->setUserClass('MyChatUser'); // The user class would be a prime candidate for implementing an interface, since the server needs to be able to store and retrieve the socket resource, among other things, and an interface would ensure that the correct methods exist.
        $this->server->registerHandler('onConnect', array($this, 'myConnectHandler'));
        $this->server->registerHandler('onClose', array($this, 'myDisconnectHandler'));
        $this->server->registerHandler('onError', array($this, 'myErrorHandler'));
        $this->server->registerHandler('onTick', array($this, 'heartbeat'));
        $this->server->setListenPort('8080'); // Not sure what I'd  have the default be.  Probably 6680 since it's close to IRC, but not between 6660 and 6670, and 80 is a good HTTP related number.

    }

    public function myMessageHandler($user, $message)
    {
        $outMessage = $user->getNick() . ': ' . $message;

        $connUsers = $this->server->getConnectedUsers();
        foreach ($connUsers as $cUser)
        {
            if ($cUser->getChannel() == $user->getChannel())
            {
                $this->server->send($cUser, $message);
            }
        }
    }

    public function playThatFunkyMusic();
    {
        $this->server->run()
    }

    // ... Additional methods snipped for brevity.
}

$app = new MyChatApplication();
$app->playThatFunkyMusic();

Of course, the class could be in a different file, either explicitly loaded in a require_once() call, or handled by the autoloader... For instance, if the class were in ./apps/mychatapplication.php (and thus had the namespace Phpws\Apps:

#!/usr/bin/env php
<?php
require_once('core/bootstrap.php');

$app = new \Phpws\Apps\MyChatApplication();
$app->playThatFunkyMusic();

This is different from how I was originally intending to change things, where the core server would select the application to run based on the URI passed in for the request... However, it would lose its current simplicity and still be just as difficult to extend, so I'm opting to keep the app as the very first part called in the script, and if there were some need for multiple applications on the same server, there would have to be some other system in place to separate the logic (such as if ($user->getConnectionURI() == 'my/expected/path') or something similar in the onMessage handler.)

ghedipunk commented 9 years ago

On the topic of autoloading: My plan is to implement a system that is similar to the PHP-FIG's autoloading recommendations, but with an extra, "standards" breaking layer that allows it to interoperate with other autoloading systems.

(I can't bring myself to call any of PHP-FIG's writings a "standard" without putting the word "standard" in quotes, because an actual standard is well thought out, thoroughly discussed, and exists to solve a problem rather than simply restrict people to your way of doing things.)

More thoughts on autoloading: https://r.je/php-psr-0-pretty-shortsighted-really.html (PSR-4 still has the exact problem expressed in this article) https://r.je/php-autoloaders-should-not-be-case-sensitive.html

Xaraknid commented 9 years ago

I brainstrom a little to find the shortest and simplest way to handle the handler the way you want :

I try to imagine it with interface as one of solution to make a bridge between core ( websockets ) and userclass ( testwebsocket ). Same as your design you'll need to code a controller.

The simplest way it's a shame how easy it is. Simply by moving the userclass contained handler in same layer of protocol are. So instead of injecting websockets class to userclass we will inject handler to websocket class.

in folder handler <-- developper will edit this file to their liking editing onmessage,onopen,onclose,ontick

Then the testwebsocket.php will be there to load the websocket class, maybe also some configuration and if needed there we specify wich handler class we want to load.

What do you think ?

ghedipunk commented 9 years ago

I like the idea of a router sending messages and events to controllers. I've been mulling that idea in my mind for a while.

I'm reworking some things to make it as easy and as default friendly as possible, so that if a developer doesn't need routing, they won't even know that their messages are going through any sort of router... and making the router easy and direct enough to read that if a developer really needs to dig in and see what's happening, it's evident without having to go through seventy billion layers of abstraction.

Xaraknid commented 9 years ago

Trait are good for current design as everything is on same layer class and we only overwrite method. Trait are like multiple extends in a class but the problem is they work at compile time so we can't use them dynamically.

I think the best course of action will be to divide the big class containning everything into smaller class based on the layer they are working .

Will look like that :

-Core
run() ->eventloop ..............run() ..................................->buffer ..............addwritewatcher()...............cb_read() ..............removewritewatcher()

will eventually lead to the app class made by dev. Contained into an array

->apps['default']->onmessage() ->apps['/echobot']->onmessage()

Permissive mode use default for /* Strict mode kill the connection if 'Get' is not one allowed by server.

Xaraknid commented 9 years ago

struct

I be able to refactoring it this way up to apps level. Need to move send,broadcast and some other method outside websockets, because each apps are isolated class from each other and from eventloop also.

Because of this isolation we will be able to have a list of clients inside each apps for outgoing data.

Also the websockets are no longer an abstract class in this structure.

Right now the loading of this project is simple as that : 

$test = new phpwebsocket("0.0.0.0","8080",'libev');  //host,port,eventloop 
$test->addapp("/admin",new admintool());
$test->addapp("/echobot",new echobot());
$test->addapp("/chat",new chatsystem());
$test->run();