Drupal 8 version

+1

+1

+1

Would be great to have a D8 Version.

There's interest here!

Hi there! My organization has a need for a D8 version of this module so I've been working on building it, unaware there was discussion here about a D8 version. I'm happy to share my code but I need about 24 hours to wrap up the piece I'm currently working on.

@mike.davis Awesome! I'll share it with you at some point tomorrow! I've delved pretty deep into how Group 8 works and it's plugin system. The biggest problem I had with development boiled down to this issue: https://www.drupal.org/node/2346347 Essentially we can't just reference a menu using the recommended GroupContent method, so I had to make an interesting workaround (you'll see what I'm talking about when I share the code). But in the end it still uses the same plugin system that would seem very familiar to functionality presented by the gnode submodule.

I like the idea of storing all the links in one menu, but I see one kind of big problem off the bat -I know for a fact that I have a couple of odd-ball situations where I need to access the menu block for a group menu outside of the group context. You'd also have the same problem with menu links that you do with menus -when a global admin looks at the menu link list for the one group menu, there would be an absurd number of menu links. I'm not sure if this particular situation is better or worse than having a lot of menus. I'm curious if we can do something similar to what og_menu did where we simply have a setting that allows us to hide the giant list of group menus on menu lists.

Hi Jack!

As Mike and I discussed during our coder lounge, we'd approach Group Menu 8 using one menu for all links. The reason behind it is that Group 8 does not support the grouping of config entities (yet).

After careful consideration, it's become clear that we should not treat config entities like content entities. They serve vastly different purposes and to treat them as equals would be setting ourselves up for a fall. So to that end, we may one day have a GroupConfigEnabler plugin and GroupConfig / GroupConfigType entity types. But for now, that's out of scope for a 1.0 release.

About the approach for GM8: We chose for one menu because we can have a simple UX that way.

• We would hide the one menu from the general menu UI
• We would then show the same menu UI people are used to per group, but filtered by links for that group
• When showing the menu in the front end, the same would apply: We'd only show links that are part of that group

The only downside we could think of so far is performance on large sites. But that should be fixed once we get entity grants into core.

Hi Mike and Kristiaan,

So I've posted my code at https://github.com/awalacabab/group_menu. I do realize that I've called it "group_menu" instead of "groupmenu" but haven't had time to fix that yet. The big issues I have yet to fix are:

• Setting up Group access to edit menu links. What I have so far is about associating menus with a group and creating a menu in the group, but the rest should be pretty straightforward.
• Hiding group menus from menu lists.

Take a look at my code and maybe try installing it. It's activated under the "content" tab of the group type management, like a content plugin.

I'm about to run into a meeting, but have some more thoughts about the one-menu-to-rule-them-all approach that I'll share with you later.

I'm going to be needing this module in the next month. Even just an alpha version as long as the basics are there. I started today on getting the Group menus exposed on the node edit form: https://github.com/awalacabab/group_menu/pull/1

My 2 cents is that individual menus seems the better approach but I really don't have any insight deeper than that :)

Having a dev/alpha would definitely help people installing, testing and adding patches. So +1 for that :)

We're going live in the next few days with a groups based site as a proof-of-concept with 2 groups. Hoping to have a follow-up in the next year with about 300 groups (each group a sub-site of a major university).

At the moment we have two individual menus - one for each group - and the process of adding links to them is a little cumbersome - plus all editors then have access to all menus.

It would be great to see this module reaching maturity for our use case. Thanks for the work done so far.

Sorry about the dead air. I have a lot of time this week to try and get an alpha version ready. Will keep y'all posted.

Is it even possible to go forward with the multiple menus approach? Menus are configuration, so on a production site content editors are able to generate new configuration all the time, which might be deleted during a deployment and drush configuration-import?

Hi @mErilainen, I'm actually taking care of this issue by setting the list_builder handler of menu entities to use a modified version of Drupal\menu_ui\MenuListBuilder. We need to do this anyway to exclude group menus from system menu lists. If I am correct, updating this should also take care of making sure that group menu entities do not get pulled in when doing configuration exports. I should have this on my github repository tomorrow.

You bring up a good point though -we'll have to make sure that group menus don't get overwritten on deployments. I don't think this should be hard to deal with either. I suspect that it's just a matter of implementing hook_config_import_steps_alter, although I haven't looked much into this issue yet.

