I don't think it's a good idea to hardcode this. The extra cost of calculating the string's length should be meaningless, and we would only maintain one constant.

Do we want lowercase comparations?

For display names I'd say yes

Isn't this a bit complex? Why do we want to use prefix?

The prefix is to avoid collisions with groups created in other backends like LDAP, etc.

Also we're using a numeric group id in this backend internally to be able to rename the display name.
Another alternative would be to generate a random hash string id instead of numeric... not sure what's best, I thought this approach would be the simplest.

I don't think it's the backend responsability to resolve the collisions nor to prevent them. Any other backend could have a group with the same name "customgroup_12", and there shouldn't be any reason to avoid it.

I think it's core the one who should take care of how to resolve the collisions. Generally, just saying "I want this group" is enough to know what is the group you're refering to, but maybe sometimes you should say "I want this group which is located in this backend" so there is no doubt about the group you want.

While I agree that it's not this backend's responsibility, we'll still get into trouble if all our groups are called "1", "2", "3", etc... And I don't intend to fix core to have its own collision detection / namespacing as part of this task, which is likely a bigger task on its own which might not be backward compatible.

I'm questioning the purpose of this class... does it make the code too complex if we move this code to the backend?

Anyway, we should rename the class. The backend class will have a manager above (provided by core, where the backend will be added) and also below (this class) which is confusing. CustomGroupsDBHandler seems a better choice.

The purpose of the manager is to be used only by this one app to manage custom groups.

The backend itself only uses a subset of operations that it exposes to core because we don't want the users page to allow managing these, but we still want the search to work for the sharing autocomplete. Furthermore the backend class needs to work with core-style group ids (strings) while the manager works with this app's specific numeric id, so a conversion needs to be done.
I prefer to keep these separate to make the code clearer.

If I would move the extra operations to manage group members to the backend class as well, I don't want to expose this to the core interface. So we'd have two methods addToGroup, one that is empty for the backend and another one that is only known by the customgroups app. That's not very clean and clear to understand.

Hope this clears it out a bit 😄

ok, got it, but I'd still rename the class to reduce confusion.

you're right, this is more a database accessor. But we never used the name "DBHandler" before.

In core we used manager for "CommentsManager" and "SystemTagsManager" which do the same.
Some older call calls it "Mapper" like "TagMapper", but it's mostly because they use real objects instead of just arrays.

What do you think ?

I'm not sure if we should add another index only for the user_id. I expect the queries for groups will use this index, but I don't think the queries for users will.

Ah, I thought that this was the proper way to define multi-column primary unique keys ?
I copied this idea from core's group_user table.

@DeepDiver1975

The multi-column index is fine because we want group_id + user_id be unique. I trust the DBMSs be smart enough to use this index when we query for group information, but I don't think this will happen for user information.

I don't know where this come from.

What do you mean ? This method is called in appinfo/app.php

The \OCA\CustomGroups\CustomGroupsBackend needs to be register in the container, but I don't know where this registration is made.

No, it doesn't. DI container power !

It works because the class name matches exactly so it's able to resolve it.
You only need explicit registration when the names different or when your constructor requires parameters that cannot be injected (ex: strings)

😮 👏

we might want to document how the search is done, case-insensitive and in the middle of the string.

$limit = 0 is acceptable?

why don't return $result?

because $result is false, not null...

We should log this exception if possible. I think it's fine for now to keep the dependencies as simple as possible, but it's something to be considered.

I'd wrap this in a transaction. If we want to delete a group, we want both actions to be executed or none of them. Something in the middle it be would be an unexpected result.