🇧🇪Belgium @msnassar

msnassar → created an issue.

Test only patch to demonstrate the issue.

msnassar → created an issue.

@akshay_d
I couldn't reproduce the issue in #16...
I followed your steps and besides I used both block and embed display types...
Also tested it with/without having indexed items...
For me: $this->total_items is "0" in all cases!

@drunken monkey Thanks, works like a charm if the aggregation type is NOT “Concatenation”. However the issue reported by @RichardDavies still exists if the type is “Concatenation” (This is expected). I believe we should create a follow up, so moving this to RTBC...

@drunken monkey, Thanks for your feedback...
You are right this is not a bug...Config values could be set while not used... The thing is, setting the separator might be confusing, specially when doesn't know why a separator is set when you have aggregated field of types other than "Concatenation". I think, maybe it would be nicer in such a case to set it to ''?

@RichardDavies I have encountered the same issue. As a workaround, I set "Value separator" to nothing. For sure this doesn't work in case you have "Concatenation" field and you would like to set it to "\n\n" (There is no problem with other values e.g. "\n", "\t"). I am not sure if it is drash problem, because I see the diff in the UI as well.

@LOBsTerr Thanks for moving on with this issue. I looked quickly into the MR, I didn't see the application of _group_permissions_enabled to anything? Am I missing something?

Also as mentioned in #8 (point 2), I think we should applied custom validation on the group permission entity that prevent creating group permission entities in case the group permission is not enabled on the type of the group? This will be helpful for modules that uses group permission module as a api (We could do this in a follow up).

@LOBsTerr I wonder why we have different access check in v1.0 and v2.0... I believe that both should have same access check like the one in #5. No?

@LOBsTerr Sorry I didn't noticed that the other PR has been merged... Thanks
I wonder what is the plan for Group permission 2.0?

@LOBsTerr Thanks for the quick fix...
Just one has been missed... see here https://git.drupalcode.org/project/group_permissions/-/merge_requests/27...

@matslats Yes, you should create insider and outsider role...
This is good for cases like the one mentioned in #3

@matslats I think the proper solution is to create new 'insider' admin role and give it the permissions you gave for the 'outsider' admin role. Besides fixing your issue, this will also be helpful in case someone would like to give more permissions to users who on longer outsider but become insider (member).

msnassar → created an issue.

msnassar → created an issue.

I believe this can also be solved by an existing facet processor "Exclude specified items".

How to:
- On your glossary facet, enable "Exclude specified items".
- In your index processor settings, if you enabled "Group Numeric (0-9)", then you should enter 0-9 in Exclude items, otherwise, you should enter all numbers from 0 to 9 separated by comma.

@aleix There is no plan to add the modules mentioned in #5 to group media. This is because, they can be used without the group media module. Both has submodules to integrate with group media.

Hello @aleix did you have a look into Group media library extra → module. It comes with pluggable solution for changing the behavior of the media library view when it is being opened from a group context.
The module depends on Group media library → which solves other issues when using media library with groups. Including 🐛 Incorrect Access Check on Media Library RTBC

I think, we could also remove the hook node_local_tasks_alter here.
see: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/node/...

Sorry forgot to change to NR.
I have updated the description of the ticket to have better explanation for the issue and added UPDATE section too...

@koosvdkolk
This is not out of the box feature. You have multiple options:
- Create the settings and PermissionCalculators yourself.
- Use group_permissions_template → module (works perfectly for your simple use case)
- Use group_permissions_parameter → module (Use for complex use cases e.g. having conditional permissions overrides)

It might be related. After updating to D10.1, we encounter similar issue:
TypeError: Cannot assign array to property Drupal\views\Plugin\views\field\FieldPluginBase::$last_render of type Drupal\Component\Render\MarkupInterface|string|null in Drupal\views_aggregator\Plugin\views\style\Table->executeAggregationFunctions() (line 540 of modules/contrib/views_aggregator/src/Plugin/views/style/Table.php).

After some debugging, I noticed that the function views_aggregator_tally declares the variable $values = ['column' => []];. However, at the end, the 'column' element will never get a value! In this case, I wonder why the $values has the 'column' element? I also removed $separator_column = empty($separator_column) ? '<br/>' : $separator_column; because it is not being used by the function.

