Group cardinality interferes with other group types

The scenario you describe for group 2.0.x is working as intended. The first one seems buggy if it indeed behaves like that. GroupRelationshipCardinalityTest covers exactly this behavior. Given how 8.x-1.x is not receiving any more updates I'd say this is a bug in v1 and won't be fixed any more.

To me it seems that the 1st scenario is working as intended and the 2nd is bug introduced in v2.

The plugin configuration form states the following:

This form allows you to configure the Group media (Audio) plugin for the Musical piece group type.

and

The amount of Musical piece groups a single Media entity can be added to as a Group media (Audio). Set to 0 for unlimited.

From this, it is clear that this restriction in should apply to the group type and not across all group types. It would be interesting to understand in what scenarios would somebody be interested to restrict cardinality across different groups types. Is there an issue that describes why the change was made in version 2? I don't understand it.

Moreover, to me it seems that in the latter case it would be at least confusing, if not buggy. Let's say you set cardinality on Group Type 1 to 1 and on Group Type 2 to 0. So here you have a conflicting instruction. If that's the intended functionality, it should be ensured to be the same for all relationships using the same plugin. Right now, it will allow you to add the same entity to as many groups of Group Type 2, but it will enforce the restriction when relating to groups of Group Type 1.

That may be considered a bug due to this line:
if ($group_cardinality <= 0 && $entity_cardinality <= 0) {

But in any case, if you would set to non-zero cardinalities but different values on different Group Types you would still get inconsistent behavior.

I'm opening the issue again to discuss, and I'm also creating an MR for people that need the previous behavior. Maybe there should be an additional option e.g. checkbox that defines the behavior. Feel free to close if you think this is still intentional, but in that open separate issues as there's 3 bugs identified in my text above.

It would seem you're right. I apologize for closing this before.

Having said that, to fix bugs like these, we always need to expand the test first to prove something is broken and then add the fix showing the new or expanded red test once again goes green. Otherwise we risk breaking this again in the future.

I've fixed the test locally and it went red without your fix and green with. I've also changed the violation message to be more clear.

Using patch workflow because I CBA checking out the MR for every small bug report I get :P

Renamed issue and committed it.

kristiaanvandeneynde → closed merge request !99

Looks good, thanks.

Automatically closed - issue fixed for 2 weeks with no activity.