Another thing crawled to my brain while sleeping: Bundle needs to have certain menus enabled so that entities can be placed into the menu. Is there a way to make it dynamically, otherwise that configuration is going to be overwritten also and a "site-builder" is required to configure content type(s) when someone creates a new group.

Okay, so an update for everyone: I have something that mostly works right now on my github repository.

Here's what's left:

• Create a view that lists group menus and links to the "edit" and "delete" forms.
• Unfortunately, just modifying the Drupal\menu_ui\MenuListBuilder class did not solve all menu list loading problems as expected. (It did solve some though!) In particular, we still need to:
  • Stop group menus from being exported when using core's configuration synchronization.
  • Hold onto group menus when doing a configuration import. I think it's a matter of properly implementing hook_config_import_steps_alter.
  • Potentially modify admin_toolbar functionality to not list all group menus.
  • Add a setting that controls the hiding/showing of group menus in menu lists in places like the menu overview page and the adding a block page. If I recall, a similar setting is available in the og_menu module for Drupal 7.
• Investigate @mErilainen's concern.
• Rename all occurrences of "group_menu" to "groupmenu". (This is my fault; I didn't realize this project existed before I started work on the code. I'll try to take care of this ASAP.).
• I'd also love to have someone intimately review my code at some point. I'm sure there are some places that could use improvement.
• Fix any bugs people find/can think of.

I may have some time to work on these things this weekend, and probably mid next week as well.

I think that the one menu vs many menus debate should still be had. I do feel like there have been more workarounds in my code than I expected due to the fact that menus are configuration entities. I don't know if this is necessarily a bad thing or if the pros outweigh the cons. I would, however, like to finish my code before I participate in a discussion about whether or not we should go the one menu route; I do need something working soon for my organization so even if the full release for this module goes a different way, I can convert later on.

Thank you for working on this, we will most likely need this in a project which is also finishing quite soon. But to me it seems that all problems are due to having a menu per group instead a single "group menu". I'm afraid the community will not be very helpful to make this work for you quickly, especially when the maintainer has indicated that one menu solution would be recommended https://www.drupal.org/node/2716443#comment-11907501 and it makes more sense to me too.

I guess the only concern with the one menu approach is the performance issues, which can be solved quite easily with a functionality similar to Big menu on the admin UI https://www.drupal.org/project/bigmenu

Well, again, I'm developing this mostly for the benefit of my organization and am sharing my code for the benefit of the community. (I've actually kind of become personally invested in the development of this module as well.) If people want to use what I wrote, they can. If they don't, they can wait until this module gets a release. The same thing goes for the module maintainer; maybe he'll see after my code is ready that the route I took has some advantages over his original idea, maybe not.

All I know is that at a first glance the one menu approach does not seem like it will work for my organization without extra modification. There are a couple odd-ball instances where I need to reference a group menu outside of the group context, but those menus still need to be editable by group members. I suppose a workaround in a one-menu approach would be to have some setting in the group menu block that allows you to override the context-specific rendering of menu links. For example, for a group named "foo" you can tell the block to render the "foo" group menu(s) regardless of context.

The other situation that seems tricky in the one menu approach is that some of my groups have multiple menus, mainly for aesthetics. The groups that do are very critical and are some of the most high traffic areas on my site so if I took away the multiple menus I'd definitely take some heat. I'm curious, though, if we can find a workaround for this as well. I'm thinking we would almost have to, regardless of my personal requirements, have some system in place that would allow for a menu-like configuration. By that I mean, it's not applicable to my site (or many sites, I think) to have all of the group menus have same same title. So for this requirement we'd essentially have to create a custom content entity that has it's own title and then use the group content relater to reference this entity from the group menu links. This is very analogous to what my code is doing now except that we'd be avoiding a bunch of configuration entities.

Sorry for the long post but now that I've thought it through a little more, I'd be very curious to see what @mike.davis thinks. I'd be happy to help write code to go either way. As my current multiple-menu approach code is further along, the advantage there would be that we could probably have an actual alpha release by the end of the week. If we go the other way, it'll probably take me 1-2 weeks to get something working enough for alpha. I've also made a chart for my own benefit summarizing the different approaches that I've attached.

Thanks for the work Jane, it's coming along. I made another couple pull requests and I'll continue to poke at it. I'll probably look at the config management aspect next.

Thanks for your help as well, Eric! I really appreciate it. The configuration management is being a bit of a pain. I was trying to get away with just implementing hook_query_alter() but it looks like there is no alter tag for the resulting query from Entity::loadMultiple().