Regarding the PHP version, we are using PHP 8.1, which has no issues with dynamic properties. The dynamic properties are deprecated in PHP 8.2

@SomebodySysop It should work for all Group (1/2/3) and Drupal (9/10) versions. However, you don't need it for Drupal < 10.1
Note: This is a temporary fix. It can be used until Core and Group completely switch to the generic revision UI.

Here is a patch that should work for all versions.

@dalra creating a version that doesn't need the core patch is not possible (simply it won't work). The is due to the fact that core is hardcoding permissions in order to give access to CRUD comments.

@sahaj our team created new patch for D10. Please see here #137 📌 Use comment access handler instead of hardcoding permissions Needs work

I have updated the MR to make it compatible with D10. Here is new patch at commit a61a4731

Add the missing @param to the docblock

New module has been created https://www.drupal.org/project/gutenberg_group_media →

msnassar → created an issue.

In one group type, we are running some custom code that checks the access on the from mode of the group type. Due to the fact that the group is not exists on the group create, we get a fatal error:
Drupal\Core\Database\InvalidQueryException: Query condition 'group_permission.gid IN ()' cannot be empty. in Drupal\Core\Database\Query\Condition->condition() (line 117 of core/lib/Drupal/Core/Database/Query/Condition.php).

I think, we should always check the group id before loading group_permission entity by a group..

  public function loadByGroup(GroupInterface $group) {
    if (!$group->id()) {
      return NULL;
    }
    $group_permissions = $this->loadByProperties(['gid' => $group->id()]);
    return !empty($group_permissions) ? reset($group_permissions) : NULL;
  }

Here is the patch in #11 with this little fix included.

msnassar → created an issue.

Here is a patch has been originated from the MR at commit 798169a7. It adds the following to the MR:

• Fix property assignment $this->groupPermissionsManager = $group_membership_loader
• Add group_list cache tag to the synchronized group permission calculator.
• Add the group bundle "group_permission:{$group_permission->getGroup()->bundle()}:{$group_permission->getGroup()->id()}" to the calculated permission items for the synchronized group permission calculator
• Add new tag "group_permission:{$group_permission->getGroup()->bundle()}:{$group_permission->getGroup()->id()}" to the cached calulated permissions.

Regarding the QueryAlter issues, I think we should fix it in another ticket. It seems there is no an easy way to be fixed in a contrib modules/custom code without overriding the Query alter classes that comes with group module.
The thing is, group module assumes that the group permission items:
- Have only the group id as identifier for the calculated individual group permissions.
- Have only the group bundle as identifier for the calculated synchronized group permissions.
Which is totally prefect for group module. But this is not the case for modules like group_permissions. However, as I mentioned, we could override the query alter classes that are provided by group module. But, I believe the group module should make it easier for other modules
to intervein. So I think we should fix this in a separate ticket. As a workaround, I am applying a local patch to the group module to make the
query alters working with group_permissions.

msnassar → created an issue.

@shashank5563 The group finder context will return the first applied plugin. It depends on the weight of the plugins. The higher the weight, the later the founder is being called...
If changing the weight is not working for you, you could create your own context, then the context will be available in the "Select a group value".

@shashank5563 the group_finder module is an api module. it doesn't provide a UI. The finder plugins are being called by GroupFinderContext and modules/custom code that are using it...

Patch for group 3

msnassar → created an issue.

msnassar → created an issue.

msnassar → created an issue.

Here is a patch originated from the PR.

Hello @ydahi

Yes, I think this is feasible!

@ydahi This module doesn't prevent you from selecting media items that do not belong to the current group even when using the module with core's media library widget. For such functionality, I have already created another module Group Media Library Extra → . The module should work out of the box if the media library in Gutenberg editor is aware of the group context. But this is not the case, because Group Media Library module doesn't support Gutenberg editor. It only supports the core's media library widget. To keep this module simple, I do not have a plan to support functionalities other than the ones that come with core.
On the other hand, the media library in Gutenberg editor module is working differently than the one in core.
If we want to support Gutenberg editor, I prefer to do it in new contrib module.

