Entities identified by strings as group content

Hi there!

Currently Group only supports content entities because of the restriction of entity reference fields. I guess it would be possible to write an update to change this so we can allow menus to be added to groups as well. It used to be possible in Group 7, we just haven't built in support yet in D8 because no-one seemed to have a valid use case.

Either we try to get this in now before we hit RC1, but that might introduce a BC break. Or we try to get this done in 8.2.x. I'd love to support it, though. It's just that Group currently uses the same trick Comment does to have "dynamic" entity reference fields and that functionality is far from perfect in core either.

I'd someday like to hook up groups with https://www.drupal.org/project/asset_injector which uses ConfigEntityBase. This would be a nice feature.

+1 for adding menu's as group content. Do you have any pointers? We might have time to work on this for our current project.

I changed the issue title to make this easier to find.

I don't think there is a restriction on entity reference fields? For example:

    $fields['user_role'] = BaseFieldDefinition::create('entity_reference')
      ->setLabel(t('User role'))
      ->setDescription(t('The role of the associated user.'))
      ->setSetting('target_type', 'user_role')
      ->setSetting('handler', 'default');

Or in the webform module:

    $fields['webform_id'] = BaseFieldDefinition::create('entity_reference')
      ->setLabel(t('Webform'))
      ->setDescription(t('The associated webform.'))
      ->setSetting('target_type', 'webform');

I think this can be done without major changes.

While working on the groupmenu module for D8 this patch worked for me. The only problems I found where:

• No views integration for config entities. You have to provide a custom list.
• The autocomplete when relating entities doesn't work. The form needs to be overridden to use a custom autocomplete callback.

Please review!

The patch looks good, but the functionality is still something I'm not sure about. This patch would also need an update hook for the existing websites and would be a backwards compatibility break API-wise. I'd definitely reconsider this for an 8.x-2.x branch, but right now it's a bit too risky to work on right before we ship a release candidate.

Regarding the group menu module: Have you considered the suggestion of making menu links group content and using one big menu?

Thanks for your feedback. I've considered using 1 big menu, but I'm currently working on a site with 4300 groups (most of them have menu's). Having a big menu with thousand of links introduced issues in whole bunch of other area's in the interface (as well as permissions). Besides that you can't have multiple menu's per groups (like a main menu and a footer menu for example).

I think breaking the API now, vs maintaining 2 branches is something to consider. It's not a big change in the API, modules calling the methods don't need to change, only modules implementing/overriding them. Reimplementing Group or GroupContent is probably not a common use case. In the end it is your call off course, but for me it would seem this change is not worth the trouble of maintaining an extra branch.

Plus this patch makes it possible to add webforms as group content, one of the things I was going to work on next. The ability to add config entities makes a lot of new things possible, so even though it is risky, the high impact of this change in terms of new possibilities could make it worth while. So I would kindly ask you to reconsider, as the D8 beta change policy states:

Certain other issues with high impact and low disruption at committer discretion only.

Will work on the update hook asap.

I see your point. I can definitely see the benefit but the implementation might be tricky. Config entities tend to have dependencies for instance. For the Group Menu module: Having thousands of menus also means having thousands of config entities that need exporting when syncing sites. There's all sorts of cans of worms we might be opening here.

About maintaining two branches, I need to clarify something.

Group 7.x-1.x was ported to Group 8.x-1.x from scratch. Some concepts that didn't work were scrapped, some were added, some things were completely rewritten (e.g.: content plugins). But the main idea behind the module remained the same and the goal was to get something working rather sooner than later.

Group 8.x-2.x will not be an additional branch, but a replacement of Group 8.x-1.x. It will have the exact same functionality to begin with, but the classes will follow PHP principles better to allow for unit testing. The second release will be a developer release: Better API, cleaner code, more abstraction, ...

So once 8.x-2.0 is out, 1.x may stop receiving updates and (sub)module maintainers may be asked to upgrade. Their code will generally not have to change except for how they use the Group API.

Yes, adding config is indeed tricky (no views support, import / export issues, dependencies). There is some discussion on this topic in the groupmenu module as well. As I stated over there.

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).

Thanks for explaining the plans for 2.x. Sounds like this is something that could take some time. It would be great to have groupmenu working for the 1.x branch as well (I could patch the module, but still). I just posted a working version of groupmenu depending on this patch in #2716443: Drupal 8 version (also see the sandbox https://www.drupal.org/sandbox/seanpenn079/2868316). You could test that to see how it is working if you like.

Here's a re-roll of #6 that will apply to the latest dev state. No changes.

For anyone wanting to use it: you either have to apply the patch and install Group from scratch -- or if you already have Group installed: alter the `group_content_field_data` table to change entity_id to VARCHAR(32).

Added extra changes to new methods in GroupContentStorage.
Also see #2716443-51: Drupal 8 version .

Reroll against latest dev.

The patch works as expected, however:

For anyone wanting to use it: you either have to apply the patch and install Group from scratch -- or if you already have Group installed: alter the `group_content_field_data` table to change entity_id to VARCHAR(32).

There should be an upgrade path for this.

The patch could not be apply
`sudo git apply -v 2797793-13.patch`
---
Skipped patch 'group.module'.
Skipped patch 'src/Entity/Group.php'.
Skipped patch 'src/Entity/GroupContent.php'.
Skipped patch 'src/Entity/GroupContentInterface.php'.
Skipped patch 'src/Entity/GroupInterface.php'.
Skipped patch 'src/Entity/Storage/GroupContentStorage.php'.
Skipped patch 'src/Entity/Storage/GroupContentStorageInterface.php'.

`cd modules/group/`
`git init`
`git add .`
`git commit -m 'init'`
`git apply -v 2797793-13.patch`
`rm 2797793-13.patch`

I create a module also needs this patch
This patch #2900635: Group Vocabulary provide you add taxonomy vocabulary to group type content.

I create a upgrade patch change implements

alter the `group_content_field_data` table to change entity_id to VARCHAR(32)

Clean code

Feature request cannot be critical

Just found a serious possibly HUGE performance issue with this patch:

