Initiative for Group 1.x compatibility under Drupal 10

I have created a first version of a possible patch. I'm not the right person for UNIT tests, maybe someone else could do that, but the first tests are very promising: tested in a custom project of my own with over 4,000 groups & subgroups, Drupal 10.0.9, PHP 8.1.17, only minor adjustments to custom modules of my own were necessary, but related to Drupal 10. So far everything works well and error free, testing will continue and testing is welcome.

Yup, I want to get this done. I've just been too slammed with other things for the last few months, and most of my paying clients don't need Group module. 😢 I'm about to be AFK for a couple weeks, but hopefully in mid July when I'm back online I can move this forward.

Thanks/sorry,
-Derek

+++ b/src/Access/GroupContentCreateAnyAccessCheck.php
@@ -57,7 +57,7 @@ class GroupContentCreateAnyAccessCheck implements AccessInterface {
+    $group_content_type_ids = $entity_query->accessCheck(FALSE)->execute();

+++ b/src/Access/SynchronizedGroupPermissionCalculator.php
@@ -53,7 +53,7 @@ class SynchronizedGroupPermissionCalculator extends GroupPermissionCalculatorBas
+    foreach ($group_type_storage->getQuery()->accessCheck(FALSE)->execute() as $group_type_id) {

+++ b/src/Entity/Controller/GroupContentListBuilder.php
@@ -85,7 +85,7 @@ class GroupContentListBuilder extends EntityListBuilder {
+    return $query->accessCheck(FALSE)->execute();

+++ b/src/Entity/Controller/GroupListBuilder.php
@@ -157,7 +157,7 @@ class GroupListBuilder extends EntityListBuilder {
+    return $query->accessCheck(FALSE)->execute();

+++ b/src/Entity/Controller/GroupRoleListBuilder.php
@@ -69,7 +69,7 @@ class GroupRoleListBuilder extends DraggableListBuilder {
+    return array_values($query->accessCheck(FALSE)->execute());

+++ b/src/Entity/Form/GroupRevisionDeleteForm.php
@@ -138,6 +138,7 @@ class GroupRevisionDeleteForm extends ConfirmFormBase {
+      ->accessCheck(FALSE)

+++ b/src/Entity/GroupType.php
@@ -160,7 +160,7 @@ class GroupType extends ConfigEntityBundleBase implements GroupTypeInterface {
+    return $query->accessCheck(FALSE)->execute();

+++ b/src/Entity/Storage/GroupRoleStorage.php
@@ -211,7 +211,7 @@ class GroupRoleStorage extends ConfigEntityStorage implements GroupRoleStorageIn
+      $group_type_ids = $this->entityTypeManager->getStorage('group_type')->getQuery()->accessCheck(FALSE)->execute();

+++ b/src/GroupRoleSynchronizer.php
@@ -86,7 +86,7 @@ class GroupRoleSynchronizer implements GroupRoleSynchronizerInterface {
+    $group_type_ids = $this->entityTypeManager->getStorage('group_type')->getQuery()->accessCheck(FALSE)->execute();

+++ b/tests/src/Kernel/EntityQueryAlterCacheabilityTest.php
@@ -81,7 +81,7 @@ class EntityQueryAlterCacheabilityTest extends GroupKernelTestBase {
+      $storage->getQuery()->accessCheck(FALSE)->execute();

+++ b/tests/src/Kernel/EntityQueryAlterComplexTest.php
@@ -519,7 +519,7 @@ class EntityQueryAlterComplexTest extends GroupKernelTestBase {
+    $ids = $this->storage->getQuery()->accessCheck(FALSE)->execute();

+++ b/tests/src/Kernel/EntityQueryAlterTest.php
@@ -303,7 +303,7 @@ class EntityQueryAlterTest extends GroupKernelTestBase {
+    $ids = $this->storage->getQuery()->accessCheck(FALSE)->execute();

The default for an access check is TRUE. This would technically be a regression.

Thanks for the review, here is the update of the patch.

Primarily, the recommendation is to simply upgrade the module from Group 1.x to 2.x, there is no upgrade path for this, but the 2.x version is compatible with Drupal 10.

The bold part is simply not true. I've gone ahead and updated the IS.

Having said that, I see changes like these:

+++ b/group.install
@@ -23,7 +23,7 @@ function group_update_8001(&$sandbox) {
-    $sandbox['ids'] = $storage_handler->getQuery()->accessCheck(FALSE)->execute();
+    $sandbox['ids'] = $storage_handler->getQuery()->accessCheck(TRUE)->execute();

You can't go around and change access checks like that. The D10 requirement is that all entity queries specifically call accessCheck, but ones that were already deliberately set to either TRUE or FALSE cannot be changed. You can check the original issue here: 🐛 D10 compatibility Fixed

That should give you some guidance on what changes we expect to see (e.g. the ProphecyTrait is a false one, don't do that one) and how to split these changes into smaller patches that are easier to review. Ideally, you have one child issue per type of change to make the review process easier.

I just did a quick re-roll add some of the missing voids for some of the test functions so my phpunit would pass.

This doesn't address any of @kristiaanvandeneynde feedback.

Unfortunately in my case, upgrading from version 1.5 to 2.1, removes all previously added groups.

@kristiaanvandeneynde yes, you are absolutely right. existing accessCheck must not be changed of course, I did not pay attention to it. patch is reworked:

• - existing accessChecks are the same
• - in case of not existing accessCheck they are inserted with TRUE
• - ProphecyTrait removed, was an "optimization" of phpStorm

patch reroll with the adjustments from the patch from comment #8

Rerolling patch for failed tests.

The patch is working for us in all automated tests and scenarios in our internal Drupal distro.

The patch is also passing all the module's automated tests for D9 and d10.

It would be great to have another 1.x release with it !

The patch looks good. RTBC.

+++ b/tests/src/Unit/GroupContentEnablerManagerTest.php
@@ -69,13 +69,12 @@ class GroupContentEnablerManagerTest extends UnitTestCase {
-    $this->moduleHandler->getImplementations('entity_type_build')->willReturn([]);

Looking at the patch, why was this line removed?

Line doesn't seem to be needed anymore and tests were green without it.

There were similar changes and comments I could find related to similar fixes:
https://www.drupal.org/project/rules/issues/3346842 📌 [10.0] ModuleHandlerInterface::getImplementations() removed from core Fixed
https://www.drupal.org/project/group/issues/3278740#comment-14838701 🐛 D10 compatibility Fixed

Did a first round of review, found a bunch of config entity queries that had no accessCheck added. This is possible because config entity queries don't run any SQL query. I've made all config entities' access check FALSE except for listing pages so that if core ever starts supporting access checks on config entity queries, the listing pages immediately work as expected.

Further review incoming.

Cross referenced with the v2 and v3 work and looks good. In fact, we might want to check the config entity queries there too. Also replaced accessCheck(TRUE) with accessCheck().

Will tag a new release tomorrow.

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