[Bug]: Incorrect connection creation #4

[ineersa]

Fixing connection management.
Configured tests and phpstan to actually work.
Small changes/improvements/bugfixes.

Related Issue:

• [Bug]: Incorrect connection creation #4
• [Bug]: Insert not working #11

[darkterminal]

What the reason of adding libsql::file prefix in config.url value?
Did you really read the libSQL standard connection at Turso Client PHP?
Did you know if Turso Client PHP is not build under PDO?

[ineersa]

What the reason of adding libsql::file prefix in config.url value?

Reason is that ConfigurationUrlParser::parseConfiguration will parse this correctly into:

• driver - libsql
• database ($path) - file:tests/_files/test.db

Then we look at the path:

elseif (str_starts_with($path, 'file:') !== false) {
   $this->connection_mode = 'local';
}

And see that it's local connection.

Did you really read the libSQL standard connection at [Turso Client PHP](https://github.com/tursodatabase/turso-client-php)?
Guilty, not really. I've looked into examples, and then mostly looked into stubs, and your connection mode resolving, implemented and tested each of those (memory, local, remote, remote_replicated), but maybe it need more tests to support all DSN's.
I've tried to make URL's in laravel config to be close to extension DSN one's.
https://github.com/tursodatabase/turso-driver-laravel/blob/main/src/libsql_php_extension.stubs.php

Did you know if [Turso Client PHP](https://github.com/tursodatabase/turso-client-php) is not build under PDO?
Nope, I don't know Rust, didn't look inside extension.

[darkterminal]

Thank you to introduce singleton method, now the Service Provider look more clean and on point. But, still I need to tested with other Laravel instances and other Laravel Package. So far I worked on this driver to be able to work with the stancl/tenancy package.

<?php

namespace Turso\Driver\Laravel;

use Illuminate\Database\DatabaseManager;
use Spatie\LaravelPackageTools\Package;
use Spatie\LaravelPackageTools\PackageServiceProvider;
use Turso\Driver\Laravel\Database\LibSQLConnection;
use Turso\Driver\Laravel\Database\LibSQLConnector;

class LibSQLDriverServiceProvider extends PackageServiceProvider
{
    public function configurePackage(Package $package): void
    {
        /*
         * This class is a Package Service Provider
         *
         * More info: https://github.com/spatie/laravel-package-tools
         */
        $package
            ->name('turso-driver-laravel');
    }

    public function register(): void
    {
        parent::register();

        $this->app->singleton(LibSQLConnector::class, function ($app) {
            return new LibSQLConnector();
        });

        $this->app->resolving('db', function (DatabaseManager $db) {
            $db->extend('libsql', function ($config, $name) {
                $config = config('database.connections.libsql');
                $config['name'] = $name;
                if (! isset($config['driver'])) {
                    $config['driver'] = 'libsql';
                }
                $db = app()->get(LibSQLConnector::class)->connect($config);

                $connection = new LibSQLConnection($db, $config['database'] ?? ':memory:', $config['prefix'], $config);
                app()->instance(LibSQLConnection::class, $connection);

                $connection->createReadPdo($config);

                return $connection;
            });
        });
    }
}

I love the idea about getDb and getConnectionMode function, but those method should be used in LibSQLConnection.php:

public function getDb(): LibSQL
{
    return $this->db->getDb();
}

public function getConnectionMode(): string
{
    return $this->db->getConnectionMode();
}

It's should be available via $this->db property from the __construct:

public function __construct(LibSQLDatabase $db, string $database = ':memory:', string $tablePrefix = '', array $config = [])
{
    
    $this->db = $db;
    $this->schemaGrammar = $this->getDefaultSchemaGrammar();
    $libsqlDatbase = function() use ($db) {
        return $db;
    };

    parent::__construct($libsqlDatbase, $database, $tablePrefix, $config);
}

Note: instead passing the $db variable into Connection constructor, now I used database Closure:

public function __construct(LibSQLDatabase $db, string $database = ':memory:', string $tablePrefix = '', array $config = [])
{
    
    $this->db = $db;
    $this->schemaGrammar = $this->getDefaultSchemaGrammar();
-   parent::__construct($db, $database, $tablePrefix, $config);
+   $libsqlDatbase = function() use ($db) {
+       return $db;
+   };
+
+   parent::__construct($libsqlDatbase, $database, $tablePrefix, $config);
}

Illuminate\Database\Connection::construct will Create a new database connection instance. And the first parameter should be PDO instance or Closure.

When those 2 methods getDb and getConnecionMode is used in LibSQLConnection then we can access it from DB Facades like DB::getConnectionMode()

[ineersa]

I love the idea about getDb and getConnectionMode function, but those method should be used in LibSQLConnection.php

It might be but, both LibSQLConnection and LibSQLDatabase has db property, which are actually different, so it will be very ambiguous to use it inside LibSQLConnection.
I've added those methods to make classes more usable in tests only.

[ineersa]

Note: instead passing the $db variable into Connection constructor, now I used database Closure:

Changed that in code.

When those 2 methods getDb and getConnecionMode is used in LibSQLConnection then we can access it from DB Facades like DB::getConnectionMode()

I don't think that it works like that.
First of all you shouldn't do things like this app()->instance(LibSQLConnection::class, $connection);, imagine you have 2 libsql connections, which one will be inside container?
As for facades, imagine you have 2 libsql connections, which of those will be in container, which one of those you will access via facade?

[darkterminal]

This PR is awesome! However, some adjustments are needed regarding the standard connection string used by the PHP Turso/libSQL client.

[ineersa]

This PR is awesome! However, some adjustments are needed regarding the standard connection string used by the PHP Turso/libSQL client.

Thanks, but it needs testing, for example I've missed exact how Arr:add works in Laravel and connection had no name in configs, and actually everything was working=)

I've caught error on migrations rollback since i set connection explicitly for migrations it does

$this->resolver->setDefaultConnection($connection->getName());
$migration->{$method}();

Took me an hour to find error, so it needs more tests=)
Fixed it here a02cd06

[darkterminal]

This function should not be removed, considering that libSQL is not built under PDO. This will cause problems when running artisan commands related to the database.

[ineersa]

I'll look into it, can rollback.
Removed since it was not used anymore.
The only place it's used is \Illuminate\Database\Connectors\ConnectionFactory::make which is called when DBManager creates connection via ConnectionFactory, but in our case we extend it, so it will not use ConnectionFactory.
But yea it's easy to rollback. Will look today

[ineersa]

Looking more closely, createReadPdo() is not related in any way to connection, it's actually ConnectionFactory method, which is not used by this driver. So i don't see why should we have this method.

[darkterminal]

The LibSQLDatabase constructor should be look like this, to allow custom database path location:

public function __construct(array $config = [])
{
    $config = config('database.connections.libsql');

    $url = str_replace('file:', '', $config['url']);
    $config['url'] = match ($this->checkPathOrFilename($config['url'])) {
        'filename' => 'file:' . database_path($url),
        default => $config['url'],
    };

    $this->setConnectionMode($config['url'], $config['syncUrl'], $config['authToken'], $config['remoteOnly']);

    $this->db = match ($this->connection_mode) {
        'local' => $this->createLibSQL(
            $config['url'],
            LibSQL::OPEN_READWRITE | LibSQL::OPEN_CREATE,
            $config['encryptionKey']
        ),
        'memory' => $this->createLibSQL(':memory:'),
        'remote' => $config['remoteOnly'] === true
        ? $this->createLibSQL("libsql:dbname={$config['syncUrl']};authToken={$config['authToken']}")
        : throw new ConfigurationIsNotFound('Connection not found!'),
        'remote_replica' => $this->createLibSQL(
            array_diff_key($config, array_flip(['driver', 'name', 'prefix', 'database', 'remoteOnly']))
        ),
        default => throw new ConfigurationIsNotFound('Connection not found!'),
    };
}

This part determine if:

• DB_DATABASE value is database.sqlite it's detected use default database path location
• DB_DATABASE value is /your/custom/path/database.sqlite it's detected use custom database path location

$url = str_replace('file:', '', $config['url']);
$config['url'] = match ($this->checkPathOrFilename($config['url'])) {
    'filename' => 'file:' . database_path($url),
    default => $config['url'],
};

The checkPathOrFilename method is simply check if the given string is a path or just a filename.

private function checkPathOrFilename(string $string): string
{
    if (strpos($string, DIRECTORY_SEPARATOR) !== false || strpos($string, '/') !== false || strpos($string, '\\') !== false) {
        return 'path';
    } else {
        return 'filename';
    }
}

And we need to changed the setConnectionMode method to check the connection type in order from remote_replica down to memory type connection, ensures that more specific and stricter conditions are checked first. This helps to prevent incorrect connection modes from being set due to less specific conditions being met first.

private function setConnectionMode(string $path, string $url = '', string $token = '', bool $remoteOnly = false): void
{
    if ((str_starts_with($path, 'file:') !== false || $path !== 'file:') && ! empty($url) && ! empty($token) && $remoteOnly === false) {
        $this->connection_mode = 'remote_replica';
    } elseif (strpos($path, 'file:') !== false && !empty($url) && !empty($token) && $remoteOnly === true) {
        $this->connection_mode = 'remote';
    } elseif (strpos($path, 'file:') !== false) {
        $this->connection_mode = 'local';
    } elseif ($path === ':memory:') {
        $this->connection_mode = 'memory';
    } else {
        $this->connection_mode = false;
    }
}

[ineersa]

The LibSQLDatabase constructor should be look like this, to allow custom database path location

No it's not, you have all of the problems which were existing before here again:

• $config = config('database.connections.libsql'); you should not read config, it's passed here already. If you hardcoding this you make impossible to use any coonection apart from libsql from config and denying possibility to use more then 1 connection.
• db creation should not be done in constructor, in this case you cannot mock it to make tests, i know that init() method is not good also, maybe need to reconsider architecture wise classes structure to make it testable.
• you trying to use some hacks to parse url, which is not even available in config. URL from config is parsed inside Laravel via ConfigurationUrlParser, and what you actually receive here is:

array:10 [
 "driver" => "libsql"
 "authToken" => ""
 "syncUrl" => ""
 "syncInterval" => 5
 "readYourWrites" => true
 "encryptionKey" => ""
 "remoteOnly" => false
 "prefix" => ""
 "database" => "file:tests/_files/test.db"
 "name" => "local_file"
]

As you can see there is no url at all, and if you pass correct URL to your config it will be parsed correctly.
That's why i removed URL from your resolving and using $path which is $config['database'], which is exactly how Laravel resolves it for all other connections.

• setConnectionMode is already using correct order from specific ones to more broad - remote_replica->local->remote->memory

You can dump config and run test and look what is actually coming to this method, this will help to understand that you DON'T parse raw hardcoded configs, you already have parsed configs here, and the only thing you need is change documentation with correct URL definitions.

[darkterminal]

Talking about tests, I don't write any test before. So if you can write tests for this package that would be amazing!

[ineersa]

Talking about tests, I don't write any test before. So if you can write tests for this package that would be amazing!

I can add tests to verify that examples provided are working with correct configurations.
For examples i mean this ones:

/**
         * Creates a new LibSQL instance.
         *
         * ## Example Usage
         * 1. **Local Connection:**
         *
         *    Establishing a connection to a local database is straightforward with LibSQL. You have three options:
         *
         *    a. **Standard DSN Connection:** If you're using a DSN string, use the following format:
         *       ```
         *       $db = new LibSQL("libsql:dbname=database.db", LibSQL::OPEN_READWRITE | LibSQL::OPEN_CREATE, "");
         *       ```
         *
         *    b. **Standard SQLite Connection:** For direct SQLite connections, simply provide the database file name:
         *       ```
         *       $db = new LibSQL("database.db", LibSQL::OPEN_READWRITE | LibSQL::OPEN_CREATE, "");
         *       ```
         *
         *    c. **Standard LibSQL Connection:** Alternatively, you can specify the file protocol explicitly:
         *       ```
         *       $db = new LibSQL("file:database.db", LibSQL::OPEN_READWRITE | LibSQL::OPEN_CREATE, "");
         *       ```
         *
         * 2. **Remote Connection:**
         *
         *    Connecting to a remote database is equally effortless. Choose between two options:
         *
         *    a. **Standard DSN Connection with 'libsql://':**
         *       ```
         *       $db = new LibSQL("libsql:dbname=libsql://database-org.turso.io;authToken=random-token");
         *       ```
         *
         *    b. **Standard DSN Connection with 'https://':**
         *       ```
         *       $db = new LibSQL("libsql:dbname=https://database-org.turso.io;authToken=random-token");
         *       ```
         *
         * 3. **Remote Replica Connection:**
         *
         *    To set up a replica connection for distributed systems, follow these steps:
         *
         *    a. Define the configuration array with the required parameters:
         *       ```
         *       $config = [
         *          "url" => "file:database.db",
         *          "authToken" => "secrettoken",
         *          "syncUrl" => "libsql://database-org.turso.io",
         *          "syncInterval" => 5,
         *          "read_your_writes" => true,
         *          "encryptionKey" => "",
         *       ];
         *       ```
         *
         *    b. Instantiate a new LibSQL object with the configuration array:
         *       ```
         *       $db = new LibSQL($config);
         *       ```
         *
         * With this Quick Start guide, you're ready to seamlessly integrate LibSQL PHP Extension into your projects, whether for local, remote, or distributed database connections.
         */

Other then that, maybe if I'll get some errors or make changes I'll add test for those.

[darkterminal]

@ineersa You can test this PR now and let me know if you have any adjustment.