I need some time to investigate how to bring in group context for gutenberg editor.
BTW, Thanks for offering a fund. I really appreciate your help.

@LOBsTerr
Thanks for moving on with this issue. Here is my feedback about the latest changes and my previous comment #13

Are you sure you checked the latest changes, we have group entity in every route (including revisions), and _group_permission access check works without any fatal errros.

Yes, I have checked and test it the last changes. I know we have the group entity in all routes :) BUT what I have mentioned in #13 is the "group permission" entity!
See below for more info. about the fatal errors.

Also we can drop GroupPermissionAccessControlHandler, since it is not used.

I wonder if this is ideal! By doing this, we lose the access check on the entity type. Meaning websites that are using a different way to create "group permission" entities for example using "rest/json api", might end up not being able to do so or might have vulnerability impact.
This is because they might end up using the "administer group permission entities" global permission. Granting this permission to all global roles that should create "group permissions" entities, will skip the permission granted in the group level. This will have a vulnerability impact. Because if they do so, unprivileged users, will then be able to create "group permissions" entities in the groups that are not suppose to do so! On the other hand, a members who should be able to create "group permissions" entities based on the permission in the group, might not be able to create entities because they do not have the global permission "administer group permission entities" granted to them!

My idea is still to use "override group permissions" to control access to all routes related to group permissions

I am still in favor of applying the entity access on the routes :) This is because of the reasons I mentioned in #13...
On the other hand, every time we need to apply new access rules to the entity, we have to apply them in the routes! Example ✨ Enabling per-group permissions by group type Needs work

I can't see any fatal errors, because we have group entity for all group permissions related routes. I have tested.

Here are the steps to reproduce:

• Create new group. Make sure there is no group permission entity for that entity
• Go to: Group permissions by clicking on the "Group permissions" tab
• You should see the delete and revisions tabs.
• When you click on the tabs or visit group/XX/permissions/delete, you should see the fatal errors.

The issue here is that, the user has access to the delete and revisions pages for a non existing "group permission" entity
!
Let my know your thoughts please.

Sorry, I forgot to add the decorator. Here is new patch.

Here is another approach:
- Instead of using hook_node_create_access, it is decorating the node preview access check service.
- Instead of checking the create permission, it is delectating the access check to the access control handler.
- Instead of using the hook_form_alter, it is using hook_form_BASE_FORM_ID_alter to alter the preview form.
This patch works with group 2 only. I will create another one for group 3 if the approach is acceptable.
@kristiaanvandeneynde please let me know thoughts?

The MR! works for me. Thanks

I also believe that:
1- we should generate the permissions (we have only one for now "override group permissions") through "permission_callbacks". By doing it this way we can guarantee that the permissions are generated only when the group type supports group permissions.
2- Apply a custom validation on the group permission that validates the group type supports group permission. This will guarantee that the modules (e.g. group permissions parameter, group permissions flex, ...) using the group permissions module as API to only create group permissions entities if the group type support it.

@LOBsTerr what do you think?

Hello @LOBsTerr
I agree with the first 2 points.
I wonder if the last one is the way to go. Here is why:

1. Usually the access to entity routes are handled by adding _entity_access as a requirements. It is not possible to do this with some group permission routes (e.g. delete route) because we do not have the group permission entity in the route
2. The _entity_access grantees a correct access check on the route based on the entity access handler. So we don't have to add other access requirements (e.g. permissions) to the route explicitly. This grantees that any change that is being applied to the entity access handler, will automatically be applied to the route access without having to change the access requirements on the route.
3. Because of the above 2 points, I have added a custom GroupPermissionEntityAccessCheck.
4. Without applying the entity access check on routes (e.g. delete route and revisions route) and having only the _group_permission requirements, users will get fatal error (see below) when visiting the delete/revisions pages for not existing group permission entity. This is because the user has the access to the route anyway. Meaning the user has access to the route even if the group permission entity is not exist!

Here are the fatal errors:

The website encountered an unexpected error. Please try again later.
Error: Call to a member function getGroup() on null in Drupal\group_permissions\Form\GroupPermissionDeleteForm->getQuestion() (line 64 of modules/contrib/group_permissions/src/Form/GroupPermissionDeleteForm.php). 

