"Parent item" is not set correctly on /node/%/edit if user does not have global Administer Menus permission

Thank you for spotting this. I have some remarks and questions, but the basic approach seems fine to me. When the node is in a lot of groups this could be an issue, but I guess when you edit a node it is logical to see all menu's of the groups the node belongs to.

1. +++ b/groupmenu.module
   @@ -97,16 +98,28 @@ function groupmenu_form_node_form_alter(&$form, FormStateInterface $form_state)
   +  $groups[] = $form_state->getStorage()['group'];
   +
   +  // If a node is being edited at /node/%/edit, look for groups that it is a part of.
   +  if (!$groups[0]) {
   +    unset($groups);
   +    $node = $form_state->getFormObject()->getEntity();
   +    if ($node->id()) {
   

   I guess we should always use the node groups on edit since a node can belong to multiple groups. If you edit in group B while the node is in the menu of group A, this would still be a problem I think? Maybe we could doe something like this:

     $group = $form_state->getStorage()['group'];
     $node = $form_state->getFormObject()->getEntity();
     // If a node is being edited at /node/%/edit, look for groups that it is a
     // part of, if not try to use the group ID from the URL.
     if ($node->id()) {
       $group_content_array = GroupContent::loadByEntity($node);
       foreach ($group_content_array as $group_content) {
         $groups[] = $group_content->getGroup();
       }
     }
     elseif ($group) {
       $groups[] = $group;
     }
   

2. +++ b/groupmenu.module
   @@ -97,16 +98,28 @@ function groupmenu_form_node_form_alter(&$form, FormStateInterface $form_state)
   +        $groups[] = $group_content->getGroup();
   

   This could be an issue when the node is added to a lot of groups. Can we just retrieve the IDs for each group from $group_content and doe a loadMultiple()? That would save us a bunch of queries.

3. +++ b/groupmenu.module
   @@ -120,16 +133,29 @@ function groupmenu_form_node_form_alter(&$form, FormStateInterface $form_state)
   - *   Optionally filter menus by group.
   

   We can typehint the param like this to document an array of group entities: @param \Drupal\group\Entity\GroupInterface[]

4. +++ b/groupmenu.module
   @@ -120,16 +133,29 @@ function groupmenu_form_node_form_alter(&$form, FormStateInterface $form_state)
   +    foreach($groups as $key => $group) {
   +      $allowed_menus_raw[$key] = $groupmenu_service->loadUserGroupMenusByGroup('edit', $group->id());
   

   I don't think we need to use $key here?

5. +++ b/groupmenu.module
   @@ -120,16 +133,29 @@ function groupmenu_form_node_form_alter(&$form, FormStateInterface $form_state)
   +    foreach ($allowed_menus_raw as $key => $value) {
   

   Same here, let's just remove $key.

6. +++ b/groupmenu.module
   @@ -120,16 +133,29 @@ function groupmenu_form_node_form_alter(&$form, FormStateInterface $form_state)
   +      if (is_array($value)) {
   

   We can remove the check, since loadUserGroupMenusByGroup() always returns an array.

#6.2 This should work: $group_content->gid->target_id.

Using magic may not be the proper way to do it, but it is readable and sure is a lot quicker for our use case. So I'd be ok with that. Could you change this? Will commit after this one.

Sorry, I was about to commit and realised something:

+++ b/groupmenu.module
@@ -94,19 +96,28 @@ function groupmenu_form_alter(&$form, FormStateInterface $form_state, $form_id)
+  if ($node->id()) {
+    $group_content_array = GroupContent::loadByEntity($node);
+    foreach ($group_content_array as $group_content) {
+      $groups[] = $group_content->gid->target_id;
+    }
+    $groups = Group::loadMultiple($groups);
...
+  if (isset($groups) || !$account->hasPermission('administer menu')) {

This code will return an empty array for $groups. Which means that $groups will return true for the isset() and will filter the enabled menu's even when a node is not placed in groups. I don't think we want that?

+++ b/groupmenu.module
@@ -94,19 +96,28 @@ function groupmenu_form_alter(&$form, FormStateInterface $form_state, $form_id)
+      $groups[] = $group_content->gid->target_id;

Small nit, the var will contain group IDs and not groups. We should rename this to $group_ids

I've updated the patch was suggested and added an extra check to prevent a notice when a node is not placed in groups.

This approach adds available menus that have not been enabled in the node type. As a result any node type that disabled all menus still has the 'Menu settings' in its node form. The menu ui module also returns a notice:

Notice: Undefined offset: 1 in Drupal\Core\Menu\MenuParentFormSelector->parentSelectElement() (line 91 of core/lib/Drupal/Core/Menu/MenuParentFormSelector.php).

Re #12.1:

This approach adds available menus that have not been enabled in the node type. As a result any node type that disabled all menus still has the 'Menu settings' in its node form.

Apparently this is a feature that can be configured per Group Content Plugin, so I left this as is.

#12.2

The menu ui module also returns a notice:

Notice: Undefined offset: 1 in Drupal\Core\Menu\MenuParentFormSelector->parentSelectElement() (line 91 of core/lib/Drupal/Core/Menu/MenuParentFormSelector.php).

Fixed the notice.

In addition I initialized the $groups variable early in groupmenu_form_node_form_alter() to prevent an exception when filtering the available items.

Thanks all! Committed and pushed to dev.