After 280+ comments in a solution-searching thread, we finally found both an approach and sponsor to allow Group to support the grouping of config entities. This thread will serve to group all of the individual tasks so that we can keep a nice overview of progress.


Here is the summary from that other issue:

  1. We create a content entity type called ConfigWrapper (group_config_wrapper)
  2. This entity type has almost zero UI, routes, operations, etc.; it only exists in code
  3. If you want to be able to group certain config, you write a plugin that targets said config entity type
  4. Then, for each plugin targeting a config entity type, we automatically create a bundle for ConfigWrapper
  5. Behind the scenes, we secretly wrap the config entity in order to add it to a group, but as far as handlers are concerned, they're dealing with the actual config entity
  6. We do not write a UI yet that allows you to add config entities to a group, modules for Menu, Webform, etc. will have to write their own UI
  7. Third-party code should be almost oblivious to the fact that wrappers exist
  8. Query access checks don't even run because config entity types don't use query access
  9. Regular access checks work the same as before because we'll adapt the group content storage to deal with wrappers, so loadByGroup() et al will still work as before

This should lead to minimal impact and actually be fully backwards compatible. The only major change will happen in the group content storage and people directly dealing with queries on any GroupContent table will need to make their code aware of wrappers.

Some important things of note:

  • The wrapper would literally be that, a reference to a config entity
  • It would not have any other fields
  • If the target config entity is removed, so is the wrapper and GroupContent entity
  • If the GroupContent entity is removed, the wrapper remains

In essence: A wrapper cannot exist without wrapped config, but a wrapper existing does not necessarily mean the config entity is grouped.


All contributors to the other issue will be credited here.

Comments

kristiaanvandeneynde created an issue. See original summary.

kristiaanvandeneynde’s picture

Issue summary: View changes

Crediting contributors to related issue.

kristiaanvandeneynde’s picture

More credits, if you feel like I've missed you, ping me on Slack.

kristiaanvandeneynde’s picture

Oof, so many people. Think I've got all of them now.

heddn’s picture

The approach suggested here sounds like the one we took in group content menu. +1 on a path forward.

tstoeckler’s picture

Not opposed to the approach here in particular, but wanted to note that having configuration entities be part of groups is only one use-case for #2797793: Entities identified by strings as group content. The other is content entities with string IDs. Those are not used in core, but are and always have been explicitly supported in core and are often used for business applications where entities are imported from external systems and it is useful to have Drupal use the same ID as the other system(s). Not sure if #2797793: Entities identified by strings as group content should be re-opened for that, if we should have a separate issue for that or in general what your thoughts are on that.

kristiaanvandeneynde’s picture

As per #2797793-250: Entities identified by strings as group content, I'm afraid content entities with string IDs are still second rate citizens in core. Until they are fully supported, I don't see how I can support them. See #2496699: Allow comments to be attached to entities using a string primary key

kristiaanvandeneynde’s picture

Issue summary: View changes

Approach changed a bit during development, plugins now directly can target config entity types.

tstoeckler’s picture

Re #33: I would definitely argue otherwise, but that's neither here nor there. Certainly helpful to have a clear stance from a maintainer on this, that helps to plan how to move forward with that feature request. So thanks anyway, even if I would have appreciated a different response a tad more ;-)

kristiaanvandeneynde’s picture

Thanks for understanding, I'm not opposed to supporting it but I follow Comment's lead as to make sure my approach doesn't get broken by core all of a sudden. So as long as Comment doesn't support it, I won't either.

kristiaanvandeneynde’s picture

Issue summary: View changes

I've managed to significantly simplify how we use wrappers, adjusted the IS to reflect that.

nigelcunningham’s picture

@kristiaanvandeneynde, could I get you to clarify what you mean by "content entities with string IDs are still second rate citizens in core" - that might help in getting some work done on making them be better supported.

Also, I'm missing the context for "I follow Comment's lead" (not seeing someone with the alias Comment - could we get a link to the message for future reference?

Thanks!

kristiaanvandeneynde’s picture

Re #38, Group can add many entity types into a group, just like Comment in core can attach comments to many entity types. The safest bet years ago to deliver this similar "any entity type goes" logic was to copy it from Comment to make sure that our use case would not all of a sudden become impossible because of changes in core. After all, if Comment supports it, core supports it.

So far, Comment only supports entities with integer IDs, so that's as far as Group goes. If you want to see this fixed, then try and get the link I posted in #33 fixed.

kristiaanvandeneynde’s picture

The functionality works now, just added a few clean-up tasks.

kristiaanvandeneynde’s picture

Status: Active » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.