Review and improvements in databases drivers MySQLi, Postgre and SQLite: #1335
Conversation
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
1) Fixed typo method name "escapeLikeStr()" to "escapeLikeString()". 2) Methods _fieldData(), _indexData() and _foreignKeyData() now return only array and throw DatabaseException (instead of returning false). 2.1) BaseConnection, added abstract methods() missing because this class relies on all its subclasses having those methods. 3) BaseBuilder, added TODO about a parameter with default value. 4) composer.json, added ext-mysqli at require-dev section.
lonnieezell
requested changes
Oct 24, 2018
There was a problem hiding this comment.
Thanks for making those to throw exceptions! The biggest thing I commented on here is for you to localize the strings thrown by the exceptions.
All of the other exception files use a different style for throwing exceptions. If you wanted to emulate what those were doing, that would be great, but I won't push that one since I obviously missed the DatabaseException class when I went through and converted those.
composer.json
Outdated
| @@ -24,7 +24,8 @@ | |||
| "require-dev": { | |||
| "phpunit/phpunit": "^7.0", | |||
| "mikey179/vfsStream": "1.6.*", | |||
| "codeigniter4/codeigniter4-standard": "^1.0" | |||
| "codeigniter4/codeigniter4-standard": "^1.0", | |||
| "ext-mysqli": "*" | |||
There was a problem hiding this comment.
Mysql driver is not needed if you plan on using Postgres. Please remove.
| { | ||
| return FALSE; | ||
| throw new DatabaseException('Failed to get field data from MySQL.'); |
There was a problem hiding this comment.
Please use a localized string that is added to system/Language/en/Database.php
| { | ||
| return FALSE; | ||
| throw new DatabaseException('Failed to get index data from MySQL.'); |
| @@ -488,7 +494,7 @@ public function _indexData(string $table) | |||
| } | |||
| else | |||
| { | |||
| throw new \LogicException('parsing key string failed.'); | |||
| throw new \LogicException('Parsing key string failed.'); | |||
There was a problem hiding this comment.
If you wouldn't mind moving this one to the Database lang file too, that would be great!
| @@ -521,7 +528,7 @@ public function _foreignKeyData(string $table) | |||
|
|
|||
| if (($query = $this->query($sql)) === false) | |||
| { | |||
| return false; | |||
| throw new DatabaseException('Failed to get foreign key data from MySQL'); | |||
| @@ -338,7 +341,7 @@ public function _indexData(string $table) | |||
|
|
|||
| if (($query = $this->query($sql)) === false) | |||
| { | |||
| return false; | |||
| throw new DatabaseException('Failed to get index data from PostgreSQL.'); | |||
|
|
||
| if (($query = $this->query($sql)) === false) | ||
| { | ||
| return false; | ||
| throw new DatabaseException('Failed to get foreign key data from PostgreSQL.'); |
| if (($query = $this->query('PRAGMA TABLE_INFO('.$this->protectIdentifiers($table, true, null, | ||
| false).')')) === false) | ||
| { | ||
| return false; | ||
| throw new DatabaseException('Failed to get field data from SQLite.'); |
| if (($query = $this->query($sql)) === false) | ||
| { | ||
| return false; | ||
| throw new DatabaseException('Failed to get index data from SQLite.'); |
| @@ -354,7 +353,7 @@ public function _indexData(string $table) | |||
| $obj->fields = []; | |||
| if (($fields = $this->query('PRAGMA index_info('.$this->escape(strtolower($row->name)).')')) === false) | |||
| { | |||
| return false; | |||
| throw new DatabaseException('Failed to get index_info() from SQLite.'); | |||
|
Hi Lonnie thank you I'll do these improvements still this week. |
jim-parry
added
the
waiting for info
added 2 commits
October 30, 2018 21:51
…ngs. 2) Language/en/Database.php, new entries about exceptions. 3) composer.json, removed ext-mysqli.
|
Hi there, I've done the modifications, thanks. |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
2.1) BaseConnection, added abstract methods() missing because this class relies on all its subclasses having those methods.