First-class menu item translation support

As of a recent version of Group module, we are supposed to introduce permissions via annotation on the content enabler plugin.

 *   handlers = {
 *     "access" = "Drupal\group\Plugin\GroupContentAccessControlHandler",
 *     "permission_provider" = "Drupal\group\Plugin\GroupMembershipPermissionProvider",
 *   },

See #3132097: Introduce permission providers for GroupContentEnabler plugins and #3151770: Introduce access control handlers for GroupContentEnabler plugins

Yeah, the permission options are pretty confusing. I need to give the global administer menus permission in order for a group member to add group content to a group menu on the node/edit page. But that doesn't allow them to add a link to the group menu from the menu's manage screen? Even if they have permission to create and delete entire group menus, they still can't add a new menu link from the manage menu screen.

It seems like there should be a group permission "add content to group menus" that determine whether an editor can add to a group menu. And that permission should allow them to add via the node/edit screen without the administer menus permission and via the Group Content Menu's manage menu page. What am I missing?

Just ran into this issue myself as well and hoping for a solution. Like sonfd said, there should be more granularity to the global / group menu permissions here.

It looks like Group Module advices to use Annotation based permission one Content Enabler(as mentioned in #5). However, Group Content Menu has not completely implemented it yet.
I tried to update the method which creates Groups based permission. Hope it helps.

In rc3, permissions were loosened up so if you had no non-group menus, then you don't need anything more granular. See #3176446: Node form menu permission improvements. If that doesn't address your concerns, feel free to post some code/suggestions how this can be better managed.

I am experiencing a similar situation where even though the user has all of the group permissions related to the group menu, they can not add a menu link.

The following screenshot illustrates this.

Here is a patch that adds a PermissionProvider, changes some route permissions and adjusts the logic in hook_form_node_form_alter. It solves the problem for our use case. To really fix the issue, I guess some more conceptional discussion, and programming work and tests would be neccessary. This patch could a least be a starting point...

Can we do baby steps here? First convert to access and permission provider? Land that. Then do the next step of extending the permissions?

Here's a patch that adds a new Permission provider for group content menus. Unlike patch #11, this patch adds just one permission, "manage $this->pluginId menu links". The idea is that we can continue to use the "Manage menus" permission defined in group_content_menu.group.permissions.yml as an "Administer menus" tier permission, and use this new permission to grant access to manage links within the various group menus.

Note that the new permission isn't checked anywhere yet so it doesn't do anything other than add the option to the permissions grid.

Looking at this further, I can't find an easy way to get the permissions provided by the Permission Provider to respect the group_content_menu they're assigned to. In testing, I was seeing that giving a permission for one menu type would actually give it to all types.

This got me thinking and it seems like a single group-wide permission to manage menu links for all group menus would be sufficient. Personally, I just want to let editors manage menu links without allowing them to create or delete menus, and without giving them the Drupal-wide "Administer menus and menu links" permission.

Here's a patch that takes an alternate approach:

1. Add a new "manage group_content_menu links" permission via group_content_menu.group.permissions.yml.
2. Update the add menu link, edit menu link, and edit menu routes to check for this new permission or the existing "manage group_content_menu" permission.
3. Update the node form alter to also consider this new permission

-----

As part of this patch, I removed the getCreatePermissions() method from GroupContentMenuRouteProvider. This method was only used to generate the permissions list for the add menu link route (which is now updated per the above). It was doing a couple things that are weird and just seem wrong. I think this explains why the add link was behaving so strangely.

1. It was adding a check for "create group_content_menu:$entity_type_id content" permissions (one for each group menu type, as far as I can tell) but this permission doesn't / couldn't exist.
2. As a fallback it was adding a check for the "access group content menu overview" permission. I believe this is also an error. This fallback state could only be triggered if no group menus were allowed in the group. (or maybe it would always trigger?) Either way, I'm not sure why you'd check that other permission in this scenario.

1. #14:

   +++ b/group_content_menu.module
   @@ -213,7 +213,7 @@ function group_content_menu_form_node_form_alter(&$form, FormStateInterface $for
   +      elseif (!$traditional_menus && ($route_group = \Drupal::routeMatch()->getParameter('group')) && ($route_group->hasPermission('manage group_content_menu', \Drupal::currentUser()) || ($route_group->hasPermission('manage group_content_menu links', \Drupal::currentUser())))) {
   

   This is getting complicated. Can we move this logic into a helper method? And honestly, maybe we should wholesale copy/paste this entire form alter into a class that we can invoke like: \Drupal::classResolver(NodeFormAlter::class)->alter($form, $form_state);. It would make the code review harder. But it would make moving things into protected methods much easier. If we keep the amount of modification when moving into the class, it shouldn't be that hard.

2. #13:

   +++ b/src/Plugin/GroupContentEnabler/GroupContentMenu.php
   @@ -20,6 +20,9 @@ use Drupal\group_content_menu\Entity\GroupContentMenuType;
   + *   handlers = {
   + *     "permission_provider" = "Drupal\group_content_menu\Plugin\GroupContentMenuPermissionProvider",
   + *   }
   

   But I really want to get to using permission_provider. Can we combine the provider approach with the method used in #14?

Ok - I'm going to do this in pieces.

Step #1 - Move existing permissions from group_content_menu.group.permissions.yml to a Permission Provider

Step #2 - Create NodeFormAlter class and move node form alter logic there.
Woops. A dud. Please ignore :(

Step #2 - Create NodeFormAlter class and move node form alter logic there.

Step #3 - Add permission to allow managing menu items (basically same as #14)

1. Add a new "manage group_content_menu menu items" permission.
2. Update the add menu link, edit menu link, and edit menu routes to check for this new permission or the existing "manage group_content_menu" permission.
3. Update the node form alter to also consider this new permission

With this new permission, we can now run into situations where a user doesn't have "Administer menus" permission, but does have permission to edit group menus.

When editing a node that only allows group menus (i.e. no menus allowed on the /admin/structure/types/manage/CONTENT_TYPE) form, then this works well.

However, in a scenario where the node does allow sitewide menus (and has relevant group menus), the menu options don't appear for editors with only the group level permissions. Expected behavior is that they would see menu options, but only for the group menus they can edit.

Here's a new patch that handles the menu options a little differently.

If the menu options are already visible on the node form, show everything. If they're not already visible, show them, but only with the group menu options.

Fix logic error in #22 when checking if `$form['menu']['#access'] != FALSE`

The tests need to be updated.

I think the form alter can be broken up a bit more now, too.

And I think it's incorrect to build all menu options for the content type's available menus. There are other modules, like Menu Admin Per Menu that also alter the available menus. I think a better approach would be to be much more passive and just merge our new group menu parent options with the existing array.

Here's the updated code from the recent patches that's changed a little to only load group menus if the menu access was previously set to false. (This allows editors to add to group menus if they have permission to work with group menus, but not global menus.)

// Here, I think we should call $menu_parent_selector->parentSelectElement() passing just group menus.
// Then, if the menu options aren't already visible, set the form element with just the group menu options.
// However, if there are already menu parent options, merge our group menu parent options with the
// existing rather than building all menu options from scratch. 
$parent_element = $menu_parent_selector
  ->parentSelectElement($default, $defaults['id'], $allowed_menus); // e.g. $allowed_menus = array_merge($group_menus, $menu_ui_menus)
$form['menu']['link']['menu_parent'] = $parent_element;

Looking back at all of this, I think this should probably be broken up into several separate issues.

1. Refactor Permissions to use a Permission Provider (patch #17)
2. Refactor NodeFormAlter to use class (patch #19) + I think this should also include additional work to split into more helper methods, e.g. getGroups(), and getGroupMenus()
3. Refactor NodeFormAlter method to be more passive - right now, if the node is in some groups, it just overrides the entire menu form element whether there are group menus or not. This should be much more passive - just appending group menu options to the existing element if it's visible, and only building the element from scratch if there are no traditional menus and the user has permission to add to group menus.
4. This issue (introduce new permission with appropriate updates)

@heddn - if you're ok with this plan, I can create new issues and try to do a lot of this in this upcoming coming week/weekend.

@sonfd, I really love your methodical approach you present in #27. Let's do it.

Ok, regarding #27 and #28:

1. Issue #3211817: Refactor to use a Permission Provider
2. Issue #3211821: Move node form alter processing to a helper class
3. Issue #3211822: Update node form alter logic to be more passive

With the larger problems handled in those other issues, the work needed here is greatly simplified.

This patch will apply after the patches from the other issues are applied.

Issue 3211817, Patch #2 AND Issue 3211821, Patch #3 AND Issue 3211822, Patch #2

• This patch adds a new group permission "Manage menu items" (manage group_content_menu menu items).
• It updates the Add Link, Edit Link, and Edit Menu routes to allow access to users with this permission.
• It updates the node form to show group menu options to users with this permission.

I love the simplicity of how this all broke up with all these distinct issues. Thanks for working on that. Will review this more seriously after we land the other tasks.

Makes sense - it will also need tests.

Getting close here as #3211821: Move node form alter processing to a helper class just landed. One blocking issue remaining.

Patch on #30 works great to grant the creation permission for menu links. However, there is still missing the delete and translation permission, so users without the site-wide permission to administer menu links, can't delete or translate menu links on groups.

Sorry, I changed the status and title by mistake.

I'm adding a new patch with #30 as starting point adding the following features:
- Deletion of menu links with group level permissions.
- Link to the menu link translations overview page with group level permissions.

Pending issues detected when the user hasn't the site permission to administer menus:
- Add menu link translation
- Edit menu link translation
- Change the parent on the menu link edition form (now is empty)

This patch uses #37 as the starting point and patch #10 from https://www.drupal.org/project/group_content_menu/issues/3211822.

The patch in #37 is great, but assumes that the content_translation module is enabled. If the content_translation module isn't enabled we receive a Class \Drupal\content_translation\Controller\ContentTranslationController does not exist error.

This patch adds logic to check if the content_translation module is enabled before adding the translation menu link.

I realized I incorrectly setup the interdiff. Here's the corrected interdiff showing what was changed specifically. Ignore this. This wasn't properly diff'd. See comment #40 for corrected solution.

Apologies, ignore #39, it was also incorrectly setup. Corrected interdiff and patch.

I'm adding a new patch with #40 as starting point adding the following features:

- Add menu link translation
- Edit menu link translation

Hello @sudesh.solaskar Thank you very much for the contribution.
Patch #41 contains accidentally changes from other patches. I'm uploading the right patch with only the desired changes, and the interdiff with the patch #40
As mentioned in previous comments, this patch requires patch #10 on #3211822: Update node form alter logic to be more passive in order to work

@sudesh.solaskar I applied the patch properly, but now I'm getting a fatal error:

Class Drupal\group_content_menu\Routing\GroupContentMenuRouteSubscriber does not exist

I see your patch is adding a event subscriber, but the referenced class is not added in the patch.

@akalam thank you for rectifying the patch #41. I have updated my patch with starting from #42.

Hello All, Please ignore the above patch as it does not include all the changes. I am uploading the patch again with all the changes. Also, while createing interdiff from #42 to #45 i was getting some error hence not uploading the interdiff

The patch on #45 works great but it has a small drawback with the menu link deletion, which is not being displayed.
Here is a patch fixing it.

Patch on #46 was throwing errors regarding null language when accessing to the translations overview and when editing translations. Among that, the menu links were wrongly generated.
Here is a new patch fixing those issues

During testing as a non-administrator user I found 2 issues:
- There was a fatal error regarding add translation links
- The edit link wasn't renderer

After a lot of debugging it was much simpler and efficient to override completely the parent method of \Drupal\group_content_menu\Controller\GroupContentTranslationController::overview()

Here is a patch fixing those issues.

I found an issue with destination query param when the site is placed under a subdirectory. Here is a patch fixing it.

I found an issue when user only with permission "Manage menu items translations" tries to delete the menu item translation from menu item translation edit page receives access denied error. Here is a patch fixing that issue.

After applying patch 50, I notice that a member from group1 can add and edit links in group 2's menu. It seems like members from one group should not be able to edit menus in other groups (which they are not members of)

I can't dig in deeper right now but hopefully I can soon.

Patch 49 overrides permission handling of ContentTranslationHandler but fails to take into account ContentTranslationHandler::getTranslationAccess when no group ist found. As a result, no translations can be added outside of a group context by anyone.

While I wasn't able to reproduce the issue mention by @selwynpolit, I didn't encounter a bunch of other issues. I've attemted to fix this in #53.

• Get rid of GroupMenuItemTranslateAccess and GroupContentMenuRouteSubscriber as it subscribes to the wrong routes anyways and can interfere with contrib and custom modules overriding permissions outsite of a group context.
• Take ContentTranslationHandler::getTranslationAccess into account when outside of any group context
• Display link to add a new menu item when menu is empty and user has permissions to do so
• Fix delete link which was no longer showing (related to #46)

Should also delete the service declarations when removing the classes. See patch.

Edit: I fubar-ed this patch completely, do not use.

I just noticed that GroupContentTranslationController is being referred on web/modules/contrib/group_content_menu/src/Routing/GroupContentMenuRouteProvider.php:192 but that class does not even exist and causes errors.

Edit 2: I somehow managed to mess up patching the module with the patch from #53 and then also creating a entirely messed up patch in #54. If there's someone able to remove that patch, please do. ;-)

Sorry for that mental breakdown of a patch. This time, this one should be good. Again, removing the service definitions for GroupMenuItemTranslateAccess and GroupContentMenuRouteSubscriber since they were deleted in #53.

Thanks @nielsva, just noticed myself this morning that he service declarations crept back into my last patch again somehow. #56 looks good.

If I understand it right, this is still blocked by #3211822: Update node form alter logic to be more passive and needs additional test coverage?

I didn't get the permission right last time.

-      $group->hasPermission("manage group_content_menu items", $this->currentUser());
+      $group->hasPermission("manage group_content_menu menu items", $this->currentUser());

This patch can be applied in addtion to patch #58 after applying patch #18 from #3211822: Update node form alter logic to be more passive,
see my comment on #3211822.

This allows menu item creation from Node form without needing access to manage menus, thus removing the "Delete Menu" from the menu overview for group members. Because I noticed that after deleting, re-adding a new menu with just the manage menu permission doesn't seem to work yet.

This is unblocked now. And I've added #3310748: Delete menu link. I don't think we want to merge it into here, but it does have repercussions to what we are trying to solve here.

1. +++ b/group_content_menu.module
   @@ -241,3 +241,13 @@ function group_content_menu_get_menu_link_default(NodeInterface $node, array $me
   +    $entity_types['menu_link_content']->setHandlerClass('translation', \Drupal\group_content_menu\GroupMenuItemTranslateAccessHandler::class);
   

   I don't think we want to overtake the entire access handler from core.

2. +++ b/src/Controller/GroupContentMenuController.php
   @@ -184,4 +184,17 @@ class GroupContentMenuController extends GroupContentController {
   +    return $this->entityFormBuilder()->getForm($menu_link_content, 'delete');
   

   Let's move delete access into #3310748: Delete menu link.

3. +++ b/src/Controller/GroupContentTranslationController.php
   @@ -0,0 +1,283 @@
   +class GroupContentTranslationController extends ContentTranslationController {
   

   This looks like a massive copy/paste from somewhere. Is that the case? Should we refer to something else and maybe even call parent:: and then override parts?

4. +++ b/src/Entity/GroupContentMenu.php
   @@ -44,8 +45,13 @@ use Drupal\group_content_menu\GroupContentMenuInterface;
   + *     "add-link" = "/group/{group}/menu/{group_content_menu}/add-link",
   

   why all the renaming?

5. +++ b/src/Form/GroupContentMenuForm.php
   @@ -293,17 +295,25 @@ class GroupContentMenuForm extends ContentEntityForm {
   +        elseif ($link->isDeletable()) {
   

   Nice. We should move this logic into #3310748: Delete menu link.

6. +++ b/src/GroupMenuItemTranslateAccessHandler.php
   @@ -0,0 +1,56 @@
   +  protected function entityFormDeleteTranslationUrl(EntityInterface $entity, $form_langcode) {
   

   I don't see where this code is getting used.

The work here seems focused on adding a delete permission and translation access. Let's split these into 2 things. Re-titling things here. Leaving at NW for tests and the feedback posted above. Sizing the scope of an issue is always a bit tricky, but I think if we split this up, it will be easier to commit parts in bits and pieces rather then the whole thing.

Patch #58 and #54 are not working .
I got an error

error: patch failed: src/Controller/GroupContentMenuController.php:184
error: src/Controller/GroupContentMenuController.php: patch does not apply
error: patch failed: src/Entity/GroupContentMenu.php:44
error: src/Entity/GroupContentMenu.php: patch does not apply
error: patch failed: src/Form/GroupContentMenuForm.php:293
error: src/Form/GroupContentMenuForm.php: patch does not apply
error: patch failed: src/Plugin/GroupContentMenuPermissionProvider.php:32
error: src/Plugin/GroupContentMenuPermissionProvider.php: patch does not apply
error: patch failed: src/Routing/GroupContentMenuRouteProvider.php:25
error: src/Routing/GroupContentMenuRouteProvider.php: patch does not apply

Rerolled patch #54 in 8.x-1.x version .

I needed a working patch for dev-1.x ab0e72f2, because afterwards, backwards compatibility to PHP 7.4 has silently been dropped on this branch (I should create a separated issue to address this). Also, the changes are a lot more significant and possibly breaking other things.

The reroll of #54 isn't much help, as @nielsva clearly stated he made a mistake when uploading it. Important pieces are missing in this patch.

#67 is a reroll of #58 based off commit ab0e72f2 just before GroupContentMenuStorage.php is introduced. It also addresses some of @heddn's comments, but not all.

Opened #3323420: Restore PHP 7 compatibility and a MR with a re-roll of #67 instead.

Since #3321521: Move group menu permission to yml file, since we don't have it by bundle landed, we should re-roll this here.

As Drupal 10 is out and group module advises version 3.x I needed this functionality of having translate menu items.
I'm attaching a patch which is applied on top of the latest 3.0.x branch containing the rework supporting group 3.x : https://git.drupalcode.org/project/group_content_menu/-/tree/3.0.x

Hopefully this can help anyone else.

Seems like there are some test failures? Back to NW.

I modified entity annotation links of @kevin.vanbelle@skynet.be patch

Patch on #73 is missing the new files and doesn't apply. Neither did #71 so I have recreated a patch from these that applies to current 3.0.x-dev

Reroll of #58 against the latest 1.x version

Last patch for 1.x from @carsoncho gives me the error:

In EntityResolverManager.php line 135:

  Class "\Drupal\group_content_menu\Controller\GroupContentTranslationController" does not exist

Maybe it's not related but when I'm trying to translate a menu link, I get:

Warning: Undefined array key 1 in docroot/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php on line 114
The website encountered an unexpected error. Please try again later.
TypeError: Drupal\simple_sitemap\Entity\EntityHelper::getEntityBundle(): Return value must be of type string, null returned in Drupal\simple_sitemap\Entity\EntityHelper->getEntityBundle() (line 124 of modules/contrib/simple_sitemap/src/Entity/EntityHelper.php).

I'm on drupal 9.5, simple_sitemap 4.1 and group_content_menu 1.2

I have rebased the MR but it doesn't apply on 8.x-1.2 but only on 8.x-1.x so I have reroll the patch for 8.x-1.2

A lot of changes have been added to the delete functionality, so I prioritized what was done already instead of some changes I saw here.

I still get the same error from simple_sitemap as my previous comment #78
I just noticed that this error happens when I'm editing a 2nd child menu item but it doesn't happen when I'm editing a menu item on the menu root.
I also noticed that on the root menu item form there's a field for "Parent (all languages)" and it's not there for child items.

Ok, that's pretty lame, but there was a duplicated line setting parameters on GroupContentTranslationController causing all this.
One line... I'm submitting another patch with that fixed.

Hello!

I wanted to help contribute a patch that I was able to write. It's for the 2.x branch of the module (this is the version our customer is moving onto from 1.x) but I based the start of it all from the patch on #80.

This patch is able to get the UI functionality for letting the administrative user be able to have the Translate links in the Operations dropdown for both the Group menu and Group menu links, and help avoid the WSOD errors that I was seeing when initially trying to re-roll the patch from #80 onto the base of the 2.x branch.

The only thing that needs work on, is that in the GroupContentMenuForm.php, I had to remove the $link->isTranslatable() check because when enabling the Group Content Menu's as translatable content, it's enabling it for the entity of group_content_menu but not for the links (and I was not seeing any way to be able to see how to make it enabled). I'm hoping someone might be able to expand on my work here and be able to include this.

The issue fork is currently mixed up, some classes got duplicated code due to merge conflicts between the major branches 3.x and 8.x-1.x.

Here is an updated patch for 8.x-1.x.

I don't think the issue summary is accurate any longer. The high-level permission is not longer needed. Or is it? Can we verify?

I'll do the IS update shortly and I'm working on some updates. The code in question is a good starting point, but it needs some dusting off.

Tests are passing green. For those using an older version of this patch, I changed the name of the translation permission to make it more English phonetic. You'll need to re-build group permissions to use the latest version with the changed permission name. But otherwise, the latest version shrink the number of changes to a much more manageable lines of changes. Sadly, still needs tests. But manual testing, things work.

Tests added. I just need someone, anyone to chime in that this fixes the issue for _your_ site.

Thanks everyone for your contributions here. I'm pretty sure we might have missed something in this massive effort. But... we can fix that in follow-ups. LGTM.

Actually, this doesn't merge cleanly into 2.x. Re-opening for someone to re-base.

Resolved the conflicts.