The sync function in includes/functions_admin.php

General discussion of development ideas and the approaches taken in the 3.x branch of phpBB. The current feature release of phpBB 3 is 3.3/Proteus.
Forum rules
Please do not post support questions regarding installing, updating, or upgrading phpBB 3.3.x. If you need support for phpBB 3.3.x please visit the 3.3.x Support Forum on phpbb.com.

If you have questions regarding writing extensions please post in Extension Writers Discussion to receive proper guidance from our staff and community.
Post Reply
User avatar
Toxyy
Registered User
Posts: 8
Joined: Thu Aug 02, 2018 9:15 am

The sync function in includes/functions_admin.php

Post by 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);
Last edited by Toxyy on Fri Jan 04, 2019 12:42 am, edited 1 time in total.

User avatar
DavidIQ
Customisations Team Leader
Customisations Team Leader
Posts: 1904
Joined: Thu Mar 02, 2006 4:29 pm
Location: Earth
Contact:

Re: The sync function in includes/functions_admin.php

Post by 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.
Image

User avatar
Toxyy
Registered User
Posts: 8
Joined: Thu Aug 02, 2018 9:15 am

Re: The sync function in includes/functions_admin.php

Post by Toxyy »

DavidIQ wrote: Thu Jan 03, 2019 10:14 am 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.

Post Reply