surrealdb / surrealdb.php

SurrealDB SDK for PHP
Apache License 2.0
42 stars 8 forks source link

Complete refactor/rewrite #6

Open iDevelopThings opened 1 year ago

iDevelopThings commented 1 year ago

Let me know any opinions/things that would need to be changed. But I feel this is a great starting point for the sdk, it just needs the regular create/update/patch/delete methods adding also

Remaining methods to implement:

signature description status
db.use(ns, db) Switch to a specific namespace and database
db.signup(vars) Signs this connection up to a specific authentication scope
db.signin(vars) Signs this connection in to a specific authentication scope
db.invalidate() Invalidates the authentication for the current connection
db.authenticate(token) Authenticates the current connection with a JWT token
db.let(key, val) Assigns a value as a parameter for this connection
db.select(thing) Selects all records in a table, or a specific record
db.create(thing, data) Creates a record in the database
db.update(thing, data) Updates all records in a table, or a specific record
db.change(thing, data) Modifies all records in a table, or a specific record
db.modify(thing, data) Applies JSON Patch changes to all records in a table, or a specific record
db.delete(thing) Deletes all records, or a specific record
DarkGhostHunter commented 1 year ago

Does this uses curl/rest? Because Websockets are the way to go.

iDevelopThings commented 1 year ago

Does this uses curl/rest? Because Websockets are the way to go.

It does - and i agree, but because PHP spins up a new instance for each incoming request, websockets aren't really viable unfortunately - I experimented a few times with a solution and still haven't found a decent one

DarkGhostHunter commented 1 year ago

Does this uses curl/rest? Because Websockets are the way to go.

It does - and i agree, but because PHP spins up a new instance for each incoming request, websockets aren't really viable unfortunately - I experimented a few times with a solution and still haven't found a decent one

I used ampphp/websocket.client, which uses Revolt underneath.

When you query via WS, you must push it with an ID (hopefully an UUID) to identify the incoming query, because you can do async queries, or even hear queries on SurrealDB.

iDevelopThings commented 1 year ago

@DarkGhostHunter take a look at that commit - I realised now that you're the person who worked on laragear/Surreal which i've used in the past :) - based some of the logic on your implementation.

As a side note also, i did try ampphp/websocket.client in the past, but it was blocking all further code from processing... I also see a situation occurring where responses will return out of order(if making multiple requests concurrently/using laravel octane), it's possible the next response via $this->connection->receive() won't be for the message which was sent - just as a heads-up for your library 👍

iDevelopThings commented 1 year ago

Also for progress reporting/before this gets merged, will hopefully have these done at some point this week

I want to add the methods that exist in the other sdks:

signature description
db.use(ns, db) Switch to a specific namespace and database
db.signup(vars) Signs this connection up to a specific authentication scope
db.signin(vars) Signs this connection in to a specific authentication scope
db.invalidate() Invalidates the authentication for the current connection
db.authenticate(token) Authenticates the current connection with a JWT token
db.let(key, val) Assigns a value as a parameter for this connection
db.select(thing) Selects all records in a table, or a specific record
db.create(thing, data) Creates a record in the database
db.update(thing, data) Updates all records in a table, or a specific record
db.change(thing, data) Modifies all records in a table, or a specific record
db.modify(thing, data) Applies JSON Patch changes to all records in a table, or a specific record
db.delete(thing) Deletes all records, or a specific record
DarkGhostHunter commented 1 year ago

I also see a situation occurring where responses will return out of order(if making multiple requests concurrently/using laravel octane), it's possible the next response via $this->connection->receive() won't be for the message which was sent - just as a heads-up for your library 👍

I'm thinking about a total rewrite, or making it more basic and tackle Laravel integration with another package.

DarkGhostHunter commented 1 year ago

Some things to point out:

Don't use singletons.

Create an instance, and just then save it into a singleton, like a static variable. You can add an static helper.

use SurrealDB\SurrealDb\Client;

Client::instance('https://db.app.com:8080', 'root', 'password', 'my-namespace', 'my-db');

// Same as:

$client = Client::make('https://db.app.com:8080', 'root', 'password', 'my-namespace', 'my-db');

Client::setInstance($client);

This allows to tackle both devs using vanilla PHP, and other using a framework with its own container. Imagine I want to use two databases at the same time...

This way you don't need to use static calls to the class to query something.

$results = Client::query('SELECT * FROM users');

// Underneath is..
public static function query($query = null)
{
    if (!static::$instance) {
        throw new Exception('There is no SurrealDB Client instance registered as singleton');
    }

    $builder = static::$instance->query();

    return $query ? $builder->raw($query) : $builder;
}

Also, since it should use WebSockets, there is the challenge to separate variables from bindings, which nullifies SQL injections. I don't know if SurrealDB has fixed that or not at time of writing.

iDevelopThings commented 1 year ago

Some things to point out:

Don't use singletons.

Create an instance, and just then save it into a singleton, like a static variable. You can add an static helper.

use SurrealDB\SurrealDb\Client;

Client::instance('https://db.app.com:8080', 'root', 'password', 'my-namespace', 'my-db');

// Same as:

$client = Client::make('https://db.app.com:8080', 'root', 'password', 'my-namespace', 'my-db');

Client::setInstance($client);

This allows to tackle both devs using vanilla PHP, and other using a framework with its own container. Imagine I want to use two databases at the same time...

Thanks for this idea :D I was trying to think of a way earlier on how I can use both setups, this will work perfectly 👍

Also, since it should use WebSockets, there is the challenge to separate variables from bindings, which nullifies SQL injections. I don't know if SurrealDB has fixed that or not at time of writing.

I don't think they have, currently the QueryBuilder I added handles using bindings properly, it should be good with websockets too. For raw queries it's up to the developer to use them

JABirchall commented 1 year ago

Its generally a bad idea to register global functions as part of the package. The functions should reside in a namespace, and they are needed by the developer they can be pulled in.