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

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

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

HQJaTu
Copy link
Contributor

@HQJaTu HQJaTu commented Mar 7, 2024

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

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

@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
Copy link
Contributor Author

HQJaTu commented Mar 9, 2024

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

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

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

HQJaTu commented Mar 13, 2024

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

@garvinhicking garvinhicking merged commit ba544f9 into s9y:master Mar 13, 2024
@garvinhicking
Copy link
Member

Perfect! Thanks!

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

Successfully merging this pull request may close these issues.

2 participants