I'm thinking that changing the handler class for "storage" in the menu entity type would do the trick. We could point it to a class inherited from ConfigEntityStorage and override the doLoadMultiple() method. Let me know if another method seems like it would also work and/or be better.

I'm in favor of adding multiple "normal" menu's in stead of working around what core already provides.

Stop group menus from being exported when using core's configuration synchronization.
Hold onto group menus when doing a configuration import. I think it's a matter of properly implementing hook_config_import_steps_alter.

Core has the same problem with custom blocks, and webform 5.x has the same issue with webforms being config entities. I think that's why solutions like config split / config_ignore / CMI tools are being developed.

Could we post a patch here for review? It's easier to go trough a patch and provide an extensive review to discuss it first (without opening a bunch of pull requests). I could make some time to do a review.

As I mentioned before, starting a dev branch and working with the Drupal issue queue also helps getting more contributors to test, write patches etc. Committing patches and giving credits on d.o. is important for a lot of organizations. Just mentioning this again hoping it helps :)

Just posting a quickly whipped up patch for review...

Here is a first review of the module. I still need to play with it a little more to fully grasp all the concepts and internals, so I only reviewed the code at a higher level for now.

I'll try to follow up with a second review asap, but this could take a couple of days.

1. --- /dev/null
   +++ b/composer.json
   
   +++ b/composer.json
   @@ -0,0 +1,6 @@
   
   @@ -0,0 +1,6 @@
   +{
   +  "name": "unlcms/group_menu",
   +  "description": "Gives the ability to create and manage menus for groups",
   +  "type": "drupal-module",
   +  "license": "GPL-2.0+"
   +}
   

   We can remove the composer.json

2. +++ b/composer.json
   --- /dev/null
   +++ b/group_menu.group.permissions.yml
   
   +++ b/group_menu.group.permissions.yml
   +++ b/group_menu.group.permissions.yml
   @@ -0,0 +1,3 @@
   
   @@ -0,0 +1,3 @@
   +access group_menu overview:
   +  title: 'Access group menu overview'
   +  description: 'Access the overview of all group menus'
   

   I don't see this actually being used? Is this still a TODO?

3. +++ b/group_menu.group.permissions.yml
   --- /dev/null
   +++ b/group_menu.info.yml
   
   +++ b/group_menu.info.yml
   +++ b/group_menu.info.yml
   @@ -0,0 +1,9 @@
   
   @@ -0,0 +1,9 @@
   +name: Group Menu
   +description: Gives the ability to create and manage menus for groups
   +package: Group
   +
   +dependencies:
   + - group:group
   +
   +type: module
   +core: 8.x
   \ No newline at end of file
   

   The dependency can be just 'group'.
   No newline at end of file. Also we might want to change the order a bit. Most of core is using name/type/description/package/core/dependencies without newlines between them.

4. +++ b/group_menu.info.yml
   --- /dev/null
   +++ b/group_menu.install
   
   +++ b/group_menu.install
   +++ b/group_menu.install
   @@ -0,0 +1,8 @@
   
   @@ -0,0 +1,8 @@
   +<?php
   +
   +/**
   + * Implements hook_install().
   + */
   +function group_menu_install() {
   +  module_set_weight('group_menu', 1);
   +}
   

   Do we really need this? If so, it might be nice to document why.

5. +++ b/group_menu.links.action.yml
   @@ -0,0 +1,18 @@
   +    - 'view.group_menus.page_1'
   

   This views seems to be missing?

6. +++ b/group_menu.module
   @@ -0,0 +1,157 @@
   + * Enables Group functionality for the Menu module.
   

   I think this is the other way. Enables menu functionality for group module.

7. +++ b/group_menu.module
   @@ -0,0 +1,157 @@
   +use Drupal\group\Entity\Group;
   +use Drupal\group\Entity\GroupContentType;
   +use Drupal\group_menu\Entity\GroupMenu;
   +use Drupal\system\MenuInterface;
   +use Drupal\Core\Access\AccessResult;
   +use Drupal\Core\Database\Database;
   +use Drupal\Core\Database\Query\AlterableInterface;
   +use Drupal\Core\Form\FormStateInterface;
   +use Drupal\Core\Session\AccountInterface;
   +use Drupal\Core\Url;
   +use Drupal\menu_link_content\Entity\MenuLinkContent;
   

   Some statements are unused.

