Fixes [Bug]: Incorrect connection creation #4 #13
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,171 @@ | ||
| parameters: | ||
| ignoreErrors: | ||
| - | ||
| message: "#^Call to function is_null\\(\\) with Illuminate\\\\Database\\\\Schema\\\\Grammars\\\\Grammar will always evaluate to false\\.$#" | ||
| count: 2 | ||
| path: src/Database/LibSQLConnection.php | ||
|
|
||
| - | ||
| message: "#^Method Turso\\\\Driver\\\\Laravel\\\\Database\\\\LibSQLConnection\\:\\:getDefaultSchemaGrammar\\(\\) should return Illuminate\\\\Database\\\\Schema\\\\Grammars\\\\Grammar but returns Illuminate\\\\Database\\\\Grammar\\.$#" | ||
| count: 1 | ||
| path: src/Database/LibSQLConnection.php | ||
|
|
||
| - | ||
| message: "#^Return type \\(Turso\\\\Driver\\\\Laravel\\\\Database\\\\LibSQLDatabase\\) of method Turso\\\\Driver\\\\Laravel\\\\Database\\\\LibSQLConnector\\:\\:connect\\(\\) should be compatible with return type \\(PDO\\) of method Illuminate\\\\Database\\\\Connectors\\\\ConnectorInterface\\:\\:connect\\(\\)$#" | ||
| count: 1 | ||
| path: src/Database/LibSQLConnector.php | ||
|
|
||
| - | ||
| message: "#^Constructor of class LibSQL has an unused parameter \\$config\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Constructor of class LibSQL has an unused parameter \\$encryption_key\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Constructor of class LibSQL has an unused parameter \\$flags\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Constructor of class LibSQLResult has an unused parameter \\$config\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Constructor of class LibSQLResult has an unused parameter \\$parameters\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Constructor of class LibSQLResult has an unused parameter \\$sql\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Constructor of class LibSQLStatement has an unused parameter \\$conn_id\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Constructor of class LibSQLStatement has an unused parameter \\$sql\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Constructor of class LibSQLTransaction has an unused parameter \\$conn_id\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Constructor of class LibSQLTransaction has an unused parameter \\$trx_mode\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQL\\:\\:changes\\(\\) should return int but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQL\\:\\:execute\\(\\) should return int but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQL\\:\\:executeBatch\\(\\) should return bool but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQL\\:\\:isAutocommit\\(\\) should return bool but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQL\\:\\:prepare\\(\\) should return LibSQLStatement but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQL\\:\\:query\\(\\) should return LibSQLResult but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQL\\:\\:transaction\\(\\) should return LibSQLTransaction but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQL\\:\\:version\\(\\) should return string but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQLResult\\:\\:columnName\\(\\) should return string but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQLResult\\:\\:columnType\\(\\) should return string but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQLResult\\:\\:fetchArray\\(\\) should return array but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQLResult\\:\\:numColumns\\(\\) should return int but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQLStatement\\:\\:columns\\(\\) should return array but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQLStatement\\:\\:execute\\(\\) should return int but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQLStatement\\:\\:parameterCount\\(\\) should return int but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQLStatement\\:\\:parameterName\\(\\) should return string but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQLStatement\\:\\:query\\(\\) should return LibSQLResult but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQLTransaction\\:\\:changes\\(\\) should return int but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQLTransaction\\:\\:execute\\(\\) should return int but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQLTransaction\\:\\:isAutocommit\\(\\) should return bool but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php | ||
|
|
||
| - | ||
| message: "#^Method LibSQLTransaction\\:\\:query\\(\\) should return array but return statement is missing\\.$#" | ||
| count: 1 | ||
| path: src/libsql_php_extension.stubs.php |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,12 +16,13 @@ class LibSQLConnection extends Connection | |
|
|
||
| public function __construct(LibSQLDatabase $db, string $database = ':memory:', string $tablePrefix = '', array $config = []) | ||
| { | ||
| $libsqlDatabase = function () use ($db) { | ||
| return $db; | ||
| }; | ||
| parent::__construct($libsqlDatabase, $database, $tablePrefix, $config); | ||
| $this->db = $db; | ||
| $this->setReadPdo($libsqlDatabase); | ||
|
|
||
| $this->schemaGrammar = $this->getDefaultSchemaGrammar(); | ||
| } | ||
|
|
||
|
|
@@ -30,33 +31,6 @@ public function sync(): void | |
| $this->db->sync(); | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Why are you removing actual implementation? /**
* Execute an SQL statement and return the boolean result.
*
* @param string $query
* @param array $bindings
* @return bool
*/
public function statement($query, $bindings = [])
{
return $this->run($query, $bindings, function ($query, $bindings) {
if ($this->pretending()) {
return true;
}
$statement = $this->getPdo()->prepare($query);
$this->bindValues($statement, $this->prepareBindings($bindings));
$this->recordsHaveBeenModified();
return $statement->execute();
});
}This will totally mess up things further, especially if you will work with models inserts/updates. There was a problem hiding this comment. I had to lie with this to the public function getDb(): LibSQL
{
return $this->db->getDb();
}
public function getConnectionMode(): string
{
return $this->db->getConnectionMode();
}
public function statement($query, $bindings = []): bool
{
$res = $this->select($query, $bindings);
return !empty($res);
}
public function getPdo(): LibSQLDatabase
{
return $this->db;
}
public function getReadPdo(): LibSQLDatabase
{
return $this->db;
}
public function select($query, $bindings = [], $useReadPdo = true)
{
$result = (array) parent::select($query, $bindings, $useReadPdo);
$resultArray = array_map(function ($item) {
return (array) $item;
}, $result);
return $resultArray;
}Because when I used this method: /**
* Execute an SQL statement and return the boolean result.
*
* @param string $query
* @param array $bindings
* @return bool
*/
public function statement($query, $bindings = [])
{
return $this->run($query, $bindings, function ($query, $bindings) {
if ($this->pretending()) {
return true;
}
$statement = $this->getPdo()->prepare($query);
$this->bindValues($statement, $this->prepareBindings($bindings));
$this->recordsHaveBeenModified();
return $statement->execute();
});
}Database connections will use the PDO engine to perform binding, whereas libSQL itself does not have natural binding parameters like PDO. There was a problem hiding this comment. Yes, that's right, and you are rewriting it inside your Statement class, method execute. On my dev branch I've changed structure a little, to make concerns for statement/connection classes more clear. Primary I done that to address issue with Also I have some strange problems with bindings on INSERT currently, where TEXT column data is truncated. But if you will try and look into my dev branch, you can see that everything is working via base methods. There was a problem hiding this comment. Yea problem is more fundamental, and lies in internal not compatibility between this implementation and PDO classes. There was a problem hiding this comment. So, now it's clearer why I made a lot of unconventional things with this package. And for reason about the connection string need to follow the Turso conventional, because the binary itself is have a logic to identifying the connection mode like this: pub fn get_mode(
url: Option<String>,
auth_token: Option<String>,
sync_url: Option<String>,
) -> String {
match (url, auth_token, sync_url) {
(Some(ref url), Some(ref auth_token), Some(ref sync_url))
if (url.starts_with("file:") || url.ends_with(".db") || url.starts_with("libsql:"))
&& !auth_token.is_empty()
&& (sync_url.starts_with("libsql://")
|| sync_url.starts_with("http://")
|| sync_url.starts_with("https://")) =>
{
"remote_replica".to_string()
}
(Some(ref url), Some(ref auth_token), _)
if !auth_token.is_empty() && url.starts_with("libsql://")
|| url.starts_with("http://")
|| url.starts_with("https://") =>
{
"remote".to_string()
}
(Some(ref url), _, _)
if url.starts_with("file:")
|| url.ends_with(".db")
|| url.starts_with("libsql:")
|| url.contains(":memory:") =>
{
"local".to_string()
}
_ => "Mode is not available!".to_string(),
}
}Check if is Remote Replica Connection?
Check if is Remote Connection?
Check if is Local Connection?
There was a problem hiding this comment.
Please remember, the real challenge is to make it compatible without using PDO. So far what I've done is freestyle which may look very strange. |
||
| public function select($query, $bindings = [], $useReadPdo = true) | ||
| { | ||
| $result = (array) parent::select($query, $bindings, $useReadPdo); | ||
|
|
@@ -68,6 +42,13 @@ public function select($query, $bindings = [], $useReadPdo = true) | |
| return $resultArray; | ||
| } | ||
|
|
||
| protected function getDefaultQueryGrammar() | ||
darkterminal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| ($grammar = new LibSQLQueryGrammar())->setConnection($this); | ||
|
|
||
| return $grammar; | ||
| } | ||
|
|
||
| /** | ||
| * Get the default schema grammar instance. | ||
| * | ||
|
|
@@ -88,16 +69,6 @@ public function useDefaultSchemaGrammar() | |
| } | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Again, createReadPdo() is used only in ConnectionFactory, which we are not using, and it's ConnectionFactory method, not connection. |
||
| protected function escapeBinary(mixed $value): string | ||
| { | ||
| $hex = bin2hex($value); | ||
|
|
@@ -145,14 +116,4 @@ public function quote(string $value): string | |
| { | ||
| return $this->escapeString($value); | ||
| } | ||
darkterminal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@darkterminal As we were talking #5 (comment)
I don't think it's good idea to have this methods, because they are ambiguous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better if this were reserved for internal derivatives only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can already access
$this->db, and even tho, since it's actually public it's not reserved for internal usage only, maybe it would be better to set it to protected.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense... the property are need to be protected.