phpList / phplist3

Fully functional Open Source email marketing manager for creating, sending, integrating, and analysing email campaigns and newsletters.
https://www.phplist.org
GNU Affero General Public License v3.0
737 stars 268 forks source link

phpList 3.7 #989

Open michield opened 11 months ago

michield commented 11 months ago

I'd like to start working towards version 3.7. We can collect issues we want to achieve here:

  1. Postgres support (using PDO or ADOdb)
  2. 914 - new way of setting URLs for the app.

  3. 929 - make admins teams

  4. single CLI cron to process tasks, which may be more than just queue and bounces
  5. CI for Mysql as is, Mysql PDO and Postgres PDO
  6. config in the file as a global object CFG

more?

For Postgres I've noticed a lot of phpList SQL is Mysql specific.

table columns:

michield commented 11 months ago

Actually, it probably makes sense to use ADODB for this. https://adodb.org/dokuwiki/doku.php?id=index

They have some useful links:

michield commented 11 months ago

I will start a WIP PR for the PostgresQL/ADOdb changes, marked for 3.7 and will see how far I get

michield commented 11 months ago

Moodle seems to use ADOdb https://github.com/moodle/moodle/tree/master/lib/adodb

But I'm interested to hear Duncan's opinion how to approach this.

michield commented 11 months ago

Maybe another option is Doctrine https://www.doctrine-project.org/projects/doctrine-orm/en/2.16/tutorials/getting-started.html

bramley commented 11 months ago

What's the prime objective? If it is to support other databases then PDO seems to be the simplest. The current approach of functions in mysqli.inc can be replaced by similar functions that invoke PDO methods. So there's no need to change existing code, except to identify and rewrite non-portable sql constructs, but that needs to be done in any case.

ADOdb has syntax differences which would mean an enormous number of code changes, and Doctrine seems to be an entirely different approach. Either might be suitable if starting a new project but I don't think are suitable now.

Here's how the sql_* functions could be implemented to use PDO. The query is fully formed, so this doesn't get the benefits of pdo parameter binding, but is simple to implement. New code could use PDO properly. The PDOStatement object is analogous to the mysqli_result object and can be used to iterate through a result set.

function Sql_Connect($host, $user, $password, $database)
{
    global $database_port, $database_socket, $database_connection_compression, $database_connection_ssl;

    $dsn = sprintf('mysql:dbname=%s;charset=%s;host=%s;port=%s', $database, 'utf8mb4', $host, $database_port);
    $pdo = new PDO($dsn, $user, $password);

    if (!$pdo) {
        header('HTTP/1.0 500 Cannot connect to database');
        print "Cannot connect to Database, please check your configuration";
        exit;
    }

    return $pdo;
}

function Sql_Query($query, $ignore = 0)
{
    global $pdo;

    $statement = $pdo->prepare($query, []);
    $result = $statement->execute([]);

    return $statement;
}

function Sql_Fetch_Array($statement)
{
    return $statement->fetch(PDO::FETCH_BOTH);
}

function Sql_Fetch_Assoc($statement)
{
    return $statement->fetch(PDO::FETCH_ASSOC);
}
michield commented 11 months ago

Ok. thanks.

Yes, my initial primary objective is to support Postgres, and other DBs would be a bonus but not an aim.

Yes, the PDO as you describe was the approach I was taking, and then I was wondering if it was the correct one. Ok, great, if you agree with that, I will start a PDO branch and work my way through stuff.

I think we also want to use the Sql_Query_Params function and once we have PDO in place, it should work for both Psql and Mysql, so we can drop the mysqli.inc file.

michield commented 8 months ago

This is not as easy as it seems. Just initialising the DB structure already throws up enormous amounts of issues. One thing we need to do in a next version is convert all datetimes to bigints and do date and time calculations in the application, instead of the SQL. Eg "date_add(field, interval 1 day)" becomes "field + 86400"

michield commented 8 months ago

postgres doesn't like this in SQL, which phpList does all over the place

michield commented 8 months ago

Also, we need to rename these columns as 'user' is a reserved word.

bramley commented 8 months ago

This is not as easy as it seems. Just initialising the DB structure already throws up enormous amounts of issues. One thing we need to do in a next version is convert all datetimes to bigints and do date and time calculations in the application, instead of the SQL. Eg "date_add(field, interval 1 day)" becomes "field + 86400"

I found that postgres now does have DATE_ADD but with slightly different syntax to mysql which seems to make them incompatible https://www.postgresql.org/docs/16/functions-datetime.html#FUNCTIONS-DATETIME-TABLE

But this does seem a lot of work making changes and then testing. Having to reduce to a "lowest common denominator" of SQL that is supported by both mysql and postgres, and if that doesn't exist then moving processing into the application.

I'd be inclined not to go any further with this.

michield commented 8 months ago

Yes, I also noticed that postgres has date_add, but only from version 16. I'd like to try to support 14+

But if the functions are not interchangeable, we'll have to move the logic into the app anyway. One option would be to write stored procedures, but that's also a big ask, and additionally a less common approach.

I think I'll go the following route:

  1. change all date (except timestamps) columns to bigint (it has to be bigint, because "int" will finish Tuesday, 19 January 2038 03:14:07)
  2. store the "epoch" value of the related date
  3. do calculations in the application

It's going to be a big change. I will also add a pipeline to run an "upgrade from 3.6" on all changes. It won't cover everything, but it will help