Add a list builder for OG Menu instances and replace the custom OverviewMenuInstances form

Thanks! Let's add some test coverage for this.

I tried this out on an installation with 3 OG Menu types. Since the titles of the menu instances are automatically taken from the group it is currently not possible to distinguish between the different menus that are associated with a single group.

We should add a column for the menu type.

+++ b/src/OgMenuInstanceListBuilder.php
@@ -0,0 +1,69 @@
+<?php
...
+  public function setContext(OgContextInterface $context) {
+    $this->context = $context;
+  }

Is there any reason this method is public? It is not part of the EntityListBuilderInterface.

It seems to me that this being set in the constructor should be sufficient, and the method can be made protected.

Are you perhaps planning on calling this from OG code? If this is the case we should add a new interface that contains this method.

OK thanks for the link, I understand the reason now why you propose to use this pattern. I have been pondering it for a while but I think I would prefer to still use the standard constructor based approach from core. I think having to update the factory method in the future is just a minor inconvenience, and it might not even be necessary during the life cycle of Drupal 8. I think the benefit of potentially avoiding a few minutes of work in the future is not a strong enough argument for violating the OO principles.

This pattern makes the class some kind of a mix of constructor based and setter based dependency injection. The correct way to handle this would be to use to provide a factory class which is a service in itself instead of a static factory method. Then your class can be protected against upstream changes and you avoid adding any public methods which are not intended to be part of the public interface. Unfortunately this is something that should be fixed in core itself. Using a factory service is a pattern that is supported by the Symfony dependency injection container and is already used in core in a few places, for example \Drupal\Core\Http\ClientFactory which has the non-static factory method fromOptions().

I don't know if there is some kind of initiative planned to move to service based factories in Drupal 9? Could be interesting to avoid these kind of issues.

Updated patch: it has an additional column for the menu type and the constructor based instance creation which I argued for in my previous comment. I am open for discussion to revert this back to the "safe" static-method-plus-public-setter based approach though if this is deemed to be very important. I am in any case willing to do the required work to update this in the future in case the constructor signature changes.

I also added an initial route for it so the list can be inspected at /admin/structure/ogmenu_instance. It doesn't have any tests yet. I probably want to get rid of this route again before merging the patch.

While working on this I noticed a few hairy issues in the module's code base which I want to address first. I would like to be able to show the human readable OG Menu label in the table rather than the machine name of the bundle, but this is not accessible directly from an OG Menu Instance entity.

What I also would love to have is if we could replace the existing lists at /admin/structure/menu/ogmenu/&ltmenu_name>/overview with this new fancy list builder. We would be able to leverage the OgContext based filter. We might need to write a custom OgGroupResolver plugin for these paths though.

Re-scoping to revive this instead of closing

Has kernel test and I think this picks up this really old issue