8. +++ b/group_menu.module
   @@ -0,0 +1,157 @@
   +// This doesn't work to override menu_ui_form_node_type_form_alter
   ...
   +// Instead, setting module weight with hook_install for now.
   +//
   +//function group_menu_module_implements_alter(&$implementations, $hook) {
   +//  if ($hook == 'form_node_type_form_alter') {
   +//    $group = $implementations['group_menu'];
   +//    unset($implementations['group_menu']);
   +//    $implementations['group_menu'] = $group;
   +//  }
   +//}
   

   Hmm, https://www.drupal.org/node/765860 seems to say this was committed to D8?

9. +++ b/group_menu.routing.yml
   @@ -0,0 +1,60 @@
   +    _group_menu_add_access: 'TRUE'
   ...
   +    _group_menu_edit_access: 'TRUE'
   ...
   +    _group_menu_delete_access: 'TRUE'
   ...
   +    _group_menu_edit_access: 'TRUE'
   ...
   +    _group_menu_link_edit_access: 'TRUE'
   ...
   +    _group_menu_link_edit_access: 'TRUE'
   ...
   +    _group_menu_link_edit_access: 'TRUE'
   

   Can we use a generic access service in the form of _group_menu_access: 'permission name' to make it a little more flexible?

10. +++ b/group_menu.services.yml
    @@ -0,0 +1,17 @@
    +services:
    +  access_check.group_menu.add:
    +    class: Drupal\group_menu\Access\GroupMenuAddAccessCheck
    +    tags:
    +      - { name: access_check, applies_to: _group_menu_add_access }
    +  access_check.group_menu.edit:
    +    class: Drupal\group_menu\Access\GroupMenuEditAccessCheck
    +    tags:
    +      - { name: access_check, applies_to: _group_menu_edit_access }
    +  access_check.group_menu.delete:
    +    class: Drupal\group_menu\Access\GroupMenuDeleteAccessCheck
    +    tags:
    +      - { name: access_check, applies_to: _group_menu_delete_access }
    +  access_check.group_menu.menu_link.edit:
    +    class: Drupal\group_menu\Access\GroupMenuLinkEditAccessCheck
    +    tags:
    +      - { name: access_check, applies_to: _group_menu_link_edit_access }
    

    See previous point.

11. +++ b/group_menu.services.yml
    @@ -0,0 +1,17 @@
    \ No newline at end of file
    

    Nit

12. +++ b/group_menu.services.yml
    --- /dev/null
    +++ b/modules/group_menu_block/group_menu_block.info.yml
    
    +++ b/modules/group_menu_block/group_menu_block.info.yml
    +++ b/modules/group_menu_block/group_menu_block.info.yml
    @@ -0,0 +1,10 @@
    
    @@ -0,0 +1,10 @@
    +name: Group Menu Block
    +description: Provides a block to display group menus
    +package: Group
    +
    +dependencies:
    + - 'gnode'
    + - 'group_menu'
    +
    +type: module
    +core: 8.x
    \ No newline at end of file
    

    Same comments as the other info file.

13. +++ b/modules/group_menu_block/group_menu_block.module
    @@ -0,0 +1,31 @@
    \ No newline at end of file
    

    Nit