• The patch changes the entity_id column for the table that saves the relationship between a group and an entity
• Let's say you create a view (for example a media browser) that filters by group content, defining a relation between the media table and the group_content_field_data table
• You now added a join between a numeric field (media: mid) and a varchar field (group_content_field_data: entity_id)
• MySQL ensures that your indexes are not used if the data types of the columns do not match (https://dev.mysql.com/doc/refman/5.7/en/mysql-indexes.html, search the page for 'Comparison of dissimilar columns')
• When no indexes are used for 12,000+ records, the query slows down from 90ms to about 60 seconds!

We are probably going to solve this by creating a media browser using a search API based view, but this is something to seriously consider!

MySQL ensures that your indexes are not used if the data types of the columns do not match (https://dev.mysql.com/doc/refman/5.7/en/mysql-indexes.html, search the page for 'Comparison of dissimilar columns')

I have some test on this:
Building two site, execute the same sql

EXPLAIN
select * from
group_content_field_data gf
join node n on
gf.entity_id = n.nid
where gf.entity_id = 12

Result:

1. apply patch 2797793-20.patch:
   
2. no patch :
   

conclusion:

The useless index on diffrent mysql field type when use join operation really making drupal extremely slow.

I think we should redesign the structure of groups on content entity and config entity based on

How core works:
If we take a look at how entity reference fields work in Drupal, then you select 1 type of entity (Node, comment, menu...) and then it creates a table for the field with an id of a machine name (Varchar) for config entities or an Integer for non-config entities.

This patch is so wrong. When you request resources use include multiple level property , It will cash 500 server error. The e.g url like that

?include=entity_id,entity_id.uid

steps to reproduce:

with JSON API 8.1.3 , Group 8.x-1.x
1. create group
2. enable install gnode plugin to group
3. create one article
4. relate one article to the group
4. request /jsonapi/group_content/{group}-group_node-article?include=entity_id,entity_id.uid

After applyed this patch, you will got the error in the response.

How core works:
If we take a look at how entity reference fields work in Drupal, then you select 1 type of entity (Node, comment, menu...) and then it creates a table for the field with an id of a machine name (Varchar) for config entities or an Integer for non-config entities.

This let's group imitate entity reference's design, but it can't

Reason:

\Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem
entityreference-propertyDefinition
[entityreference-schema]
Entity reference do the work of choosing between string or integer based on field, It's field who create new table, not entity reference, So if group want to use the design, it should learn the architecture of **field, not entity reference**.
The design can be used on three level:

1. (bundle level)can't work if directly flow it's design, If imitating it, means group do sth on bundle(group_content_type or group_type):gnode_article, gnode_basic_page group_type_teacher,
   It dosn't has 'config_entity' effect on entity:group or group content
2. (entity level)If group applies the field's design for entity:group and group_content arbitrarily, it means create a table on every entity instance. But group's cose idea(repalce og) is entity type which make it conforming to core's other component, new table on entity break it's origin idea, make it can't work with drupal core's views、 import、export, dependencies, so this level can't work.
3. (table level)If group just use 'new table' idea on group_content_field_data, can't work with core's views、import、export..... can't work either

So entity reference can't be used as an example to resolving group-entity-level-config-entity problem.

Besides:

1. newset drupal7's group dev version can't add bundle(vocabulary) ethier.
2. drupal field's base_field_override method need to be investigated, But I can't find a proper example use case in drupal core

let's figure out the drupal8's content_entity and config_entity, use node for example:
Configuration entities use the new Drupal 8 entity API in order to keep configuration in the database.

1. node is content_entity_type
2. content_type is Config_entity
3. bundle is subset or variant of content_entity_type
4. node.type.article is a content_type instance, this config_entity instance store the information of bundle article, Config_entity can't has fields, but article can has fields.
5. bundle != config_entity instance
6. But bundle and config_entity instance has consanguineous relationships.

what group need do is make each group type's instance has some restrictions on some bundle, not config_entity.

How to do this?

This may be a good direction

Create a new config_entity type called group_config or group_restrict or whatever else;
This entity type has two properties that group need:

1. group
2. bundle----->because bundle is defined by confige entity, so this point to corresponded config_entity instance to get it's information.

The entity_id is a BaseField of group_content_type, so it can't like bundle's entity reference field: every entity reference field create a new table
So if we move entity_id from BaseField to BundleField, we can solve #22 's performance problem.

Because it's a very different implementation, so I created a new issue: https://www.drupal.org/node/2920474

Comment #22 is serious enough for me to be extremely hesitant about implementing this. It might be better to start working with a GroupConfig/GroupConfigType instead to follow the model set out by GroupContent/GroupContentType instead of having one column in the database trying to do both.

@kristiaanvandeneynde
I agree with #31 saied, what i think on #28 is very similar with this, but I just consider one level: group_config config entity type, #31 furthermore define it as GroupConfig/GroupConfigType

Every relation between a group and its content/config needs to be fieldable, hence content entities.

#31 in general +1 to having another GroupConfig / GroupConfigType. Not sure what the implication are, but I guess that is a very serious change.

@kristiaanvandeneynde, could you provide some insight on the work that needs to be done to implement this? I might be able to help push this forward, and since I have some sites running Group Menu 8.x, it is also important for me to provide an upgrade path.

This is actually work I had planned for an 8.2.0 release, but here's the basic idea: A GroupConfig entity that is almost exactly like GroupContent, except it links config entities instead of content entities. The entity_id field would obviously be a string base field in this case.

This has several potential downsides, which I'll list below, so there may be a better way to tackle it. I just hadn't gotten around to fleshing out this idea yet because it was rather low on my priority list.

Potential downsides:

• Two entities that do the same thing: Connect something to a group.
  • I had thought of adding another base field to GroupContent to reference config entities with, but that would imply dynamically selecting a database column on many levels: Direct field access, views, ...
  • I had also thought about a shared base class and shared handler classes. This might make the code more readable and clearly outline what parts are shared and what parts are specific to one or the other.
• A code distinction also implies a UI distinction. I.e.: People need to know when they're adding config vs content. We might need to duplicate our views handlers for relationships, filters, etc.
• Our group content enabler plugins should ideally not become two systems as well. Instead, we should dynamically find out whether the enabled entity type is config or content and then create the right Group(Content/Config)Type based on that.

As you can see, it can get real messy real quick. A cleaner way would be to figure out a way to dynamically select one of two columns and feed that into the entity_id field based on whether the target type is config or content. Perhaps with a custom field storage?

Core suffers from the same problem with comments, but because hardly anyone adds comments to config entities it doesn't really show :) If we were to come up with a custom field storage for the above use case, we could contrib that back to core for comments.

Implement #31's GroupConfig / GroupConfigType, and a note_type plugin GroupConfig instance
Functional work well, but

1. No hook_update yet
2. No test code yet

Maintainer shouldn't spend time review the code before completing above two tasks.
But hope groups users can test this patch.

1. Add hook_update
2. Remove two test purpose file in gnode submodule

Todo:

1. Create and modify dozens of test code:

Change

1. Add group_config crud menu tab.

Todo:

1. Create and modify dozens of test code.

When installing this patch with composer, the following error appears:

- Applying patches for drupal/group
    https://www.drupal.org/files/issues/2797793-39.patch (Configurable entities as group content)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2797793-39.patch

And in composer.json:

        "patches": {
            "drupal/group": {
                "Configurable entities as group content": "https://www.drupal.org/files/issues/2797793-39.patch"
            }
        }

#39 has to be rerolled

Hunk #2 FAILED at 235.
1 out of 2 hunks FAILED -- saving rejects to file src/Access/GroupPermissionHandler.php.rej

the lines are:
more src/Access/GroupPermissionHandler.php.rej
--- src/Access/GroupPermissionHandler.php
+++ src/Access/GroupPermissionHandler.php
@@ -235,7 +244,7 @@
$permission = ['title' => $permission];
}

- // Set the provider if none was spec
+ // Set the provider if none was spec.
$permissions[$permission_name] = $permission + ['provider' => $provider];
}

This is just a reroll of 39 for composer.

I am getting this error:

Symfony\Component\DependencyInjection\Exception\LogicException: Service [error]
'group_config_type.breadcrumb' for consumer 'breadcrumb' does not implement
Drupal\Core\Breadcrumb\BreadcrumbBuilderInterface. in
drupal-8.4.2/html/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php:164

After update group latest. This module can't be apply into repertoty. and depend on this patch build will broken.
see:
https://www.drupal.org/project/group/issues/2810131#comment-12364263

@seanB
Do you have any good idea?

@dravenk I guess the patch should be properly rerolled against the latest version?

I have this error

patch -p1 < 2797793-configurable-entities-as-group-content.patch
patching file group.install
patching file group.links.action.yml
patching file group.links.task.yml
patching file group.module
patching file group.routing.yml
patching file group.services.yml
Hunk #2 FAILED at 106.
1 out of 2 hunks FAILED -- saving rejects to file group.services.yml.rej
patching file src/Access/GroupPermissionHandler.php
Hunk #1 succeeded at 148 (offset -5 lines).
patching file src/Entity/Controller/GroupTypeController.php
patching file src/Entity/GroupType.php
patching file src/Entity/GroupTypeInterface.php

the patch introduced this error:
drush cr
Symfony\Component\DependencyInjection\Exception\LogicException: Service [error]
'group_config_type.breadcrumb' for consumer 'breadcrumb' does not implement
Drupal\Core\Breadcrumb\BreadcrumbBuilderInterface. in
drupal-8.4.3/html/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php:164

This is a re-roll of #42 against dev that resolves some patch errors.

Missed some updates from the patch in #13 in my last comment #47. This updated patch should work against dev.

While the approach in the latest patch seems okay, it essentially cope-pastes everything regarding group content and names it groyp config :) We might want to look into getting rid of code duplication there.

