-
-
Notifications
You must be signed in to change notification settings - Fork 777
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
FeverAPI 32-bit fixes #1964
FeverAPI 32-bit fixes #1964
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks straightforward
p/api/fever.php
Outdated
$sql .= ' id < :id'; | ||
$values[':id'] = $max_id; | ||
$order = ' ORDER BY id DESC'; | ||
} else { | ||
} elseif ($since_id !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was wrong before, so just the question if we want to make it fail safe:
Looks like the calling part makes sure that either since_id or max_id is not null. But if we consider that someone is trying to use the API programmatically, then we should move both the WHERE and the ORDER (or at least the WHERE) clause into the if-else. Otherwise the SQL would be broken if someone calls this function with max_id=null and since_id=null.
Or probably use an array to collect all WHERE parts and append it with implode(' AND ', $where)
before $order is added to $sql.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Here is a suggestion 44d3e51
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old 1=1 trick is also fine :-D
p/api/fever.php
Outdated
} | ||
} else if (isset($_REQUEST['with_ids'])) { | ||
$entry_ids = explode(',', $_REQUEST['with_ids']); | ||
} else { | ||
// use the since_id argument to request the next $item_limit items | ||
$since_id = isset($_REQUEST['since_id']) && is_numeric($_REQUEST['since_id']) ? intval($_REQUEST['since_id']) : 0; | ||
$since_id = '' . $_REQUEST['since_id']; | ||
if (!ctype_digit($since_id) || $since_id < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read the docu correct, a negative sign would already not be accepted by ctype_digit
as it is a non numeric value. So I guess the <0 check is not required.
Also from the documentation: the string 000000001 would be accepted, but I don't know what the database makes out of it in that case id > 000000001
. Thats why I originally used intval().
But to be honest, I didn't test these edge cases.
So you pushed while I was still reviewing and typing. LOL, seems you catched my remarks before I even submitted them ;-) |
* FeverAPI 32-bit fixes FreshRSS#1962 * Small fixes FreshRSS#1964 (comment)
* FeverAPI 32-bit fixes FreshRSS#1962 * Small fixes FreshRSS#1964 (comment)
#1962