14. +++ b/modules/group_menu_block/group_menu_block.module
    --- /dev/null
    +++ b/modules/group_menu_block/src/Plugin/Block/GroupMenuBlock.php
    
    +++ b/modules/group_menu_block/src/Plugin/Block/GroupMenuBlock.php
    @@ -0,0 +1,93 @@
    +   * Gets group menu names from group ID
    ...
    +   *   The group ID you want to load menus for
    ...
    +   *   Array of machine names for group
    ...
    +    foreach($group_menus as $group_menu) {
    ...
    +    // Get the associated group content for the current node
    ...
    +      // For each group this node belongs to...
    +      foreach($group_contents as $group_content) {
    ...
    +        // ...make an array of menus to render...
    ...
    +    } else {
    +      // Not on a node page, see if we can get the group
    ...
    +      } else {
    ...
    +    // ...and render them
    

    A bunch of coding standard issues.

15. +++ b/modules/group_menu_block/src/Plugin/Block/GroupMenuBlock.php
    @@ -0,0 +1,93 @@
    +use Drupal\group\Entity\Group;
    ...
    +use Drupal\Core\Cache\Cache;
    

    Unused use statements.

16. +++ b/modules/group_menu_block/src/Plugin/Block/GroupMenuBlock.php
    @@ -0,0 +1,93 @@
    +    $menus = array();
    

    I think [] is preferred now. Use this for all arrays.

17. +++ b/modules/group_menu_block/src/Plugin/Block/GroupMenuBlock.php
    @@ -0,0 +1,93 @@
    \ No newline at end of file
    

    Nit

18. +++ b/modules/group_menu_block/src/Plugin/Block/GroupMenuBlock.php
    --- /dev/null
    +++ b/src/Access/GroupMenuAddAccessCheck.php
    
    +++ b/src/Access/GroupMenuAddAccessCheck.php
    --- /dev/null
    +++ b/src/Access/GroupMenuDeleteAccessCheck.php
    
    +++ b/src/Access/GroupMenuDeleteAccessCheck.php
    --- /dev/null
    +++ b/src/Access/GroupMenuEditAccessCheck.php
    
    +++ b/src/Access/GroupMenuEditAccessCheck.php
    --- /dev/null
    +++ b/src/Access/GroupMenuLinkEditAccessCheck.php
    

    This could be 1 service as mentioned before.

19. +++ b/src/Controller/GroupMenuController.php
    @@ -0,0 +1,80 @@
    +  protected function addPageBundles(GroupInterface $group) {
    

    Missing argument.

20. +++ b/src/Controller/GroupMenuController.php
    @@ -0,0 +1,80 @@
    +    ¶
    

    Nit: Whitespace.

21. +++ b/src/Controller/GroupMenuController.php
    @@ -0,0 +1,80 @@
    +  protected function addPageBundleMessage(GroupInterface $group) {
    

    Missing argument.

22. +++ b/src/Controller/GroupMenuEditController.php
    @@ -0,0 +1,40 @@
    +class GroupMenuEditController extends ControllerBase {
    ...
    +  public static function create(ContainerInterface $container) {
    ...
    +  public function edit(MenuInterface $menu) {
    

    Missing PHP docs.

23. +++ b/src/Controller/GroupMenuEditController.php
    @@ -0,0 +1,40 @@
    +    return array(
    +      '#type' => 'markup',
    +      '#markup' => $this->t('This is a test'),
    +    );
    

    This is never executed.

24. +++ b/src/Controller/GroupMenuEditController.php
    @@ -0,0 +1,40 @@
    \ No newline at end of file
    

    Nit

25. +++ b/src/Controller/GroupMenuWizardController.php
    @@ -0,0 +1,212 @@
    +   * @return array
    +   *   The group menu creation overview page.
    ...
    +  public function addPage(GroupInterface $group) {
    ...
    +      return new RedirectResponse($url->toString());
    

    The return doc doesn't contain RedirectResponse.

26. +++ b/src/Controller/GroupMenuWizardController.php
    @@ -0,0 +1,212 @@
    +    $storage_handler = $this->entityTypeManager->getStorage('menu');
    ...
    +      $plugin = $group_content_type->getContentPlugin();
    

    Unused variables.

27. +++ b/src/Controller/GroupMenuWizardController.php
    @@ -0,0 +1,212 @@
    +      //$bundle_label = $storage_handler->load($plugin->getEntityBundle())->label();
    

    Remove?

28. +++ b/src/Entity/GroupMenu.php
    @@ -0,0 +1,124 @@
    +use Drupal\Core\Entity\ContentEntityInterface;
    +use Drupal\group\Entity\GroupContentInterface;
    +use Drupal\group\Entity\GroupContent;
    +use Drupal\group\Entity\GroupContentType;
    +use Drupal\user\UserInterface;
    +use Drupal\Core\Field\BaseFieldDefinition;
    +use Drupal\Core\Entity\ContentEntityBase;
    +use Drupal\Core\Entity\EntityTypeInterface;
    +use Drupal\Core\Entity\EntityChangedTrait;
    +use Drupal\Core\Entity\EntityStorageInterface;
    +use Drupal\system\MenuInterface;
    

    Unused use statements.

29. +++ b/src/Entity/GroupMenu.php
    @@ -0,0 +1,124 @@
    + *   label_singular = @Translation("group menu item"),
    + *   label_plural = @Translation("group menu items"),
    ...
    + *     singular = "@count group menu item",
    + *     plural = "@count group menu items"
    

    I think we are storing menu's, not items. I suggest losing 'item'.

30. +++ b/src/Entity/GroupMenu.php
    @@ -0,0 +1,124 @@
    + *   bundle_label = @Translation("Group content type"),
    

    Isn't this 'Group menu type'?

31. +++ b/src/Entity/GroupMenu.php
    @@ -0,0 +1,124 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function loadByMenu(MenuInterface $menu) {
    

    There are no docs to inherit. Which leads me to the following. Are group menus actually Group Content? If not, why extend Group content? We should at least add our own interface.

32. +++ b/src/Entity/GroupMenu.php
    @@ -0,0 +1,124 @@
    +    ]);
    

    Nit: Indentation error.

33. +++ b/src/Form/GroupMenuForm.php
    @@ -0,0 +1,25 @@
    +  ¶
    ...
    +} ¶
    \ No newline at end of file
    

    Nit: Whitespace errors, missing newline.

34. +++ b/src/Form/GroupMenuForm.php
    @@ -0,0 +1,25 @@
    +  /*
    

    Nit: /**

35. +++ b/src/Form/GroupMenuFormStep1.php
    @@ -0,0 +1,118 @@
    +use Drupal\Core\Entity\EntityManagerInterface;
    

    Unused use statement.

36. +++ b/src/Form/GroupMenuFormStep1.php
    @@ -0,0 +1,118 @@
    +  public function __construct(QueryFactory $entity_query_factory, MenuLinkManagerInterface $menu_link_manager, ¶
    +MenuLinkTreeInterface $menu_tree, LinkGeneratorInterface $link_generator, PrivateTempStoreFactory $temp_store_factory) {
    ...
    +      $container->get('entity.query'), ¶
    +      $container->get('plugin.manager.menu.link'), ¶
    +      $container->get('menu.link_tree'), ¶
    

    Whitespace / newline issues.

37. +++ b/src/Form/GroupMenuFormStep1.php
    @@ -0,0 +1,118 @@
    +   * @see \Drupal\gnode\Form\GroupNodeFormStep2
    

    Undefined namespace \Drupal\gnode\Form.

38. +++ b/src/Form/GroupMenuFormStep2.php
    @@ -0,0 +1,112 @@
    +  /**
    +   * Constructs a GroupNodeFormStep2 object.
    +   *
    +   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
    +   *   The entity manager.
    +   * @param \Drupal\user\PrivateTempStoreFactory $temp_store_factory
    +   *   The factory for the temp store object.
    +   */
    +  public function __construct(EntityManagerInterface $entity_manager, PrivateTempStoreFactory $temp_store_factory) {
    +    parent::__construct($temp_store_factory, $entity_manager);
    ...
    +    );
    

    It would be nice to keep the arguments in the same order as the parent.

39. +++ b/src/Form/GroupMenuFormStep2.php
    @@ -0,0 +1,112 @@
    +   * @see \Drupal\gnode\Form\GroupNodeFormStep1
    

    Missing namespace again.

40. +++ b/src/MenuListBuilder.php
    @@ -0,0 +1,31 @@
    +class MenuListBuilder extends \Drupal\menu_ui\MenuListBuilder {
    

    Reference interface through use statement. Also I suggest renaming MenuListBuilder to GroupMenuListBuilder to make it more clear we are overriding the default (we need this for the use statement as well). Naming a class the same as the one you are extending is generally a bad idea.

41. +++ b/src/MenuListBuilder.php
    @@ -0,0 +1,31 @@
    +    ->condition($this->entityType->getKey('id'),
    +      db_select('group_menu_field_data', 'gm')
    +        ->fields('gm', array('entity_id'))
    +        ->execute()
    +        ->fetchAll(\PDO::FETCH_COLUMN, 0),
    +      'NOT IN')
    +    ->sort($this->entityType->getKey('id'));
    +    ¶
    ...
    \ No newline at end of file
    

    Indentation, whitspace, newline issues.

42. +++ b/src/Plugin/GroupContentEnabler/GroupMenu.php
    @@ -0,0 +1,107 @@
    +use Symfony\Component\Routing\Route;
    

    Unused use statement.

43. +++ b/src/Plugin/GroupContentEnabler/GroupMenu.php
    @@ -0,0 +1,107 @@
    +  public function getPermissions() {
    

    Shouldn't we be able to delete menu's as well?

44. +++ b/src/Plugin/GroupContentEnabler/GroupMenu.php
    @@ -0,0 +1,107 @@
    + *   description = @Translation("Adds menus to groups both publicly and privately."),
    

    What does 'both publicly and privately' mean here? Can't we just remove this (since public and private basically equal everything)? Or do you get a separate public / private menu (2 menu's)?

45. +++ b/src/Plugin/GroupContentEnabler/GroupMenuDeriver.php
    @@ -0,0 +1,22 @@
    +class GroupMenuDeriver extends DeriverBase {
    

    Missing class doc.

46. +++ b/src/Plugin/GroupContentEnabler/GroupMenuDeriver.php
    @@ -0,0 +1,22 @@
    +   * {@inheritdoc}.
    

    No dot at the end.

Wow @seanB. This is amazing. I cannot say thank you enough for all your help! I'll put these changes into the code at some point today.

Jane

I've implemented most of the suggested changes for @seanB's first code review. What's left is pretty simple but requires some more involved code changes. Here are my notes when I was going through his list.

"fixed" just means I did as suggested.

1. fixed
2. still todo
3. fixed
4. fixed (removed)
5. still todo (with 2)
6. fixed
7. fixed
8. fixed (removed)
9. Should be easy, but I'm making this a todo
10. Same as 9
11. fixed
12. fixed
13. fixed
14. fixed (I think?)
15. fixed
16. fixed
17. fixed
18. Same as 9
19. fixed
20. fixed
21. fixed
22. fixed
23. fixed (removed)
24. fixed
25. fixed
26. fixed
27. fixed
28. fixed
29. fixed
30. fixed
31. Half fixed. Second part is todo. I'll also change the return type once finished.
32. fixed (I think?)
33. fixed
34. fixed
35. fixed
36. fixed
37. fixed
38. fixed
39. fixed
40. fixed
41. fixed
42. fixed
43. fixed
44. fixed
45. fixed
46. fixed

Hi @janebechdol,

thanks for your work so far! I'm joining in as a contributor. @seanB and I need this module pretty soon and thus we prefer to have an official Drupal release instead of a github version. In this way more people will join in and contribute too. Currently I am adding fixes to the group_menu module on my local environment.

Would it be possible for you to chat on some IRC drupal channel anytime soon? I want you to submit this module because you deserve the credits. Otherwise, if you don't mind, I would like to submit and maintain the module.

Please contact me so we can speed up a release candidate.

ps. I also renamed the module to 'groupmenu' as discussed previously.

Here is a first updated patch for review.

*edit Never mind the interdiff because of the renaming.

Other than the renaming, is there anything new in the patch other than the addition of config/optional/views.view.group_menus.yml ?

As this is group content, I think you can leave it up to the site administrators to create a menu for each group (maybe a extra option to do it automatically?). The performance should not be a big issue, the webform module has a test for this (see the discussion in #2828461: Change config entity to content entity and the test in http://cgit.drupalcode.org/webform/commit/?id=f82477e). This should be fine. The same goes for the configuration issues. Webform has the same issues. I think using config_split and/or config_ignore can solve these issues if you want to (most advanced sites use these modules anyway to deal with block_content and webform issues).

I agree on removing the update hooks!

There is one big issue I've noticed. There is a extra GroupMenu entity to deal with a limitation in the group modules #2797793: Entities identified by strings as group content. Right now we store a relation like this:
Group > Group Content > Group Menu > Menu

It should be:
Group > Group Content > Menu

That extra entity causes some issues and seems very confusing to me. I guess #2797793: Entities identified by strings as group content is a blocker to do this right.

I've worked on a new version of the module based on #2797793: Entities identified by strings as group content. It is basically a rewrite, so an interdiff wouldn't make a lot of sense. Patch is attached for review, also created a sandbox for this. https://www.drupal.org/sandbox/seanpenn079/2868316

What was changed:

• Complete rewrite (I did use the old patch as a reference, so janebechdol thanks for sharing your work!)
• Based patch on #2797793: Entities identified by strings as group content
• Removed config export code since it doesn't work with drush. Config for menu's should be handled the same as for webforms. See https://blog.liip.ch/archive/2017/04/07/advanced-drupal-8-cmi-workflows.... for a good workflow. We should create a followup for when #2830300: Allow development modules to opt out of config-export is fixed.
• Reimplemented menu access. We now have a routesubscriber to add custom access for menu related routes.
• Added hooks for the menu related forms (node form / menu form / menu link form / menu link content form) to show only the menu's for the groups you have access to.
• Allow group menu's per group node content plugin config so you can control which node types should have the possibility to be added to group menu's when creating a node in a group.

Please review!

Thanks for the efforts here. As someone with no experience or understanding of the inner workings of Group, I must first say I don't like the idea of having all menu links live in a single menu.

I tried the patch in #39 and was unable to install it. The error is below.
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "entity_type.manager", path: "plugin.cache_clearer -> entity_type.manager -> string_translation -> string_translator.locale.lookup -> config.factory -> groupmenu.config_overrides". in Drupal\Component\DependencyInjection\Container->get() (line 147 of /var/www/html/core/lib/Drupal/Component/DependencyInjection/Container.php).

I do have translation modules enabled so I wonder if it's related to that.

I also tried #35 and was able to install it and start configuring it, but couldn't work out where to add menu links. There is a link to /group/1/menu/1 but it gives me a 403 Access Denied even though I'm User 1.

Hope this helps.

Please take a look at the latest version in the sandbox: https://www.drupal.org/sandbox/seanpenn079/2868316
The issue should be fixed in there.

Thanks seanB, I tried out the sandbox and I can a bit further. I installed the group menu content plugin for my group type and tried to create a group menu via the "Related entities" tab. It seemed to work, but now when I go back to Related entities the menu is not there. I then tried to "relate an existing entity" to the menu, and now I get this error message:

Error: Call to a member function label() on null in Drupal\group\Plugin\GroupContentEnablerBase->getContentLabel() (line 141 of /var/www/html/modules/group/src/Plugin/GroupContentEnablerBase.php) #0 /var/www/html/modules/group/src/Entity/GroupContent.php(143): Drupal\group\Plugin\GroupContentEnablerBase->getContentLabel(Object(Drupal\group\Entity\GroupContent))

mstrelan: you need this patch for Group: https://www.drupal.org/node/2797793#comment-12061652

Looks good seanB. One issue I'm seeing is that if I am a site administrator and have the 'administer menu' permission, I don't get the group menus on the node edit form. I struggled to get it working on Jane's repo and kind of ended up with a mess. Your code looks cleaner but I haven't been able to pinpoint a fix yet.

Edit: Looks like if I'm merely a group editor, I don't get the group menu on the node edit form either.

Thanks @ericras, the menu's on the node form are loaded from node type config. You have to enable menu's for a node type to be able to add content to a menu.
To fix this for the dynamicly added group menu's, I added the GroupMenuConfigOverrides class. In that class we check to which group menu's the user should have access, and it then adds them to the node type config dynamically. This works kind of like how the overrides work for config in settings.php.

This seems to work for me, so if you want to check what's going on for you, you should check in GroupMenuConfigOverrides.

Just confused myself since the menu's were not added for 1 content type. It seems I have implemented a checkbox on the installation form of a node. To enable menu's:

• Go to group types overview
• Go to 'Set available content' for a group type
• Make sure the menu plugin is installed
• Install a new node type or configure a existing one
• Make sure you check 'Add group menu's to node form'

In the latest version of the sandbox https://www.drupal.org/sandbox/seanpenn079/2868316 you can now find some performance enhancements. The loading of group content and menu's was not very efficient when you have thousands of groups. This should now be fixed.

Thank you, I will look into this shortly. It would help a lot if we get a official 8.x branch (testing the code, filing issues). Hopefully we can get this soon. I'm willing to help as a co-maintainer if needed?

It would be great to get a 8.x branch to break things out into individual issues.

I'm about to move forward with using this in production so thanks again seanB.

One new bug - I had to modify the foreach loop in GroupMenuBlock:build() as follows. $menus now has numerical keys instead of named keys.

  foreach ($menus as $menu) {
      $menu_name = $menu->id();
      $tree = \Drupal::menuTree()->load($menu_name, $parameters);
      $tree = \Drupal::menuTree()->transform($tree, $manipulators);
      $build[] = \Drupal::menuTree()->build($tree);
    }

Other than that, it could just use some textual cleanup: all the instances of "menu's" (with the apostrophe) should just be "menus".

@ericras Changed the getGroupMenus() method in GroupMenuService to return menu's keyed by menu ID. Also replaced menu's with menus in code. You can pull a new version of the sandbox.

@forgenator This is only in the dev branch. Traced it to this commit: http://cgit.drupalcode.org/group/commit/?id=818977f
It seems the "new" createForEntityInGroup() method only accepts content entities. Menu's are config entities. We need to fix that in the patch in #2797793: Entities identified by strings as group content.

*edit: I updated the patch in #2797793: Entities identified by strings as group content. Could you test this?

No worries, I will add the latest changes from the sandbox. Guess this is fixed for now and we should continue in other issues if people find some.
Thank you.

I'm just retagging this as an 8.x issue so it shows up when filtering by 8.x issues.