For Group 8.2.0 I was thinking of a common interface between GroupConfig and GroupContent entities for the most part, methods on GroupInterface that will retrieve any entity regardless of type and opening up the access checks to handle both types of relations. We shouldn't be copying this much code :)

Been spending a bunch of time recently looking at the pros and cons of Group and wondered how much of a priority this particular issue has in the future development path? It seems that many of the biggest missing pieces are being able to connect Group to a config entity (taxonomy, menus, webform, core workflows?) In D7 using OG, additional OG contrib modules handled all of these cases for us and worked really well, but I would love to see Kristian do his thing, writing beautiful, clean code that does this "the Group way", instead of a big copy/paste job as he pointed out. I have no doubt he will ultimately come up with an awesome solution for this. Thanks, and looking forward to seeing this module continue to grow, improve, and expand!

Recently a similar problem was solved in the entity usage module (in the same way as dynamic entity reference module). See #2951093-4: Support alphanumeric entity IDs and config entities for more info.

Basically, the idea is to add a separate column for string IDs to the table and make the module handle this dynamically. This would prevent a lot of the duplicate code.

Hi,
Any updates on this? Our use case is a decoupled Drupal CMS powering a bunch of react frontends and using Group module for relationships between content and said groups.
Works beautifully with nodes, but we also need to have menus per group (menus of each website).
This would be truly amazing to have in Group (along other things like vocabs for ex).
Thanks for such a great module!
Cheers,

I am also interested if there are any updates in this issue as Menus per Group would be very useful when using the Group module to manage subpages. Or are there other good ways to achieve this? Cheers.

In testing patch 48 I noticed an error in the GroupConfigCardinalityValidator where it was calling an undefined method on the Group on line 119 of src/Plugin/Validation/Constraint/GroupConfigCardinalityValidator.php:

It is calling

      $entity_instances = $group->getConfigByEntityId($plugin->getPluginId(), $entity->id());

which doesn't exist.

Should that missing method be added or should this call be changed to something like the following since this is looking up a relationship entity rather than the config itself?

      $entity_instances = $group->getContentByEntityId($plugin->getPluginId(), $entity->id());

@adamfranco I can confirm that getConfigByEntityId is not defined in the group module but getContentByEntityId is.
should the patch be re-rolled?

It needs to be re-rolled to not get a fatal error when relating a config entity like a menu using groupmenu. See #2990525: Not compatible with latest "Configurable entities as group content" for a patch to groupmenu to make it compatible with the latest patches here.

As mentioned in #54, I think getConfigByEntityId() needs to be defined but I'm not totally sure.

Guessing it's probably related to @adamfranco's comment.
But when I apply #48 my group content Search API index fields no longer work.

Attached is a rerolled patch to catch up to the "master" branch. Hiding old patches that are outdated and/or not going to be used due to performance issues.

I am very interested in getting this functional because I have a need to control Webforms with Groups.

Hi, thanks for the patches.

I need to use Group Menu module which says I have to apply this patch so it can work.

I tried patch #58 but I got this:

patching file group.module
Hunk #6 FAILED at 183.
Hunk #7 FAILED at 202.
Hunk #8 succeeded at 223 (offset -8 lines).
Hunk #9 succeeded at 311 (offset -8 lines).
Hunk #10 succeeded at 563 (offset -12 lines).
Hunk #11 succeeded at 579 (offset -12 lines).
2 out of 11 hunks FAILED -- saving rejects to file group.module.rej

When I install the menu component at a group type I got the "The website encountered an unexpected error. Please try again later." white page bug.

:(

You have to apply the patch to the development branch and not the RC.

Thanks, Perignon! It's working now.

Can I use this to allow custom entities to be group content?

Hello, what a work to make all this happen! Good to see.

I am want to use ECK and group. Unforunately group only support nodes, not a thing as ECK.

So I see this thread and when I read this, this wil solve my problem. Yeah!

So I installed the patch with no issues. Now I see "Group content type - Adds content type to groups both publicly and privately."
When I do this and just enter for what it is, then I go to the 'content' page (Home>Administration>Groups>Group types>Edit Group one ) I was expecting to see the ECK fields. Bud they are not there.

I think I have to do something else to set a relation between 'group' and my ECK types. Bud reading this thread + fiddling arround, I can't figure out what I have to do to make this work.

Please can someone explain how to configure this? I think the module + patch is ok, bud I have to config somewhere a 'missing link'

Thanks,
Marchello

@SocialNicheGuru See #5 on this list https://www.deeson.co.uk/labs/9-reasons-group-drupal-8-awesome, that should help.

@marchellodepello unless your custom ECK-based entity is a config entity and not a "real" entity (I believe ECK builds the latter?), I'm not sire this issue pertains to your situation. I believe you may also need to do #5 above to connect your custom entity type to Group.

@scotself, thank you for picking up my post!
I do not understand what you mean by 'your custom ECK-based entity is a config entity and not a "real" entity'. Ahum, what is a custom or real entity? A entity is a entity to me. Als as far my knowledge goes a ECK creates entity's.
Regarding the #5 I have a look at the patch, and compare this to the patch I applyed (#58) and it seems to me things in #5 ar still also in #58. So when I apply the pach #58 I cover the code in #5.
Let me say; I am not a coder bud with my level of knowledge this is what i see. Excuse me when I am wrong.

I was searching for some way to get the ECK content into groups I stumbled over issue #2723911
I think this is what I have to to. And I was thinking the patch of this thread is to do this bud now in IU (remember I am not a coder). ( have this link from https://www.deeson.co.uk/labs/9-reasons-group-drupal-8-awesome on the second comment "Great Module ... There is.." wich also is the question how to reach the ECK fields.)

...and still I think there is a missing link; I have to config somewhere somewhat, but I just don't know what. Again I search the Drupal UI what this could be. Bud no luck. Only I think I have to make a reference field in '/admin/group/config/manage/group_[group name]group_node_type/fields'. ? Or somewhere else.

I hope you can help me a step forward. I have the feeling i am almost there, but I have to find this missing piece...

Hi guys,
What would be the next steps to getting this in?
We are in terrible need of this (and group menus as consequence).
I can see this has been going since 2016 and I'm adamant of putting this patch in the build without some idea of where we are at.
Happy to put in the work, just need to know what next steps would be?
Cheers

Sorry for the confusion @marchellodepello, but there is definitely a difference between an Entity like a Node or User ("real" entity? not sure what else to call it to differentiate...non-config entity? database entity?) v. something like a Menu or View, which is entirely configuration. There are many articles about this out there, here are a few:

https://www.drupal.org/docs/8/api/configuration-api/overview-of-configur...
https://www.drupal.org/docs/8/api/entity-api/configuration-entity
https://www.drupal.org/node/1818734
https://drupal.stackexchange.com/questions/236622/what-are-configuration...

More confusion came from me referencing "#5", which would normally refer to comment #5 on this thread, but I was specifically pointing to the same thing you were, which is mentioned in item #5 in the deeson.co.uk blog post I mentioned; that is the simple idea of extending Group Content support for a custom (content) entity with a plugin which extends GroupContentEnablerBase. This is, as you said, not very useful if you "don't code" (in my experience, you can't really harness the power of Drupal completely without getting into the code, at least at some level). Unfortunately, it isn't possible at this point to extend Group Content support to other entities (including ones you make using ECK) via the UI, you have to have that small bit of custom code mentioned in that post and the d.o post you mentioned.

If ECK doesn't build configuration entities (don't think it does), following this issue will not help you unfortunately. Again, this thread only deals with configuration entities like Menus and making them available as Group Content. Hope that clears up things a bit.

@kyuubi et al, I think that @kristianvandeneynde is right and that this patch, while well-intentioned, isn't really the right approach as it duplicates the GroupContentEnabler code in the form of GroupConfigEnabler and creates 2 places to maintain what is essentially the same code. I made a similar mistake in trying to get sub-groups to work, basically copying all of gnode as ggroup and trying to make that work. This approach also, unless I'm missing something still requires a plugin for each type of config entity too; the only one available initially is the content type config entity (provided by updates in the patch to gnode), so one would still have to create a plugin in order to extend this to other config entities?

To take it a step further, it doesn't make sense in my mind either to require the user to be comfortable coding in order to tie existing entity type bundles to a Group type. Seems like the better approach would be to offer a UI which would:

1. allow both content AND config entities as "Group Content"
2. just work for all available entity types in the system

I think the second point is important because folks like @marchello above will not be comfortable writing a custom module with a plugin file just to add the capability to, in his instance, a content entity type created by ECK module. Ideally, you'd go to a single UI page, see all available Config Entity (Menus, Contact forms, etc) types and Content Entity types (Commerce Products/Orders/etc if your site has Drupal Commerce, for example) that Drupal knows about and choose which ones to allow/create the Group Relation entity between. You could even set some options (group and entity cardinality) from the UI. Just throwing this idea out there, but if we're going to keep going down this path I can instead provide feedback on the patch. Just feels like we should step back and think about it a bit more before plowing ahead with this.

To be sure, I'm just as anxious as anyone to get this (as well as #2949408: Allow group admins to create user account and add to group) going as I have a large site we're starting to build out which, put simply, will be next to impossible to do without this piece being figured out and working well. Group-specific menus (this issue) and Group assignment during account creation are critical to our project.

Hi @scotself I tend to agree with you although I'm not sure about the route of hosting both config and content entities in the same process.

Although I am not familiar with this module's code, I imagine this approach would require a significant rewrite to the already stable process of plugging in content entities. It's also likely that we would need to diverge the implementation a bit to handle both.

However, the concept of having a base (or core) plugin system for both config and content entities, that can then be extended for each with it's own variants makes a lot of sense to me.

From a UI perspective, I agree the separation might not make as much sense. Content and Config entities are more of a technical concept and a site manager won't really distinguish between them.

Bottom line, I would vote for an approach that either:

a) Implements a separate config entity plugin (what this issue tries to do) without unnecessary code replication.
b) Gut out the current content entity plugin system to a base that works for both content and config (shared code) and then provide extended classes to deal with each entity type specificities.

