Added PostgreSQL support #160
Conversation
| /** | ||
| * HostMapper class implementation for MariaDB | ||
| */ | ||
| class HostMapper implements HostMapperInterface |
There was a problem hiding this comment.
could it not extend the MySQL one if there is no changes to the implementation ?
There was a problem hiding this comment.
Implementation is slightly different, I had to remove all backticks from sql queries to make it work
sure there's a lot of duplicate code :(
There was a problem hiding this comment.
maybe all implementations could benefit from a function to quote column names @liuch ?
There was a problem hiding this comment.
@williamdes I think it's better to remove the quotes than to add another function to confuse the code. Quotation marks are not really necessary.
There was a problem hiding this comment.
@williamdes I think it's better to remove the quotes than to add another function to confuse the code. Quotation marks are not really necessary.
i agree, plus backticks are not sql standard. I also changed some group by clauses as PG is more strict about that.
In MariaDb, maybe SQL_MODE variable could help to standardize queries.
There was a problem hiding this comment.
In MariaDb, maybe SQL_MODE variable could help to standardize queries.
Thanks for the tip!
| @@ -22,7 +22,8 @@ | |||
| "league/flysystem-aws-s3-v3": "^2.5 || ^3.25.1" | |||
| }, | |||
| "suggest": { | |||
| "ext-imap": "Needed to process incoming DMARC reports stored in a mailbox" | |||
| "ext-imap": "Needed to process incoming DMARC reports stored in a mailbox", | |||
| "ext-pdo_pgsql": "Needed to use Postgresql" | |||
There was a problem hiding this comment.
Should we not move ext-pdo_mysql here too ?
|
Hi VilainMamuth, williamdes, Quoting column names is not necessary in my implementation. I find it convenient (to search for fields and table names in code), but I'm not willing to pay for it by increasing code duplication. I propose to do this: Add a directory, let's say classes/Database/Common, move to it all the code from classes/Database/Mariadb and remove the quoting of names. The DatabaseConnector::getMapper method will first look for the required class in the database type-specific directory, and then in the Common directory. Inheriting from Mariadb classes will be confusing, especially when there are more than two database types. Therefore, I suggest inheriting from classes in the Common directory if you need to make corrections to the class implementation. What do you thing about this? Sorry for the slow replies: I'm sick and it's currently a bit inconvenient for me to use the computer. |
Very good idea! Second subject: maybe we could benefit from an active record pattern? |
|
@VilainMamuth If I make a modification (moving to Common and removing name quoting), can you modify your PR? |
I don't like the long and difficult for understanding text strings of sql queries in my code, I've been wanting to solve this for a long time somehow. But the idea did not go further than writing special tests. I will definitely look at your code more carefully. |
|
I just did a refactoring of the code related to database access. I believe now we can get rid of a lot of duplicated code. |
awesome ! @VilainMamuth can you rebase this PR and use the new abtractions ? |
|
@VilainMamuth Do I understand correctly that you have just merged your branch with this one, without removing excess code? |
|
I wanted to rebase my repo from yours but github's "Sync fork" button does the f... merge |
|
hi, just a little follow up it seems related to inet_pton. |
Because I wanted to depend on the database type as little as possible. |
Initial support for PostgreSQL i made at work to test your tool