The sync function in includes/functions_admin.php

[Toxyy]

I've been going through here trying to add an event and the more I look at some of it the more I want to fix it... one example:

Code: Select all

if (count($forum_ids) == 1)
{
	$sql = 'SELECT MAX(t.topic_last_post_id) as last_post_id
		FROM ' . TOPICS_TABLE . ' t
		WHERE ' . $db->sql_in_set('t.forum_id', $forum_ids) . '
			AND t.topic_visibility = ' . ITEM_APPROVED;
}
else
{
	$sql = 'SELECT t.forum_id, MAX(t.topic_last_post_id) as last_post_id
		FROM ' . TOPICS_TABLE . ' t
		WHERE ' . $db->sql_in_set('t.forum_id', $forum_ids) . '
			AND t.topic_visibility = ' . ITEM_APPROVED . '
		GROUP BY t.forum_id';
}

can just be

Code: Select all

$sql = 'SELECT t.forum_id, MAX(t.topic_last_post_id) as last_post_id
	FROM ' . TOPICS_TABLE . ' t
	WHERE ' . $db->sql_in_set('t.forum_id', $forum_ids) . '
		AND t.topic_visibility = ' . ITEM_APPROVED . 
	((count($forum_ids) == 1) ? '' : ' GROUP BY t.forum_id');

I understand not wanting to use group by but selecting a keyed column isn't going to slow down anything, especially in the case that it's only one forum. Even if a condition to check that is added, there is no need for two separate queries.. There's a couple cases of stuff like this among other things... since there are no events using what I want to go over, I wanted to get the go ahead before making a ticket.

The sync function is ran a lot, I haven't thought too much about these two queries but something tells me there's a better way...

Code: Select all

// This routine assumes that post_reported values are correct
// if they are not, use sync('post_reported') first
$sql = 'SELECT t.topic_id, p.post_id
	FROM ' . TOPICS_TABLE . ' t, ' . POSTS_TABLE . " p
	$where_sql_and p.topic_id = t.topic_id
		AND p.post_reported = 1
	GROUP BY t.topic_id, p.post_id";
$result = $db->sql_query($sql);

$fieldnames[] = 'reported';
while ($row = $db->sql_fetchrow($result))
{
	$topic_data[intval($row['topic_id'])]['reported'] = 1;
}
$db->sql_freeresult($result);

// This routine assumes that post_attachment values are correct
// if they are not, use sync('post_attachment') first
$sql = 'SELECT t.topic_id, p.post_id
	FROM ' . TOPICS_TABLE . ' t, ' . POSTS_TABLE . " p
	$where_sql_and p.topic_id = t.topic_id
		AND p.post_attachment = 1
	GROUP BY t.topic_id, p.post_id";
$result = $db->sql_query($sql);

$fieldnames[] = 'attachment';
while ($row = $db->sql_fetchrow($result))
{
	$topic_data[intval($row['topic_id'])]['attachment'] = 1;
}
$db->sql_freeresult($result);

[DavidIQ]

Your code fix is backwards as the group by was used when there is more than one forum_id. However that overall fix would break the query when there is only one forum_id since the topic_id column is not agregated or grouped in that instance. You'd have to exclude it the same way you did the group by clause.

[Toxyy]

Your code fix is backwards as the group by was used when there is more than one forum_id. However that overall fix would break the query when there is only one forum_id since the topic_id column is not agregated or grouped in that instance. You'd have to exclude it the same way you did the group by clause.

I haven't done anything with the last block of code, I assume you mean forum_id. Yeah I see that now, I'm going to blame 5 am coding for that mistake. I've just fixed it in the OP.

I'll take your neutral stance to OK me making a pr, I'll make one within the week about this most likely.