Hope that makes sense and keen to hear everybody's thoughts.

We are also extremely dependant on the viability of this for our project and could potentially allocate some resources to helping this become a reality, provided we can pair up with someone to give us some insights into how Group works.

Hi @scotself,
yes, this clears up some confusion... about the 'config' and 'content' entities, and about the #5 patch :-)
Regarding config/content entities, now I see the difference.
Content entities are the content, config entities are holding configuration things.
As an example, when I install plain Drupal and create an 'article', I create a content-entity which is a node of type atricle.
Somewhere there is a defenetion of what a 'node' is. This defenition is a config- entity.

Am I right?

Now, when I look back to my case, then things are a bit confusing.. when I make a new ECK entity type, ( /admin/structure/eck/add ) there I configure a new ECK- entity type. It looks to me I am making a 'config-entity' on that spot? And when I add content to that ECK-type, then I make content.
Regarding to your text "this thread only deals with configuration entities", it should deal with the config-entity I made on /admin/structure/eck/add ?

Probably I am mistaken here :-|
Bud here it is confusing for me. Is 'making a new ECK type' not equivalent to making a 'config-entity'?

@kyuubi I agree with all of your comments completely. It is nice from a coding perspective to be able to just create a plugin and extend or alter the functionality of GroupContentEnablerBase, but I would imagine it would be possible to both support known entity (content AND config) types out of the box (a UI of some sort) while still allowing for Plugin files which can override the base functionality.

We're in the same boat and I've reached out to @kristiaanvandeneynde to recruit his help but he has recently had a baby and job situation change, so it may be tough to get him engaged at the level that it sounds like we need here, but I have high hopes he'll rediscover his love for this module soon and jump in :)

@marchellodepello I have actually never worked with ECK module, but it seems to work very similar to core Entities: the Entity type itself is DB-based (and thus should be able to integrate with the GroupContentEnablerBase) but the definition of each bundle of your ECK entity type is a config entity (which can be exported via the new Config Mgmt system).

To all who have been working to try and get this working, I totally applaud the effort, but I honestly don't think @kristiaanvandeneynde will merge it in using the approach in the patch here. The code for this module is meticulously well-written and I don't think code duplication like this will fly (see his comments in #31, #36, and #49).

Hi @scotself, thanks for your reply's.
In this moment I am thinking for my own case a go an other direction and not use ECK.

Bud for the ones reading this and want to have a solution for the problem I came along, look ad:

-> issue 2775119 where kristiaanvandeneynde wrote at point #4 he wants to enable all kinds of entities bud this is 'parked' untill an other issue is checked. So hopefully this is gonna happen.

-> and Add example for how to expose custom entity to Group module for insight and learning how to get entities into the scope of Group.

-> and this also as an example of how to.. Create a custom content entity.

Maybe I help someone with this POI's.

Marchello

Hi @scotself,

Been looking a bit more into the module's code and I feel introducing a similar concept like we have for GroupContent would always lead to some duplication as well as considerable level of complexity, since we probably would need to refactor a lot of existing code to support both of these things.

It's starting to feel to me that an abstract level that we should consider the "Content" in GroupContent to encompass all type of entities. This would mean we can leverage the existing system we have and just add in support to both entities where needed.

As @kristiaanvandeneynde pointed, out maybe a way of dynamically selecting the id based on the entity type. The Group Content entity supports an entity reference field which by itself is able to target both config and content entities and all the other concepts that currently exist (like the plugin system, two step adding process, fieldable group content etc) all make sense to exist for config entities.

Looking at the approach the Group Menu module is taking seems to fall well with the above. It obviously relies on there having some support for config entities (currently with this patch), but it extends the GroupContentEnabler plugin. Using this module as a reference could actually be a nice way to try to develop this support (keep refactoring the GroupContent system until the Group Menu module works).

Then each contrib module could take a similar approach to extending GroupContentEnabler to provide support for their config entity of choice.

Bottom line, I think the original patches were more inline with the correct approach, we just need to come up with a way to handle the entity_id without changing the database column which proves to be a performance issue. Either having two fields (with a process to switch based on the entity type) or something similar to the quoted approach core takes.

Not sure this makes sense or is 100% viable, given my still lack of knowledge of the module's internals, but conceptually seems like a reasonable approach.

Thoughts?

PS: If there are a number of interested parties to getting this done, maybe we should get together on slack or something (pm me @kyuubi) and discuss further. I feel that in the pace of an issue queue this will take a long long time to get done.

Also #2931932: Explore adding support for config entities demonstrates that workbench_access (doesn't require workbench moderation or content moderation) works with config entities.

You could wire up a group - to workbench access bridge

Oh, but workbench access doesn't have an opinion on view access, while group does. Nevermind

I think at this stage I see only two viable approaches:

First we apply the changes from the initial patch (except the schema changes) that basically generalise behaviour across the code to all entities not just content. Then either:

a) Create GroupConfig content entity (that uses a string id in storage) and GroupConfigType class, then refactor code on existing system accordingly. Or:
b) Try GroupContent for both types of entities by having 2 columns in the schema and some sort of logic to switch between them.

The a) approach has the advantage of being more standard. We can probably minimise duplication to an extent by creating some base classes to share code where possible, but the approach will likely involve a higher amount of code refactoring.

The approach b) on the other hand, if achievable should be involve less messing around with Group's code, but seems quite more challenging and somewhat less elegant to an extent.

The Dynamic Entity Reference module solves this problem, but unfortunately relies on triggers which don't work on any hosting environment (e.g Acquia).

Also from conversation with @larowlan it seems Comments limits itself to integer entity ids, even for content entities, and as he pointed out Group as it stands probably doesn't support custom content entities with string entity ids, so the issue isn't completely scoped to configuration entities.

At this stage it would be awesome if @kristiaanvandeneynde could chip in if there is any of these directions that wouldn't be considered for this module, just to avoid us spending time in the wrong direction. As soon as we have a clear path to move forward I can probably allocate some time to getting this done, maybe with some other interested parties help?

I've reached out to Kristiaan, hopefully we will have his input sooner than later. As you said @kyuubi, I don't think any decision should be made without his input so as to not waste time on approaches that won't be merged in. I'd love to work through this in a group setting on slack, maybe he will be able to set some time aside to do that.

is there a way for me to uninstall just this patch ie. the update 8015 that it introduced? I can't uninstall and reinstall the group module within my site. thanks.

I am still on the fence whether I actually want this behavior at all. I see a lot of mentions for group menus: A use case I have solved myself about a year ago but never got around to contributing back. And that code is out of my hands as I work at a different agency now. I am curious as to what other use cases people might have that require you to have config entities being grouped.

But let's assume there are good use cases and that we want this in Group.

The first and foremost thing you should all be aware of is that Group uses the same hack core uses for comments to allow our entity (GroupContent vs Comment) to reference any content entity. This is behavior that Commerce uses as well and is at best to be described as "vague". See #2346347: Finalize API for creating, overriding, and altering code-defined bundle fields

Secondly, I may have closed caseylau's issue too quickly, because it might be worth investigating the ramifications of moving the entity_id field to a bundle field. If that means it gets dedicated tables where content entities have a numerical ID and config entities a varchar, then great! Might be worth exploring that route. The issue I'm talking about can be found here: #2920474: Move group_content entity type's entity_id field from base_field to bundle_field

If that is not a good solution, then seanB's mention of trying to use two columns instead of one might work, but I dread the amount of confusing code and bugs that will probably bring us. (see #2951093: Support alphanumeric entity IDs and config entities)

Finally, the more I think about it, the more I am opposed to copy-pasting a ton of code just to reproduce functionality we already have elsewhere. Copying that much code is a code smell and we should avoid it at all cost. I deeply apologize if I encouraged people to try and pursue this route, but I have had very little investment in this issue so far.

So what I'm trying to say is this:

1. Does anyone have a good use case for adding config entities to groups, other than menus? If not, I'll gladly go into detail as to how I solved that use case using what we already have (if I can remember properly).
2. If so, can anyone investigate caseylau's proposal in order to facilitate this without changing too much code?

With regard to having to write plugins to support certain entity types: This can be easily solved by writing a deriver that generated basic plugins based on what you select in a simple admin UI. I've been meaning to add such a thing for content entities, but have yet to get around to that.

I am currently in the process of finalizing some tests for new cache contexts which will allow me to tag Group 1.0 in the very near future. I sadly can't spend any time on this issue for that very reason. Sponsored time could work, but as it stands I am stretched thin trying to finally get a full release out the door.

Thanks for getting back to us, and hope the new job and kid are both going well!

For us, there are multiple critical, indispensable (like we really can't launch the site without this type of functionality) use cases:

• Menus - each group needs to be able to define and control menus for their pages, which only they can place on a page and edit but are still available for public display.
• Taxonomy - each group should be able to define and control its own vocabularies and terms which it can use to tag Group Content and is still available for public display.
• Contact forms - each group should be able to define and control its own contact forms which they can use for receiving public input for their Group.
• Workflows/moderation (core) - each group should be able to define and control its own Workflows/transitions (all config entities, i believe) to customize the workflow to be used in conjunction with that Group's roles/permissions for controlling the publishing of Group content.
• Blocks - each group should be able to define and control its own Blocks.
• (not config entity-related, but still mission-critical) User Registration #2949408: Allow group admins to create user account and add to group - must be able to create new accounts and assign them to a Group at the same time.

All this creates the possibility to use Group as a "microsite" building tool, with each Group kind of having its own little, self-contained world, with everything it needs to control the creation of its content while higher level stuff shows lists of Groups to join, links to contact forms for each group, lists of Group content by Group term, etc. Bonus: config entities can be exported/imported (read: deployed via git) via Config Mgmt (yml files).

I don't know about caseylau's proposal but I will try to make some time to look at that soon.

@scotself does an excellent job of summing up what we would need to use Group to build microsites. I'm currently maintaining a private Drupal 7 distribution for an enterprise client, and it makes use of Organic Groups and the various add-on modules to support group-specific menus, webforms, FPP's, etc. Webforms are the big one, however. And they're config in D8. The client has 100+ webforms across dozens of groups.

Hi @kristiaanvandeneynde,
Thanks for chipping in, your guidance is really appreciated.

In terms of use cases, the others have done a good job explaining the common ways where config entities become critical for people that use Group.

I find that a good portion of Group users, choose this module in order to scope parts of a website as a collection (can be microsites as some mentioned).

For my use case, Drupal is used as a content service API, powering multiple ReactJS applications. The way we segment content for each application is via Groups. This functionality is essential for us, mainly for menus (so that we can scope sets of menus to each group and serve them to the apps) as well as taxonomies (each application can have its own taxonomies). Editors are then also segmented into the various groups so it's important that editor from Group A can manage menus and taxonomies of that Group without accessing others.

I think we all agree copy pasting code is to be avoided. I still think it's likely possible to create a Config type and Config entity and then adjust the other classes to accommodate for them both, using shared based classes where needed, but let's investigate the issue you raised first and go from there.

Thanks for the input!

Okay so I'm starting to see a need for supporting config entities. I just have two major concerns in addition to #79:

1. Is core capable of supporting fine-grained config entity access? For example: Would the menu or taxonomy page actually check individual view access before listing the available taxonomies/menus?
2. What about collisions and data leaks? If a member of Group A wants to create a 'seasons' taxonomy and then a member of Group B wants to do the same, they might get an error stating that machine name is already taken. This has two downsides:
   • They might need to choose annoying machine names such as seasons_0. Group has this too when it comes to plugins and group roles and "fixes" it by hashing the machine name. Not my finest solution :(
   • They could use this to figure out what taxonomies, menus, etc. already exist in other groups (data disclosure vulnerability)

Hi @kristiaanvandeneynde,
Not sure about 1), would need to check, but I imagine it's possible.

As for 2) you are right, but I think that is something we just can't avoid. This is something that can probably handled by developers extending Group functionality if there is a need for total isolation between groups.

For example, this already happens with urls. You can't have groups using the same url for two separate content items.
We have worked around that by extending a prefix (invisible to site editors) so that all urls are in fact prefixed by the group id. Again this is something I don't think should be handled by Group as it will depend a lot case by case.

I think you can see that if we go down that road we might start questioning many things, several outside the scope of this issue.

From my end, for what it's worth, I think if there is the ability to control access and scope content and config to a Group that would be enough and clearly in much need. There will be issues we can't fix as they are inherent to core, but most of the time developers can adapt the solution to their use case to fix them where necessary.

Point 2. could be something Group (or a related module) could resolve more generically. The Group module is providing some separation of content (and hopefully config) within a single website. Without knowing exactly how this is currently achieved, namespacing or prefixing, as suggested in #84, would be one way.

This could lead to a fair bit more complexity but also a lot of flexibility. For example, if you create a vocabulary called "Seasons", the title is "Seasons" while the machine name behind the scenes is "group_name_seasons". I don't see much of a need for group admins to change the machine names of their vocabularies or other config entities.

2. What about collisions ...

My use-case is micro-sites for university departments where I need a menu per department micro-site. The individual menus would be created by an admin prefixed with the department name at setup time and then associated with the group, so collisions wouldn't a problem. After that initial setup, group members could edit their menu as needed. Adding/removing users from a group is all that is needed to grant editing access and is something that could be delegated to users with an elevated Group-management role.

Until this feature is available and the menu can be associated with the group, my ugly work around is to use Group to manage editorial access of content and then maintain a separate Drupal role alongside every Group that restricts access to menus using menu_admin_per_menu. This means that adding a new department member to a Group isn't sufficient to allow them to edit the microsite, they also have to be added to the department role by a site-wide admin in order to add nodes to the menu. This will probably become less and less workable as the number of departments passes 50. I look forward to this feature greatly simplifying our user/role management.

In the group view, there is an option 'Group content type'. Currently it only selects nodes.

How can I filter by custom entity types within a group view?

@kristiaanvandeneynde #79:
As of the question "bundle field vs 2 base fields"... i can remember when i dug the source code of Dynamic Entity Reference and they have 2 fields on that reference field. This then has a lot of not-really-nice consequences. So my gut feeling is definitely towards bundle fields.

FYI to all those interested in this and other D8 Group issues, I just created #d8-group channel in Drupal slack http://drupalslack.herokuapp.com. I've invited a few folks initially, but feel free to jump in there. It would be ideal if we could get Kristiaan to set aside a dedicated time slot ("Group module office hours"?) to working with us all to get this and other major Group module issues worked on, but I know how challenging being a new parent and a new job at the same time can be! Hope to hear from some of you there. :)

Hi all,

I'm trying to understand the very very long drupal.org thread and the proposed/implemented solutions. This is what I've understood so far:

Summary
We need that Group module supports config entities in the same way that it does support content. For example, it should be possible to relate a menu to a specific group.
Main problem: Config entity id is a string which differs from a content entity, which has an integer id.

Solution 1 (declined)
Update entity_id column to varchar which would allow referencing any kind of entity (config or content) (patch from comment #20)
Reason why it was declined: MySQL performance issues (comment #22)

Solution 2 (declined)
Duplication of all code related to GroupContent as GroupConfig.
Reason why it was declined: Maintenance. Imagine if something has to change on GroupContent and you need to change in GroupConfig too...

Solution 3
Create a Base class which GroupContent and GroupConfig will extend. We need to take a deep look at the code and figure out what is common and what are the differences. So far, the only difference we know is that we need a varchar entity_id for the GroupConfig.
This is basically the approach from solution 2 but without lots of duplicated code.
I see that this would require heaps of coding. Even considering a Base class reusing code from GroupContent, a refactor in the GroupContent would be necessary too.

Solution 4
Create a new column entity_config_id (BaseField) in the GroupContent and let the submodules to handle where to get the id (from the integer or varchar column). How the submodule would define that? Does someone have any idea about the next steps for this proposed solution?

Solution 5
Remove entity_id from BaseField and add as a field into the Plugin. I think that what caseylau suggested here: https://www.drupal.org/project/group/issues/2920474
- Remove entity_id column from group_content_field_data table
- Add entity_id as a reference field to the Plugin (e.g Membership, Article, Basic Page, Menu)
-- In this approach, we would need that each submodule (gnode, groupmenu) to be responsible for creating the field automatically when the plugin is installed.
- Refactor GroupContent entity_id references in the code to get the field defined (How do we know the field?)
I can see that this approach would require much less coding than the others, but since we need to restructure by moving the entity_id (baseField) to an entity reference field, it's still not clear how we can manage this from the parent module, since I assumed to define in the other modules (gnode, groupmenu, etc).
Can anyone help me to see better what are the next steps in this one too?

Can you guys point out if I misunderstood or if I am missing something here? Before moving forward with any implementation (solution 3, 4 or 5) I would like to hear your suggestions and concerns.

I have time allocated for this issue but I need some brains with me. Would be very happy to work together to get this done sooner rather than later. =D

Ps. Also sent this is the slack channel mentioned by @scotself

Hi everyone,

The approach number 3 looks the best one, but it requires a lot of code refactoring and however it will need the new column "entity_config_id" because the entity_reference field is defined based on the "target_type" setting so forcing it to be varchar was already discarted by performarce reasons.

I think the code changes in patch #13 were ok but removing the change of the entity_id field to varchar, for that reason I took that patch as base to do the solution 4 mentioned by @carolpettirossi and making the GroupContent entity smart enough to detect which field to use.

This is an initial work to detect how many methods we will need to override in case we decide to go for solution 3, please check the patch and let's continue the discussion.

Sorry, I forgot to remove the target type on entity_id field in previous patch.

I'm more inclined to look at the "purely bundle fields" solution (move it from GroupContent::baseFieldDefinitions to GroupContent::bundleFieldDefinitions altogether). If that fails, we could look at the custom storage (2 columns for 1 field), but as mentioned in #88 that might be nasty.

OK and we may want to take note of #2935932: Add a FieldDefinition class for defining bundle fields in code. which is going to land soon.

Oh shit, sweet! Did not know that was in the works. Great to know, thanks!

This issue should cover not only config entities, but other entity types using string entity IDs. A good example is external entities.

For instance, here's how entity reference handles content/integer vs config/string references.
From \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::propertyDefinitions():

    $target_id_data_type = 'string';
    if ($target_type_info->entityClassImplements(FieldableEntityInterface::class)) {
      $id_definition = \Drupal::entityManager()->getBaseFieldDefinitions($settings['target_type'])[$target_type_info->getKey('id')];
      if ($id_definition->getType() === 'integer') {
        $target_id_data_type = 'integer';
      }
    }

As you can see, there's no content vs config entity type check. Instead, the ID field definition is used to determine the correct data type.

According to \Drupal\Core\Field\FieldStorageDefinitionInterface doc:

While field definitions may differ by entity bundle, all of those
bundle fields have to share the same common field storage definition. Thus,
the storage definitions can be defined by entity type only.

That is also confirmed by the fact that the field storage config entity only refers entity type.
It is possible that some future core version will provide bundle-specific field storage solution but seems like it's not possible at the moment, even with all these patches floating around.

I've spent half a day finding a way to do this and it the "purely bundle fields" approach seems to be a dead end unfortunately.
Seems like the only viable solution is using two entity_id fields, one for integer, another one for string IDs.

Here's a patch for supporting both content and config entities, identified by either integers or strings.

The patch is based on #93, thus it's adding a new field for string IDs (the field name is entity_id_str).

Unit tests aren't updated yet (string IDs aren't tested) by at least regression tests should pass.

I've added a unit test for the previous patch.
Also, found a bug (missing entity_id field handing in createForEntityInGroup(), came from previous patches) while writing the test, so fixed it as well.

I'm attaching two patches, one with test only (should fail), another one with test + fix (should pass).
Tests aren't specific for a current solution and may be used for other approaches as well.

Can someone review please?

Thanks.

The patch looks good, just an small fix to support entities with IDs longer than 32 characters.

Hi all,

Amazing job in getting this done. =)
I've just applied the patch and it works properly. The groupmenu module also works with this patch.

Thanks,
Carol

Just gave this a spin, works nicely.
Great job guys!

regarding # 97:
> [also content entities can have string IDs]

correct. thanks for enlighening us with this.

regarding #98:
> I've spent half a day finding a way to do this and it the "purely bundle fields" approach seems to be a dead end unfortunately.

The comments in the issue mentioned in #95 (#2935932: Add a FieldDefinition class for defining bundle fields in code.) prove that this is done already and will be even easier with that patch.

I guess one person "did not find a way" is not what the maintainer meant in #94 so setting NW.

Hi @axel.rutz,
Sorry, I may have been not precise.
The core issue you're referring to makes it possible to define bundle fields easily, but not per bundle field storages unfortunately. Bundle fields still shares the same storage, thus making bundle fields not usable for us since we want to store both integers and strings depending on the bundle.

This patch needs an implementation of hook_update_N so as to not break existing sites. I had previously applied other patches from this issue so the schema was not able to be updated to what this latest patch requires. @Jidrone is aware and working on it :)

Adding the hook_update_N for sites already using the module.

Just setting the right name for the patch file.

Patch is starting to look very nice! Bonus points for having tests.

I'm still wondering whether we can't use bundle fields along with this approach. While the following is true:

The core issue you're referring to makes it possible to define bundle fields easily, but not per bundle field storages unfortunately. Bundle fields still shares the same storage, thus making bundle fields not usable for us since we want to store both integers and strings depending on the bundle.