The website encountered an unexpected error. Please try again later.
Error: Call to a member function getGroup() on null in Drupal\group_permissions\Controller\GroupPermissionsController->revisionOverview() (line 316 of modules/contrib/group_permissions/src/Controller/GroupPermissionsController.php).

I think we have 2 options to solve the issue:

1. Keep the GroupPermissionEntityAccessCheck. But we should simplify it, as now we have one access point check "one group permission". So we no longer need to pass the operations but instead we should pass TRUE (e.g. ->setRequirement('_group_permission_entity_access', 'TRUE'))
2. Add the group permission entity to all routes and then use _entity_access.

Let my know your thoughts please.

Here is a proposed patch for group 2 and 3. It is different from what I proposed in #6.
I would like to hear from the maintainers whether this solution is acceptable. If yes, then I will add tests.

@LOBsTerr The same approach is being used by the group module itself.
On the EC websites, user 1 is exist but blocked. At least for the websites I am working on :). We never deleted user 1!

Here is also a patch from the MR!

msnassar → created an issue.

I mark this one as duplicate as we have ✨ Allow 3rd party to easily add new params to media_library_opener_parameters Active with better description.

@dxvargas
The module is currently (with Drupal 10 compatibility changes) using the dev release because it removed the patch (see here) that is being used to fix #3228881: Allow plugins to opt-out for a given field →

The fix has already been merged. So this issue can be fixed after having new release for field_permissions containing the fix.

I have created a module "Group Media Library → " that solves some issues when using media library with groups.

The patch in #15 is working for our website as we do not allow sharing content between groups.

Anyway, the proposed solution, is good when:

• Content is not shared at all
• Content is shared between same group type
• Their is no conflict in the delete settings -for the same relation plugin- in the multiple group types

But, I wonder what should happen in case an entity is shared between 2 groups of different group type; where the relation plugins in each group type has different settings for entity deletion. For example:

• Having group type A and group type B
• Enable the relation plugin for Page content type in both A and B group types
• In group type A: The configs for the plugin is > "Delete target entity when group relation is deleted" (So we always want to delete - No matter if the entity is being shared or not.)
• In group type B: We keep the default > Do not delete the entity (The option do delete the target entity is not selected).
• Create 2 groups: Group X of type A and Group Y of type B
• Create a page node in group X and share it with group Y
• 1. If the user deleted the relation in group X, the node will be deleted
• 2. If the user deleted the relation in group Y, the node won't be deleted

Case #1:
It is deleting an entity that is being shared with other group (of different type) that assumes an entity should not be deleted when deleting its relation.

I think we should have a third option e.g. "Only delete when all same relation plugins in other group type allow to delete".
When this option is selected, Case #1 won't delete the target entity as it is being shared with a group of another type that prevents deleting the target entities!

Here is a patch for group 2 and 3...

Patch for group 2 and 3

As we are in the process of migrating from group 1 to group 2, I would like to add this feature to group 2 & 3. But I would like to hear from the maintainers first?
I have 2 options:

• Use the same concept as the patch in #5
• Use the same concept as the patch in #5 BUT move the method to AccessControlTrait:
  

    public function getEntityStatusOperations() {
      if (!isset($this->parent)) {
        throw new \LogicException('Using AccessControlTrait without assigning a parent or overwriting the methods.');
      }
      return $this->parent->getEntityStatusOperations();
    }
  

  And then, in the AccessControl:

    public function getEntityStatusOperations() {
      return ['view'];
    }
  

Any other alternatives are very welcomed...

The group permissions template is a nice module and works perfectly for a simple use case.
For other more complex uses cases e.g. having conditional permissions overrides (e.g. based on current user), permissions that depends on external resources, and multi-dimensional factors, etc..., it will be hard to achieve with group permissions template. So I have created new module to handle such case. For more info. see the module's page Group Permissions Parameter →

msnassar → created an issue.

I have added a draft MR. The MR won't work without first having https://www.drupal.org/project/group_permissions/issues/3360880 🐛 Routes are using non existing global permission Needs work merge .So change to "Needs work" for now...

This will be easy to fix once Routes are using non existing global permission 🐛 Routes are using non existing global permission Needs work get merged