Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

phpList 3.7 #989

Open
michield opened this issue Sep 17, 2023 · 11 comments
Open

phpList 3.7 #989

michield opened this issue Sep 17, 2023 · 11 comments
Assignees
Labels

Comments

@michield
Copy link
Member

michield commented Sep 17, 2023

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. Handle ip behind proxies #914 - new way of setting URLs for the app.
  3. Group users by teams #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:

  • tinyint doesn't exist in Postgres - smallint or boolean, but boolean doesn't take "0/1" needs true/false
  • datetime needs to be datetimetz
  • unique-s can't have a name (eg unique indexname (column)
  • date_add is only available in pgsql 16 and up (and we want to support at least 14)
  • auto_increment works by creating sequences
  • blob types (long, medium etc) need to be bytea
  • replace into set X = Y doesn't work
  • values between double quotes " don't work, Postgres wants single quotes only
@michield
Copy link
Member Author

@michield
Copy link
Member Author

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

@michield
Copy link
Member Author

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
Copy link
Member Author

@bramley
Copy link
Contributor

bramley commented Sep 19, 2023

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

  • where !confirmed; # when using a tinyint/smallint as a boolean
  • limit 0,50 # LIMIT #,# syntax is not supported, Use separate LIMIT and OFFSET clauses
  • date_add(now(),interval 1 hour))
  • unix_timestamp eg unix_timestamp(now()) - unix_timestamp(entered) as age

@michield
Copy link
Member Author

@bramley
Copy link
Contributor

bramley commented Dec 26, 2023

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants