Skip to content
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

[3.0] Fix Group Membership Requests #8005

Merged
merged 3 commits into from
Jan 22, 2024

Conversation

jdarwood007
Copy link
Member

Fixes #7949
Fixes #7950

@Oldiesmann Please test this
@Sesquipedalian I don't like how the fix turned out, but I didn't see a better fix.

The major issue stems from a concept in Sources\Group.php, the load().
In various places in our group logic, we assumed load() would return

  • An array of IDs for the group
  • The keys would be the group ID
  • That when calling load with only one ID, we could do a list to pop off the first group.

Neither case was true. We returned an array of objects of SMF\Group. The key was not the group ID, but the next ID, which makes list() work, but breaks other logic.
Some of the logic in load() assumed that $loaded[$id_group], yet then assigned it to the array with []. Only lines apart.
I aligned it with how SMF\User does it just for uniformity. But because of this change, it could have far-reaching effects.

The second major issue was that when you do array_merge, you lose key associations. So I had to use += logic. This means that if any of the same keys exist on both sides, the right side of the logic is lost.

A third issue was that the addMembers logic was silently failing and SMF just happily assumed it worked. I added a check but didn't add a better error message right now. If we need one, we can.

A fourth issue is that we assumed Db::$db->fetch_all() would return an array of the ids in such fashion [1,2,3,5], when in fact it returned them as [0 => ['id_group' = 1], 1 => ['id_group' = 2']....]. A simple array map fixes this.

In many places, because we assumed that load() is returning IDs, not objects, we incorrectly assumed we could just use it like that. However, that is causing the issues. I added hints that should help most IDEs recognize that we are working with an object, not an int/array.

@Oldiesmann
Copy link
Contributor

Oldiesmann commented Jan 6, 2024

Works well for the most part. I was able to join/leave a free group (both as an admin and as a regular user) and add someone to the group via the mod center (without being prompted for my password since mod security is off).

However, I did find a couple more bugs. I'm not sure if either of these is related to your changes but I'll mention them here anyway.

Trying to remove someone from a group via the groups interface (Admin -> Members -> Membergroups, click group name, check box next to a member and click the button to remove them):
Uncaught TypeError: Cannot access offset of type SMF\\User on array in /.../Sources/Group.php:1226

When you go to edit a group (Admin -> Members -> Membergroups, click "Modify" next to a group), you now get an error saying "The membergroup does not exist or is invalid".

I was unable to test requestable groups (eg a group where someone has to approve your request to join) since I didn't have any and am unable to edit any new groups I create.

@Oldiesmann
Copy link
Contributor

Everything seems to be working now. The second bug I mentioned above seems to be due to me not copying the changes to Sources/Actions/Admin/Membergroup.php. I've been able to join/leave both a free and a requestable group (and approve a request to join the requestable group), as well as add and remove group members from the admin center.

@jdarwood007 jdarwood007 changed the title Fix Group Membership Requests [3.0] Fix Group Membership Requests Jan 7, 2024
@jdarwood007
Copy link
Member Author

Can another dev merge this? I can't test the functionality of group requests with strict typing because of this.

Sources/Group.php Outdated Show resolved Hide resolved
@tyrsson tyrsson self-requested a review January 22, 2024 00:06
@tyrsson tyrsson merged commit 08ea884 into SimpleMachines:release-3.0 Jan 22, 2024
3 checks passed
@jdarwood007 jdarwood007 deleted the groupMembershipBug branch January 22, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants