These requests for comments/change have lead to an implemented feature that has been successfully merged into the 3.1/Ascraeus branch. Everything listed in this forum will be available in phpBB 3.1.
$db->sql_transaction('begin');
UPDATE phpbb_groups g1, phpbb_groups g2
SET g1.group_teampage = g1.group_teampage - 1
WHERE g1.group_teampage > g2.group_teampage
AND g2.group_id = $group_id
AND g2.group_teampage <> 0
UPDATE phpbb_groups
SET group_teampage = 0
WHERE group_id = $group_id
$db->sql_transaction('commit');
move_up($group_id) and move_up($group_id) is basically swapping two integers. You have to ensure you're not at the bottom or top there. Transactions should also be used there.
Not sure if the "UPDATE phpbb_groups g1, phpbb_groups g2" works on all DBMSes. - naderman says no
The huge switch block should probably be devided into separate functions (lesson learned from 3.0.x).
Most of those functions/methods will only take the group_id as a parameter anyway.
functions/methods should use the function/method keywords (public, private, protected, static).
functions/methods should have proper documentation about the arguments they take and return
Queries using the primary key do not need sql_query_limit(). There is only one row for what you're looking for anyway.
Fixed
bantu wrote:
Some queries can probably be joined together. For 'add' it could probably be
You can leave out the function prefix "teampage_", because you're already in the class "phpbb_teampage".
I'd for example change "teampage_action_add($group_id)" to "add_group($group_id)". The word "action" doesn't really say anything in this case.
I'd suggest to make GROUP_NOT_TEAM a class constant in the phpbb_teampage class.
Please use SQL transactions to group multiple update queries together.
All functions are missing the documentation for the returned value. If no value is returned you can use "@return void"
Since phpbb_teampage has no constructor and no properties, it might make sense to implement it fully as a static class.
function get_group_teampage should not always call trigger_error() if the group doesn't exist. This might not be what someone wants in some cases. trigger_error() should probably be called from function manage_teampage() in acp_groups.php (preferred). Or make it an optional parameter (not preferred).
You might be able to reuse this function in phpBB/includes/functions_user.php, because there is "$sql = 'SELECT group_teampage" again.
Non-trivial if statements should have a comment to explain what you're checking for. For example "if ($current_teampage > 1)". You're checking if the group is already at the top, right?
Little thing that came up to my mind.
We could change group_legend to uint aswell and using the class with an additional-parameter could introduce a sortable and easy to manage group-legend aswell.
There are currently to MODs in the DB having a dl-count of 6000 in total, but as it's very easy to implement, with the already existing functions I think that should go into the core.
Member of the Development-Team — No Support via PM
nickvergessen wrote:
There are currently to MODs in the DB having a dl-count of 6000 in total, but as it's very easy to implement, with the already existing functions I think that should go into the core.
+1. This seems like one of those features that people would use if it was there, but the MOD isn't worth it
nickvergessen wrote:Little thing that came up to my mind.
We could change group_legend to uint aswell and using the class with an additional-parameter could introduce a sortable and easy to manage group-legend aswell.
There are currently to MODs in the DB having a dl-count of 6000 in total, but as it's very easy to implement, with the already existing functions I think that should go into the core.
Sounds like it would make sense. I guess we/you can turn the class you wrote into a more generic class so it can be used for the team page and the group legend.
By the way, I guess you also have to provide an option to have it the way it is right now in 3.0.x, sorting by group_name.