Add compatibility with Group v2 and v3

Christian.wiedemann → made their first commit to this issue’s fork.

Group 3.x uses group_relationship instead of group_content. Also the corresponding classes is changed to GroupRelationship.
I added a addtional Mr. Propably a entitygroupfield is need for each group version

To use the patch you need to replace the repository in your composer.

 "repositories": {

        "entitygroupfield": {
            "type": "package",
            "package": {
                "name": "drupal/entitygroupfield",
                "version": "1.0.x-dev",
                "type": "drupal-module",
                "source": {
                    "url": "https://git.drupalcode.org/issue/entitygroupfield-3306512.git",
                    "type": "git",
                    "reference": "3306512-breaks-website-on-group-3"
                }
            }
        },

I'm on Group v3, I've installed this module, I ran into the error, coudl not apply the patches and now I can't uninstall and remove this module.

Thanks for working on this! I just marked a bunch of other issues as duplicates with this, since this is both the oldest issues, and the furthest along.

Propably a entitygroupfield is need for each group version

Bummer, I was hoping to avoid that. 😅 Perhaps I should leave the 1.0.x branch for compatibility with group 1*, skip 1.1.x 😬, start a 1.2.x branch for group 2.* compatibility, and a 1.3.x branch for group 3.*? It doesn't really make sense from a semver perspective for this module to jump major versions just because one of its dependencies jumps major versions...

Are we sure that MR 4 only works with group 2.x and MR 8 only works with group 3.x and neither still works with group 1.x? I don't have time tonight to start testing all the combinations, but I hope to do so soon.

Thanks again,
-Derek

Also, this isn't exactly a bug. We never claimed to support Group v2 or v3, yet. 😅 Changing this to a task and updating the title...

Not sure if this belongs here, but just FYI that the tests below are using the since-removed permission "bypass group access"

/tests/src/FunctionalJavascript/EntityGroupFieldWidgetTest.php:85:      'bypass group access',
/tests/src/Functional/EntityGroupFieldFormatterTest.php:49:      'bypass group access',
/tests/src/Kernel/GroupAutocompleteFormElementTest.php:131:        'bypass group access',

Noticed this when trying to uninstall the Group module and start over with G3 on a new build.

@scotwith1t: Thanks, that's helpful info, and yes, here is perfect. That said, I thought I had an issue open somewhere bemoaning the need to test with that permission 😉 But alas, I don't see it in the queue. Maybe I'm thinking of commit d5a7750. It's probably a good thing to stop having our tests rely on that perm. So maybe we should open a follow-up about ripping that out, regardless of V2/V3 porting.

Tested MR 8 with Group 3 - works well.

The last submitted patch, 7: breaks-3306512-7.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 10: breaks-3306512-10.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Thanks for testing MR8 with Group V3. Per the MR comment I just opened, we also need to confirm if that same code from MR8 works with Group V2 or not.

Also, will clearly need a new version for this. Looking more closely at the changes, some of them are pretty disruptive / breaking API changes if anyone was trying to extend entity_group_field. I should probably open a 2.0.x branch and only plan to commit this for a 2.0.0-alpha1 release, not the 1.0.x branch.

Verifying this does not work with Group v2. I can install it and the field is available and can be enabled on group content types, but the manage display tab for content types with the field enabled pops an error:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "group_relationship" entity type does not exist. in Drupal\Core\Entity\EntityTypeManager->getDefinition() (line 139 of /Users/fm56/Documents/Jobs/aws/hg/stage/web/core/lib/Drupal/Core/Entity/EntityTypeManager.php).

On creating new content with the field the Add to Group button churns for a long while but doesn't do anything but logs a similar error:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "group_relationship_type" entity type does not exist. in Drupal\Core\Entity\EntityTypeManager->getDefinition() (line 139 of /Users/fm56/Documents/Jobs/aws/hg/stage/web/core/lib/Drupal/Core/Entity/EntityTypeManager.php).

I'm thinking it ought to be possible for this module to check what version of Group is running and substitute group_content and group_content_type accordingly. Easier than trying to maintain parallel versions. I'm going to take a stab at writing a patch because I need this module rather intensely.

@phl3tch: Thanks for testing!

I can confirm now that if a few conditionals let a single branch work for both Group V2 and V3, I'd be thrilled and would probably commit/release that. In general, I think that would be better than having to maintain separate branches, and would make it easier for sites to upgrade from Group V2 to V3 when they're ready to do so.

The only thing I can foresee as a problem is that I'm not sure how to configure the d.o testbot so that we could run the test suite against both Group V2 and V3. If they're in separate branches, each branch can have its own specific dependency and then it's all simple and clear. If a single branch of ours can support multiple branches of Group, I don't know if testing a "dependency matrix" is possible or not. I'll do some research on this point...

FYI: I opened a Slack thread about this in the #testing channel https://drupal.slack.com/archives/C223PR743/p1681848554422859 -- hopefully someone has bright ideas. 😅

Yay, @moshe pointed me to an example of some "dependency matrix" testing using GitLab CI.

You dont need to maintain different branches. This is easier to do with bleeding edge of Gitlab CI. This is how keycdn does matrix testing https://git.drupalcode.org/project/keycdn/-/blob/8.x-1.x/.gitlab-ci.yml
Instead of composer-d9 and composer-d10 you want composer-group2 and composer-group3. You temporarily need to add a CI env variable to your project for this to work. I'm happy to help as you dig in.

I'll have to dig in more deeply once this is getting ready, but in principle, my concerns about testability are basically resolved, and I'd be very happy to only have to maintain a 2.0.x branch to support both Group V2 and V3, instead of having 3 separate branches here.

Thanks again!
-Derek

As you're probably aware, the patch for v2 compatibility is drop dead simple. Including it here. I didn't do the group version check because it sounds like the GitLab thing has it covered.

I assume the patch I submitted failed because it has to be applied atop MR8. No idea how to specify that when I upload the patch.

Thanks for working on this!

Yeah, you can't apply patches to MRs (automatically). You can either:

1. Push more commits directly to https://git.drupalcode.org/issue/entitygroupfield-3306512/-/tree/3306512... (for MR8)
2. Create a new branch from `3306512-breaks-website-on-group-3` (called something like `3306512-breaks-website-on-group-2-and-3`), apply your patch to that, commit it, push that to this issue fork repo, and open a new MR.
3. Upload a combined patch instead of using the MRs at all.

I'm happy with #1 and that's easier, but in some cases, #2 might be more appropriate. Some folks do #3, but the community is trying to migrate to entirely using GitLab for issues. Direct patch uploads aren't going to be around forever, so it's good not to rely on those. But if that's easier than dealing with GitLab, I totally understand and would still review such a patch. 😅

Thanks again!
-Derek

After a moderate amount of testing I'm confident saying that the current state of MR8 plus the patch in #30 functions with Group 2.x such that I'm moving forward with that on my site.

@dww Having a single branch compatible with Group 2 and 3 is great news! I don't know the intended implementation for that, so I'll leave it to you.

I'm looking over the existing tests. Due to the removal of the 'bypass group access' permission and other changes in role management, the tests need some big help. I hope to get something working there. I see you have GitLab CI access as of a few hours ago so running tests through this MR would be great.

I got EntityGroupFieldWidgetTest passing on MR8, phew. I didn't touch any of the assertions, just the group setup and the widget config for required fields.

I learned that group 2.x and 3.x, while they don't have any explicit PHP version constraints in composer.json, have syntax that can fail on PHP 7.3 or lower. I assume there's no special mention of this since PHP 7.4 and lower are all out of support. Anyway the PHP 7.3 tests will always fail due to this.

This is a follow-on from the patch in #30 with more things set back to the Group 2.x machine names. Running tests locally gives the same results now with (1) the MR8 branch with Group 3.x and (2) the MR8 branch plus this patch with Group 2.x. Some of those test assertions are still errors.

That just updated the branch `3306512-breaks-website-on` as Group 2 compatible to be the same as `3306512-breaks-website-on-group-3`, with the appropriate type machine name differences.

Thanks @ekes, I had glossed over that branch, but your solution is good. I can confirm that MR4 is Group 2.x compatible, and identical to the MR8 plus #35 patch I had done. I've hidden away the patches in favor of the two PRs for now. @dww mentioned the ability to do Group 2.x and 3.x compatibility in one branch so eventually these will become one PR but I'm still not sure how that will be accomplished.

I am on this again.

I wouldn't recommend what #14 comment says, as this imply security issues on the site applied.
MR participation is open, so anyone could add arbitrary code directly to your site.

I am adding D10 to be able to test it on Drupal 10, also removing drupal/core requirement from composer.json as https://www.drupal.org/node/2514612#s-drupal-9-compatibility → says.

Finally green!

After properly reviewing, a release for group v2 will be great. Then, when the needed conditionals are there, it could be added support for group v3 as well.
So I am marking it as needs review.
I understand that may be better to have just one new branch, but maybe we need to supersede this issue to a new one to work on v3 support. This long issue is scaring everyone as there are too many variables playing in this thread : group version, php version and drupal version.

Resolved the final notices from upgrade_status for Drupal 10. `core/once` was introduced in Drupal 9.2 so I bumped the info file's core_version_requirement to that. Group 2.x requires ^9.5 so this module de facto only supports ^9.5 anyway.

I also believe that the libraries besides `core/drupal.dropbutton` don't need to be depended on here but I don't have the time to prove that so I won't touch them.

The claro theme doesn't render the custom dropbutton implementation correctly but this occurs even before this libraries update.

I introduced two functions to handle group_[content/relationship] and group_[content/relationship]_type which provides both group 2.x and group 3.x support and tests are passing. @dww, you're good to do #29 now.

dww → closed merge request !8

Thanks to everyone who has been moving this forward! I'm thrilled that MR 4 is now hopefully compatible with both Group V2 and V3! 🎉🙏 To hopefully minimize confusion, I closed MR 8.

💯 agree with #39 that #14 is dangerous practice that we should complete avoid. Sorry I didn't say so earlier. 😉

Re: #42

The claro theme doesn't render the custom dropbutton implementation correctly but this occurs even before this libraries update.

See 🐛 Broken dropbutton in Groups field when used with Claro Fixed for that. Don't need to mess with that here.

Re: #43 and #29: 📌 Configure GitLab CI Fixed is where I added .gitlab-ci.yml for the 1.0.x branch. I'm going to work on configuring it for Group V2 + V3 locally now, and merge that into this issue branch. Stay tuned.

But first, MR 4 needs a rebase. Just pushed that much. Let's see what the Gitlab CI pipeline does with it: https://git.drupalcode.org/project/entitygroupfield/-/pipelines/12580

Weird. I pushed commit a339244 to a new 2.0.x branch here, but it's not getting automatically linked in this issue. 🤔

Sadly, getting the matrix testing for GitLab CI in here isn't trivial. 😢 I tried for a while locally, but as this is my first time messing with GitLab CI config, I wasn't successful. But I didn't want to block this any further on that problem, so I'm splitting that off to a follow-up: 📌 Configure GitLab CI matrix testing Fixed .

Therefore, I manually did the matrix testing for the code in MR4 by running the full test suite on both D9 and D10, with both Group V2 and V3. All's well! 🎉

YAY! This is now done and merged. Thanks everyone!! 🙏

dww → closed merge request !4

Great! Thank's for your work derek! 🎉

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

seantiago24 → changed the visibility of the branch 3306512-breaks-website-on to active.

seantiago24 → changed the visibility of the branch 3306512-breaks-website-on to active.