Skip to content

Update comments.inc.php for compatibility with PHP 8.3#968

Open
qbi wants to merge 3 commits intos9y:masterfrom
qbi:patch-2
Open

Update comments.inc.php for compatibility with PHP 8.3#968
qbi wants to merge 3 commits intos9y:masterfrom
qbi:patch-2

Conversation

@qbi
Copy link
Copy Markdown

@qbi qbi commented Apr 16, 2026

When you want to access all comments, S9Y runs into an HTTP 500: PHP Fatal error: Uncaught DivisionByZeroError: Division by zero in line 260.

The comparison of strings and integers has changed in PHP 8. See https://www.php.net/manual/en/migration80.incompatible.php

Previously the comparison in line 260 was 0 == 'all' -> true. Whith the changes the comparison now is 0 == 'all' -> false. Which leads to the above mentioned error message.

I changed the code in a way that $commentsPerPage has either an interger orCOMMENTS_FILTER_ALL as value and the comparison now checks type and value. So in the case of all comments the comparison is 'all' === 'all -> true.

This should fix the code.

When you want to access all comments, S9Y runs into an HTTP 500:
`PHP Fatal error:  Uncaught DivisionByZeroError: Division by zero` in line 260.

The comparison of strings and integers has changed in PHP 8. See https://www.php.net/manual/en/migration80.incompatible.php

Previously the comparison in line 260 was `0 == 'all'` -> true.
Whith the changes the comparison now is `0 == 'all'` -> false. Which leads to the above mentioned error message.

I changed the code in a way that `$commentsPerPage` has either an interger or`COMMENTS_FILTER_ALL` as value and the comparison now checks type and value. So in the case of all comments the comparison is `'all' === 'all` -> true.

This should fix the code.
@mattsches mattsches self-requested a review April 16, 2026 15:34

$commentsPerPage = (int)(!empty($serendipity['GET']['filter']['perpage']) ? $serendipity['GET']['filter']['perpage'] : 10);
$commentsPerPage = !empty($serendipity['GET']['filter']['perpage']) ? $serendipity['GET']['filter']['perpage'] : 10;
if ($commentsPerPage != COMMENTS_FILTER_ALL) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, good catch and good fix!

My suggestion would be to change the first section as follows.
This introduces a stricter (type-safe) check against COMMENTS_FILTER_ALL.

if ($commentsPerPage !== COMMENTS_FILTER_ALL) {
    $commentsPerPage = (int)$commentsPerPage;
}

What do you think?

@onli
Copy link
Copy Markdown
Member

onli commented Apr 16, 2026

Surprisingly complicated code, originally. I assume it was never intended to rely on 0 == 'all' being true, but rather it was missed that the comparison was strange.

Alternative fix would be to always cast COMMENTS_FILTER_ALL when used in the comparison, so also on line 260 and 274, like

$pages = ($commentsPerPage == (int)COMMENTS_FILTER_ALL ? 1 : ceil($totalComments/(int)$commentsPerPage));

Then this becomes a 0 == 0 when $serendipity['GET']['filter']['perpage'] was set to COMMENTS_FILTER_ALL (which will be a string like 'all', and casts to 0)

Not sure that's better, but wanted to understand the bug and have this shared :)

@onli
Copy link
Copy Markdown
Member

onli commented Apr 28, 2026

I prepared the NEWS entry for this change. @qbi, please let us know if this is the way you want to be credited, or under your full name? If you could add @mattsches suggestion this would be great, otherwise I'll merge this as is the next days. In any case, thanks for finding and fixing this bug.

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.

3 participants