We could define both entity_id and entity_id_str as bundle fields only and define a storage for each one. That way, we don't ship every GroupContent entity with at least one field it will never use (either entity_id or entity_id_str).

We'd have to keep an eye on some bugs regarding manually defining field storages, though. I know I ran into #2584729: SqlContentEntityStorage::getFieldStorageDefinitions() is poorly named and should not be public a while back. The group_roles field works by happy accident because I mistakenly defined its storage as a config entity. Turns out I circumvented a core bug by doing so :) I'd rather define it properly through the right hook, though, as that's where people will look.

P.S.: I'm happy to make this work for now and then have a thorough look into all the other options again when refactoring for Group 8.2.0; who knows what core might offer/fix by then :)

+++ b/src/Entity/Form/GroupContentForm.php
@@ -65,8 +77,12 @@ class GroupContentForm extends ContentEntityForm {
+    $form['entity_id']['#access'] = FALSE;
+    $form['entity_id_str']['#access'] = FALSE;
+
+    if ($this->operation === 'add' && !$form_state->get('group_wizard')) {
+      $entity_id_field = $this->getContentEntityField();
+      $form[$entity_id_field]['#access'] = TRUE;

Can't we do this in a field access hook? Otherwise it only applies to this one form instead of all invocations.

+++ b/group.install
@@ -424,3 +425,28 @@ function group_update_8014() {
+  ¶

Whitespace error in group_update_8015 (near the bottom on the blank line)

We could define both entity_id and entity_id_str as bundle fields only and define a storage for each one. That way, we don't ship every GroupContent entity with at least one field it will never use (either entity_id or entity_id_str).

AFAIR there's no way to _undefine_ the bundle field if it's already defined as an entity type base field.
On the other hand, there's no way to *reliably* define field storage for an entity field other than defining it as base field. So we'll always have two fields.
Another solution could be using `hook_entity_field_storage_info` to define field storages (and then define fields per bundle) but as you've mentioned the hook is broken.

I think the Entity API contrib module has an API for that but it seems to be using an approach which is different from what the core is going to use so I'm doubt we should be using it, also, it would introduce an unnecessary dependency.

There's an easy workaround mentioned in the issue I linked. I'd rather not declare base fields that will never be used by certain bundles. That just shouts "bundle field!" in my opinion :)

Let's try bundle fields now!

I was able to make bundle specific fields work by following all recommendations and suggested workarounds.
I've seen some tests failures on my local machine; uploading the work-in-progress patch to check what Drupal.org CI say.

Remaining issues:
- Failing (?) tests.
- Upgrade path. The group_update_8015() doesn't seem to be sufficient anymore; the entity_id field should be moved to a new database table.

Re-rolled #115 against latest 8.x-1.x since the last patch failed to apply.
group_update_8015() is now group_update_8016().

I'm not making an interdiff since it won't be useful anyway; the 111-115 is mostly the same.

I had a few minutes to read part of the patch and found some quick fixes and the field access thingy I mentioned #112. Starting to look really nice, especially loving the core workarounds being thoroughly documented. Keep up the good work!

1. +++ b/group.module
   @@ -519,6 +518,64 @@ function group_content_entity_submit($form, FormStateInterface $form_state) {
   +    // Field to support entities identified by strings.
   +    $fields['entity_id'] = GroupContentReferenceDefinition::create('entity_reference')
   

   By integers

2. +++ b/src/Entity/Form/GroupContentForm.php
   @@ -41,6 +52,7 @@ class GroupContentForm extends ContentEntityForm {
   +      $container->get('entity_type.manager')->getStorage('group_content'),
   

   We already inject the deprecated entity manager one line below. Why not call for the storage in the constructor? Saves us one dependency injection.

3. +++ b/src/Entity/Form/GroupContentForm.php
   @@ -65,8 +77,12 @@ class GroupContentForm extends ContentEntityForm {
   +      $form[$entity_id_field]['#access'] = TRUE;
   

   Should ideally use the field access system instead of manually manipulating #access.

Hi, just a quick note to say that I've tried the last patch (https://www.drupal.org/project/group/issues/2797793#comment-12846247) and I get the following error on the "Members" Group tab:

Drupal\Core\Database\DatabaseExceptionWrapper: Exception in Group members[group_members]: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'group_content_field_data.entity_id' in 'on clause': SELECT COUNT(*) AS expression FROM (SELECT 1 AS expression FROM {group_content_field_data} group_content_field_data INNER JOIN {users_field_data} users_field_data_group_content_field_data ON group_content_field_data.entity_id = users_field_data_group_content_field_data.uid AND group_content_field_data.type = :views_join_condition_0 WHERE (group_content_field_data.gid = :group_content_field_data_gid)) subquery; Array ( [:group_content_field_data_gid] => 1 [:views_join_condition_0] => test_gtype-group_membership ) in Drupal\views\Plugin\views\query\Sql->execute() (line 1543 of /home/eu/git-mbm/d8clean/docroot/web/core/modules/views/src/Plugin/views/query/Sql.php).

I've checked the db structure, and in fact that column "entity_id" is not present on `group_content_field_data` table:

MariaDB [drupal]> show columns from group_content_field_data;
+------------------+------------------+------+-----+---------+-------+
| Field            | Type             | Null | Key | Default | Extra |
+------------------+------------------+------+-----+---------+-------+
| id               | int(10) unsigned | NO   | PRI | NULL    |       |
| type             | varchar(32)      | NO   | MUL | NULL    |       |
| langcode         | varchar(12)      | NO   | PRI | NULL    |       |
| gid              | int(10) unsigned | YES  | MUL | NULL    |       |
| label            | varchar(255)     | YES  |     | NULL    |       |
| uid              | int(10) unsigned | YES  | MUL | NULL    |       |
| created          | int(11)          | YES  |     | NULL    |       |
| changed          | int(11)          | YES  |     | NULL    |       |
| default_langcode | tinyint(4)       | NO   |     | NULL    |       |
+------------------+------------------+------+-----+---------+-------+
9 rows in set (0.00 sec)

Same happens with the "Nodes" tab.

Hi @anairamzap!
Thank you for your feedback.

This is currently a known issue; the bug you've experienced is caused by the Views integration broken due to the field schema change. This case is already covered by unit tests and we know it's failing; you can see 4 test failures in #116.

I'm working on resolving this issue. Also, it's been discussed at #d8-group on Slack.
I believe the last problems are failing tests from #116, upgrade path and few coding standards/typos issue from #117.

Posting results of what I did with Views integration so far. We should at least get less failures.

This patch may break your code, burn your house or eat your cat. Or even burn your cat, what's even worse.
You've been warned.

Here's a new patch.
Even more dirty hacks, broken coding standards, incomplete code etc. Sorry but I had to upload this in order to check what CI say this time.

Hi @abramm,
I tested the last one, the members view is working for me but the nodes view is not, I will start checking the code to see if I can give a hand.

Another patch with my results.

All tests fixed.
All coding standards issues and typos from #117 are fixed.
@kristiaanvandeneynde as for the field #access, the new code is now following the same approach used in dev branch; the only difference is the field name. Let's raise a new issue if you think this should be changed since my patch is already 67kb (sic!).

Remaining tasks

Upgrade path. The group_update_8016() is not enough anymore since we now need to move entity_id to a new storage.

Fixed View reverse relationship plugin thanks to @jidrone.
The group_nodes view is not covered by unit tests. Views relationship handler changes broke contextual filters in #121/#123.

I'm going to raise a followup task for that.

As for this issue, the remaining task is still an upgrade path.

Finally, the patch with all functionality, all fixes, upgrade path and everything.
Ready for review.

Some initial questions going over the first part of the patch, should have more time later this week. Update hooks are looking nice so far. Don't we need an update hook to add the entity_id_str bundle field to the known fields/storages using the entity definition update manager, though?

1. +++ b/group.install
   @@ -451,3 +452,176 @@ function group_update_8015() {
   +  // We're now inserting new fields data which may be tricky. We're updating
   +  // group_content entities instead of inserting fields data directly to make
   +  // sure field data is stored correctly.
   +  $rows_per_operation = 50;
   +  $query->condition('id', $sandbox['current'], '>');
   +  $query->range(0, $rows_per_operation);
   +  $query->orderBy('id', 'ASC');
   +
   +  $rows = $query->execute()->fetchAllKeyed();
   +  if ($rows) {
   +    /** @var \Drupal\group\Entity\GroupContentInterface[] $group_contents */
   +    $group_contents = \Drupal::entityTypeManager()
   +      ->getStorage('group_content')
   +      ->loadMultiple(array_keys($rows));
   +
   +    foreach ($group_contents as $id => $group_content) {
   +      $group_content->entity_id->target_id = $rows[$id];
   +      $group_content->save();
   +    }
   

   You can't do that, sadly. Saving entities is a bad idea in update hooks; it can only be done safely in post-update hooks.

2. +++ b/group.views.inc
   @@ -5,44 +5,58 @@
   +  if (!$group_content_storage instanceof SqlEntityStorageInterface) {
   +    // Only process views data if Group content entities are stored in the
   +    // database.
   +    return;
   +  }
   

   Are we sure we want to do this?

3. +++ b/group.views.inc
   @@ -5,44 +5,58 @@
        $entity_type_id = $plugin->getEntityTypeId();
   -    $entity_type = $entity_types[$entity_type_id];
   +    $entity_type = \Drupal::entityTypeManager()->getDefinition($entity_type_id);
        $entity_data_table = $entity_type->getDataTable() ?: $entity_type->getBaseTable();
   

   Why use this over loading the entire list just once?

4. +++ b/group.views.inc
   @@ -5,44 +5,58 @@
   -    // This relationship will allow a content entity to easily map to the group
   -    // content entity that ties it to a group, optionally filtering by plugin.
   +    // This relationship will allow a content entity to easily map to the
   +    // group content entity that ties it to a group, optionally filtering by
   +    // plugin.
   

   Unnecessary change?

5. +++ b/group.views.inc
   @@ -5,44 +5,58 @@
          'relationship' => [
            'group' => t('Group content'),
   -        'base' => $data_table,
   -        'base field' => 'entity_id',
   +        'base' => 'group_content_field_data',
   +        'base field' => 'id',
   +        'entity_type' => 'group_content',
   +        'field table' => $table_mapping->getFieldTableName($content_ref_field),
   +        'field field' => $table_mapping->getFieldColumnName($field_def->getFieldStorageDefinition(), 'target_id'),
   +        'field_name' => $content_ref_field,
            'relationship field' => $entity_type->getKey('id'),
   

   I'm not sure this is right, could you explain what you're doing here?

1. I wasn't sure about this; post update hook seems to be an option.
2. I don't remember exactly, I think I've added this since some method we are using (getting SQL table map?) really belongs to SqlEntityStorageInterface.
3. Questionable; I could ask the same. Both approaches works, it's a question of taste. Probably I've changed this while re-writing the code few times, trying different ways etc. I can revert this if you prefer.
4. Yes, you're right, it's unnecessary change. The comment didn't fit the line when I moved this code to a more indented block so I changed it; then I probably moved it back no the same indentation level.
5. I've replaced the logic of the reverse entity reference relationship handler with the code copied from the core entity_reverse handler since the one found in module wasn't handling field data in separate table; the relationship definition was changed as well. The GroupContentToEntityReverse class is now almost the same as core EntityReverse, the only difference is bundles (group content types) filter.
The reason behind this change is issue at #122.

I'm testing the latest patch provided on #125 and I'm facing the following error when installing gnode module:

Drupal\Core\Entity\Sql\SqlContentEntityStorageException: Table information not available for the 'entity_id' field. in Drupal\Core\Entity\Sql\DefaultTableMapping->getFieldTableName() (line 375 of /app/web/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php).

Hi Everyone,

Thanks @abramm for all the progress on this issue.

This patch fixes the error reported in #128, it also have the following adjustments based on comment #126:
1. Moved the saving of the entities to a post update.
2. Removed those conditions, I think it is always true because the group content is a "Content entity".
3. Reverted change, I also consider it was overloading.
4. Reverted change.
5. Nothing changed, because the approach followed by @abramm is the correct for entity reference reverse relationship based core documentation.

Nice progress once again. I'm currently seeing progress on #2584729: SqlContentEntityStorage::getFieldStorageDefinitions() is poorly named and should not be public so let's definitely keep an eye on that!

I don't remember exactly but I think I've added a workaround for broken hook and it should work as is even after the hook fix is committed so it's not blocking us. Also, relying on a hook will force us to require a newer Drupal core version.

Here's a more thorough review of the patch already. I left some @todos in the code and will comment on some changes here:

• Moved the field name logic away from the storage, it has no place there. Not sure GroupContent is the right place, but the storage definitely isn't.
• The naming for both methods to retrieve the field name are awful, we need something better :)
• With the custom field definition class we can reduce a lot of clutter by moving the related code there.
• From what I've seen, it is safe to assume Views uses an SQL back-end, but let's double-check.
• Made sure the string reference schema column is always 255 characters long.

Feel free to quote parts of the interdiff you don't immediately understand and I'll explain further.

Failures show that the views need to use the new relation field names. It might need an update hook that loads all views, then all relations/fields and replaces those occurrences with the right configuration/name.

Also, GroupContentStorageTest apparently still needs the group_test_content module. Oh, I guess this needed to be (re)moved as well:

    $this->installConfig(['group_test_content']);
    $storage->createFromPlugin($group_type, 'integer_content_entity_as_content')->save();
    $storage->createFromPlugin($group_type, 'string_config_entity_as_content')->save();
    $storage->createFromPlugin($group_type, 'string_content_entity_as_content')->save();

    $this->installEntitySchema('group_test_content_entity_int');
    $this->installEntitySchema('group_test_config_entity_string');
    $this->installEntitySchema('group_test_content_entity_string');

But GroupContentTest goes green even if we don't call $this->installConfig(['group_test_content']);... That might need to be looked into as well.

Hi kristiaanvandeneynde,

Thank you for you review!

I've noticed you've changed Views field names in #133 which broke tests:

-      // Create a unique field name for this views field.
-      $field_name = 'gc__' . $entity_type_id;
+      $field_name = GroupContent::getEntityFieldNameForEntityType($entity_type_id);
+      // @todo Can't we read this from a table or field mapping instead?
+      $field_table = 'group_content__' . $field_name;

I've used the 'gc__' prefix since it's the way how it was working before my changes.
We can't use the entity field name name since different bundles of group_content refers to different entity types, e.g. a simple 'entity_id' field may refer both users and nodes and so there should be one field for each referenced entity type.

I'd suggest keeping the same gc__ENTITY_TYPE naming we're currently using since making changes here doesn't look necessary for me.

I'm going to revert your changes in GroupContentViewsData (only those related to Views field names) and see if that helps with unit test.

Here we go. Unit tests now pass locally. I didn't run all tests suite, let's see how CI reacts on this.

Ancient manuscripts says CI would work better if I'd uploaded the patch as well.

Not sure what was wrong with CI but GroupContentStorageTest started failing (which was expected because of the group_test_content module dependency removal) once I fixed Views tests. Re-enabling the module fixed those failures for me.
Uploading a new patch.
I've added interdiffs for both #133 and #138 for convenience.

EDIT:
It's definitely not a good day for CI.
PHP 7 test failed with mkdir(): File exists error in random functional test, then worked fine when I re-ran it.
PHP 5.6 test failed with same error but in a different test.
PHP 5.5 passed just fine.

The tests started failing because setUp() still tries to install the now missing module's config/schema, as mentioned in #135, those lines need to go.

"PHP 7 test failed with mkdir(): File exists error in random functional test, then worked fine when I re-ran it." Yeah, we sometimes get that. Should track that down some day. Never seems to happen locally.