s9y / Serendipity

A PHP blog software
https://s9y.org
BSD 3-Clause "New" or "Revised" License
206 stars 86 forks source link

bugfix: Made comment token clean-up work in PostgreSQL also #826

Closed HQJaTu closed 6 months ago

HQJaTu commented 6 months ago

SQL-table options has column name defined as varchar. https://github.com/s9y/Serendipity/blob/master/sql/db.sql#L182

In code, the cleanup calculates an unix timestamp pointing to history one week ago. This interger is then compared with the mentioned varchar in SQL. This comparison might fly on some RDMSes, on PostgreSQL typing is strict and an int cannot be compared with a varchar.

This commit fixes the problem making comment handling working again.

garvinhicking commented 6 months ago

Nice catch, thank your for reporting! Let me check the codebase if we might have a similar problem somewhere else, I'm thinking about other timestamp-y fields used for login validation. Will get back to you on that tomorrow!

garvinhicking commented 6 months ago

@HQJaTu I've found this in include/functions_comments.inc.php (serendipity_commentSubscriptionConfirm):

    if (stristr($serendipity['dbType'], 'sqlite')) {
        $cast = "name";
    } else {
        // Adds explicits casting for mysql, postgresql and others.
        $cast = "cast(name as integer)";
    }

    serendipity_db_query("DELETE FROM {$serendipity['dbPrefix']}options
                                WHERE okey LIKE 'commentsub_%' AND $cast < (" . (time() - 1814400) . ")");

This is also used similiary in include/functions_config.inc.php for serendipity_issueAutologin() and serendipity_checkAutologin().

I wonder if, for consistency, you could alter your PR/patch so that we also use the explicit cast here, too? What do you think?

Best regards, Garvin

HQJaTu commented 6 months ago

Sure thing. I went that way.

As D.R.Y. -principle applies, I don''t repeat myself. I added a function to assist with casting, so it won''t be done in three separate places.

garvinhicking commented 6 months ago

Nice!! I was secretly hoping you would see the uglyness in this 🤓

Thanks, I'll review properly next week and merge then. Looking good!

garvinhicking commented 6 months ago

Would you maybe like to also use your CAST unification in the place include/functions_config.inc.php for serendipity_issueAutologin() and serendipity_checkAutologin() that I mentioned? :-)

HQJaTu commented 6 months ago

My bad. I missed that comment. Should be fixed now.

garvinhicking commented 6 months ago

Perfect! Thanks!