🇳🇱Netherlands @Jan-E

Simplified example of how I added lots of cases (subgroup) to projects (group and subgroup v.1):

	$group_creator = $some_uid;
	$group_creator_account = \Drupal\user\Entity\User::load($group_creator);
	$group = \Drupal::entityTypeManager()->getStorage('group')->create(['type' => 'case']);
	$group->setOwner($group_creator_account);
	$group->set('label', $somelabel);
	$group->set("status", 1);
	$group->save();
	$caseid = $group->id();
	$pluginId = 'subgroup:case';
	$project = \Drupal\group\Entity\Group::load($projectid);
	$relation = $project->getContentByEntityId($pluginId, $caseid);
	if (!$relation) {
		$project->addContent($group, $pluginId);
	}
	$project->save();

The same happened here. Lesson learned.

+1 from me as well. I am using #4 📌 "Delete" is misleading word when removing entities from group Needs review in group 1.x for 2 years by now.

Reroll of #5 🐛 Access checking must be explicitly set on entity queries RTBC for Group permissons 1.0.0-alpha10. #5 applied quite clean against that release, except for a

Hunk #1 succeeded at 467 (offset -13 lines).

I also tested unchecking some outsider permissions in a group instance. Works OK!

The same problem exists in Group Permissions 1.0. Did saomebody already make a patch for 1.0.x?

I could reproduce the issue on a local copy of my site.
Then I applied your MR and did not run into the error 500 anymore.
If that count as RTBC, feel free to change the status.

With respect to an update hook to clear the values: it would be a tricky one, because you cannot change every 86401 value into 0. You have to actually delete the field instance and possibly also a revision instance.

A 3.x version of the patch or a MR would be welcome in the related issue.

In my case (see related issue ✨ Support empty time range values Fixed ) it was also a time field. Moreover, it was intentionally hidden from some users by a hook_form_alter() statement:

$form['field_pmto_session_start_time']['widget']['#access'] = false;

Despite it being hidden the value that was entered into the database was 86401. No post-processing was done on the field, so I still do not know where the 86401 originated.

BramDriesen → credited Jan-E → .

Do not forget to change ‘Delete’ into ‘Remove’ here:
https://git.drupalcode.org/project/group/-/blob/14b44be5e2e0debd9d92d2ea...

Related https://www.drupal.org/project/group/issues/3101314 📌 "Delete" is misleading word when removing entities from group Needs review which I am using ears by now.

@BramDriesen thanks for getting back on this.

I just had this once again. Somehow the exact value 86401 gets entered in a time field. When cron runs it apparently tries to clear up the entity cache and falls into the error.

The previous occasion I remedied it by manually deleting the field instance and the revision instance from the database. I will try to change the value into 86400 now. And will try to find out how users manage to enter 86401.

This was the first time it happened, on a time that almost nothing was done on the site:

@BramDriessen I really would not know. It also happened when a cron was invoked from outside the site.

After the update from 2.1.0 to 2.1.1 I am getting this error during a cron:

InvalidArgumentException: Provided value is out of range. in Drupal\time_field\Time::assertInRange() (line 64 of modules/contrib/time_field/src/Time.php).

A few lines of the stack trace:

#0 modules/contrib/time_field/src/Time.php(145): Drupal\time_field\Time::assertInRange('86401', 0, 86400)
#1 modules/contrib/time_field/src/Plugin/Field/FieldFormatter/TimeFormatter.php(36): Drupal\time_field\Time::createFromTimestamp('86401')
#2 modules/contrib/time_field/src/Plugin/Field/FieldFormatter/TimeFormatter.php(47): Drupal\time_field\Plugin\Field\FieldFormatter\TimeFormatter->viewValue(Object(Drupal\time_field\Plugin\Field\FieldType\TimeType))

Looks like the same error as in #15 ✨ Support empty time range values Fixed . I am running Drupal core 10.2.3.

My solution so far to extend upon what is listed above is to remove SQL rewrite from the view (which then gives group admins ability to see more items than they have access to) and then added a views_pre_render hook to remove items from the list which do not have node_access. This works and mostly completes what i am trying to do here (limit views list and node access the same) but it does break paging.. so possibly a cleaner solution?

.
Disabling SQL rewrite and adding a direct ffilter with group membership would restore paging. But then we are adding our own group content filters, while the Groups module should do that for us.

Disabling SQL rewrite in the 'your recent group content' view is no problem, because the access check in that view is done by comparing the uid of the current user with the uid of the node author. The real challenge is a view with all recent content for users that are member of a few groups, not all. There are 9 groups at the moment in my site. One user is supervisor of 4 of the 9 groups and should see only the recent group content in those groups. I tested disabling SQL rewrite in the 'all recent group content' view: the effect was that the recent content in all groups showed up. Clicking to see the details of a node like that gives a 'not authorized'. But the view already revealed far too much. I had seen that behaviour before at a lower (subgroup) level and developed a special field that shows 'No access' (Dutch: Geen toegang). That could lead to views like this (if you know the URL of that page):

Fine at the lower level but no option for the recent group content at the frontpage. So I am forced to use a View with SQL rewrite and access checks. This view shows all recent additions to older group content types like 'Video', 'Document', 'Reflectieformulier'. But does not show new nodes for the new group content types: 'PMTO Sessieformulier' and 'PMTO Gezinsformulier'. On restricted pages like '/group/*' I can disable SQL rewrite, because I do not need node or group access working at that level. Groups correctly grants or denies access to group nodes.

In fact I decided to merge 'PMTO Sessieformulier' into 'Reflectieformulier'. They have a similar function and as 'Reflectieformulier' (still?) correctly works in SQL rewrite disabled views, I can use that group content type to display one of the 2 field sets when adding and editing. Clumsy, but a workaround at the moment. So the only remaining problem for now is that new nodes of 'PMTO Gezinsformulier' do not show for anyone but the author at the frontpage. I fear the moment that new group content types have to be added to the site.

Disabling SQL rewrite works OK, but I can only use it in Views on pages that are prohibited for people without enough rights. My site has a view on the frontpage showing 'Your content items' for the current user. As administrator (and a view that has an exposed filter on user) I can see all the content that the user has created:

But the user sees this in the 'Your content items' view:

'Video' and 'Reflectie form' are Group content types that I added mid 2022, bit the group nodes were added this month. 'PMTO Sessieformulier' and 'PMTO Gezinsformulier' are new Group content types. The Views show the content item in descending order of changes in the node. The user has exactly the same permissions for all content types. That is for the content type itself, but also for the group plugins (/admin/group/types/manage/--type--/permissions).

In #2 💬 Group access doesn't work in Views. Active you said:

setting Member to View Any and it then matches the Group Admin: can no see own node pages but items do not show in the view.

Both screenshots above are of a View. But in groups v.1 with a God-like admin the admin stiil can use Views to access all nodes. Bewilering are the differences between the old content types and the new ones.

This really is a nasty bug. @kristiaanvandeneynde or @dww Do you have any hints how to debug this? I am out of ideas.

I had a similar issue where a group member could add, view, edit and delete group content, but still the content did not show up in my view of the content. 'All entities' did show all group content (and users) of the group. All entities is no View:

'Reflectie form' is a group content type that I created mid 2022. 'PMTO Sessieformulier' I added this month. Whatever I tried the first one shows up in views and the newer content type does not. See the Nodes:

My own group content view did also only show the oldest content type, not the recent one. So I went looking around for similar issues and saw your remark about disabling SQL Rewrite. That finally did the trick!

I do not have to filter out content that should not be seen, because it is a group/subgroup/subsubgroup structure. Every member of the subgroup or member of a higher group with inherited rights is allowed to view all subgroup content.

But I am still at a loss why 'PMTO Sessieformulier' does not show and 'Reflectie form' does not. All recent group content types fail to show up, the older ones work as expected. Hints where to look are eally appreciated.

@david.muffley in #31 🐛 403 error for views.ajax route on Group related views (with AJAX enabled) Needs work : can't you use $_REQUEST in stead of the switch between $_GET and $_POST?

Jan-E → created an issue.

FWIW: I re-applied patch #8 ✨ Filter by member-state in rules action "Get group members from group audience" Needs work to OG 7.x-2.11.

I had a comparable composer.json as @jomcar and got comparable problems.

composer require drupal/core-recommended:10.0.0 drupal/core-composer-scaffold:10.0.0 drupal/core-project-message:10.0.0 --update-with-all-dependencies --dry-run
./composer.json has been updated
Running composer update drupal/core-recommended drupal/core-composer-scaffold drupal/core-project-message --with-all-dependencies
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires drupal/core-recommended 10.0.0 -> satisfiable by drupal/core-recommended[10.0.0].
    - drupal/core-recommended 10.0.0 requires psr/log ~3.0.0 -> found psr/log[3.0.0] but these were not loaded, likely because it conflicts with another require.

Tried a lot of the suggestions in this issue, but did not succeed. For me deleting composer.lock seemed to do the trick. With a --dry-run:

Installing dependencies from lock file (including require-dev)
Package operations: 7 installs, 35 updates, 11 removals
  - Removing typo3/phar-stream-wrapper (v3.1.7)
  - Removing symfony/translation (v4.4.47)
  - Removing symfony/polyfill-php73 (v1.28.0)
  - Removing symfony/http-client-contracts (v2.5.2)
  - Removing symfony/debug (v4.4.44)
  - Removing symfony-cmf/routing (2.3.4)
  - Removing stack/builder (v1.0.6)
  - Removing longwave/laminas-diactoros (2.14.2)
  - Removing laminas/laminas-stdlib (3.18.0)
  - Removing laminas/laminas-feed (2.22.0)
  - Removing laminas/laminas-escaper (2.13.0)
  - Upgrading drupal/core-composer-scaffold (9.5.11 => 10.0.0)
  - Upgrading drupal/core-project-message (9.5.11 => 10.0.0)
  - Downgrading symfony/polyfill-intl-grapheme (v1.28.0 => v1.27.0)
  - Downgrading symfony/string (v6.3.5 => v6.2.13)
  - Upgrading psr/container (1.1.2 => 2.0.2)
  - Upgrading symfony/service-contracts (v2.5.2 => v3.1.1)
  - Upgrading symfony/deprecation-contracts (v2.5.2 => v3.1.1)
  - Upgrading symfony/console (v4.4.49 => v6.2.13)
  - Upgrading psr/log (1.1.4 => 3.0.0)
  - Upgrading consolidation/log (2.1.1 => 3.0.0)
  - Upgrading psr/cache (1.0.1 => 3.0.0)
  - Upgrading twig/twig (v2.15.5 => v3.4.3)
  - Upgrading symfony/yaml (v4.4.45 => v6.2.10)
  - Upgrading symfony/translation-contracts (v2.5.2 => v3.1.1)
  - Upgrading symfony/validator (v4.4.48 => v6.2.13)
  - Upgrading symfony/serializer (v4.4.47 => v6.2.13)
  - Upgrading symfony/routing (v4.4.44 => v6.2.13)
  - Upgrading symfony/http-foundation (v4.4.49 => v6.2.13)
  - Upgrading symfony/process (v4.4.44 => v6.2.13)
  - Upgrading symfony/mime (v5.4.13 => v6.2.13)
  - Installing psr/event-dispatcher (1.0.0)
  - Upgrading symfony/event-dispatcher-contracts (v1.1.13 => v3.1.1)
  - Upgrading symfony/event-dispatcher (v4.4.44 => v6.2.13)
  - Upgrading symfony/var-dumper (v5.4.29 => v6.2.13)
  - Upgrading symfony/error-handler (v4.4.44 => v6.2.13)
  - Upgrading symfony/http-kernel (v4.4.50 => v6.2.14)
  - Installing symfony/var-exporter (v6.2.13)
  - Upgrading symfony/dependency-injection (v4.4.49 => v6.2.13)
  - Upgrading guzzlehttp/psr7 (1.9.1 => 2.4.5)
  - Installing psr/http-client (1.0.3)
  - Upgrading guzzlehttp/guzzle (6.5.8 => 7.5.3)
  - Upgrading asm89/stack-cors (1.3.0 => v2.1.1)
  - Upgrading drupal/core (9.5.11 => 10.0.0)
  - Upgrading drupal/core-recommended (9.5.11 => 10.0.0)
  - Upgrading symfony/polyfill-php80 (v1.27.0 => v1.28.0)
  - Upgrading grasmash/expander (2.0.3 => 3.0.0)
  - Upgrading consolidation/site-process (4.2.1 => 5.2.0)
  - Installing symfony/polyfill-php81 (v1.28.0)
  - Installing phootwork/lang (v3.2.2)
  - Installing phootwork/collection (v3.2.2)
  - Installing phpowermove/docblock (v4.0)
  - Upgrading consolidation/robo (4.0.2 => 4.0.6)

Maybe deleting composer.lock is a common strategy for experienced composer users, but I did not know that it could work.

RTBC +1

Sorry. I should have asked you if that could be done as well. And it was a reminder to my self that that fix was not in the 1.0.x branch yet.

Thanks a lot! For a final Subgroup 1.x release the fix for doAddLeaf & doRemoveLeaf: timestamp bug and scalability 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed should be merged in the 1.0.x branch as well. You only merged that fix into the 2.0.x and 3.0.x branches as far as I know.

@mqanneh Is there something we can do to help you with a fix? My sites are nagging me about a security update that I cannot install.

Looks like this was caused by the fix for issue 3342236 📌 Fix the issues reported by phpcs Fixed , which increased the number of arguments from 3 to 5.
See https://git.drupalcode.org/project/mail_login/-/commit/929c4fa94a9cc2c11...

RTBC for the patch at #8 🐛 Wrong path of required.svg in webform.css (reopen) RTBC +1

Patch in #19 🐛 403 error for views.ajax route on Group related views (with AJAX enabled) Needs work was already applied in Group 8.x-1.6.

Fixed in Group 8.x-1.6

The patch in #4 📌 "Delete" is misleading word when removing entities from group Needs review applied without problems on Group 8.x-1.6

Group 8.x-1.6 added support for D10 now. How about Subgroup?

For the record: #56 🐛 Last access by a user should not change after masquerade as that user (D8) Fixed applies without problems to masquerade 8.x-2.0-rc4.

RTBC +1

Well. 2 failing tests fixed. A bit better.

Thanks for adjusting the patch for masquerade 8.x-2.0-rc3. Let us see what the attached patch does in the tests.

Aren't you mixing up $this->authUser and $this->auth_user in Functional/MasqueradeTest.php?
$this->auth_user is undefined in the first line of function testMasquerade().

Sorry, I overlooked this one. See https://gist.github.com/Jan-E/20218ef0b471756e0075287d3df761bc
Very unedited and only applicable with our structure: project = tree, case = subgroup, session = subsubgroup.
The idea is that you loop through the trees, loop through the subgroups below it, loop though the subsubgroups below the subgroups, start counting and formulate the queries to re-initialize all left & right values. With the argument tfr-rebuild-trees.php?execute=1 you execute all the queries.
db_left and db_right are the values from the database, sb_left and sb_right is what they should be. If they differ the query will bolded. See screenshot, made on my iPad. With no differences, because it was a long time agon that I had a corrupt tree.

Added related issue for the Group 8.x-1.x branch.

I will take a look at it next week.

Added a reference from Add Views data automatically ✨ Add Views data automatically RTBC as the Computed Field Plugin → module also relies on this issue.

Silly thing:

drush pm-update

stumbles over ::class in system.module. The site was running under PHP 7.1, but drush was using PHP 5.4.41. Applying the patch manually fixed things. I really need to update php in drush (on a Windows server),

The same happened with https://www.drupal.org/project/subgroup/issues/3281672#comment-15005592 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed

There were patches for v1 and v2/v3 and only those for v2 and v3 were committed. The one for v1 is in production now at my site for some weeks and I was really surprised that a patch for a major issue did not get committed in the subgroup v1 branch.

Or do we wait until @dww has got some spare time?

I also totally disagree with the description that @zcht gave of Open Source. It is perfectly normal to get paid for working on a open source project. And I do share @kristiaanvandeneynde ‘s astonishment that none of the agencies with dozens of customer sites relying on group and subgroup have stepped up to make sure that both modules will get D10 support. In my case it only is a single site and a single customer, so I cannot contribute much (besides my time).

Let us continue with this comment by @jurgenhaas:

Maybe this needs some special attention from a team of people, and you/we may have to ask for help. Because my feeling tells me that just letting this go until everybody woke up, may lead to some disaster. If that could be prevented, let's pull all the strings required. I'm happy to help when I can, and I'm sure others will be there too. But that should not be part of this issue, I guess.

Shall we open a new issue in the issue queue of the group module to attract attention for this problem? Would not that be the best way forward?

They may not be officially EOL, but both group and subgroup v1 do not get commits anymore. The same happened today for a major issue in this module: https://www.drupal.org/project/subgroup/issues/3281672#comment-15014560 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed
The patch for subgroup v1 was not committed.

Hidden inside an issue: https://www.drupal.org/project/group/issues/3313220#comment-14797177 🐛 permission_callbacks is not handled correctly RTBC

I suppose you are implying that subgroup 1.0.x is EOL by not committing a patch for v1? Then it might be better to make a clear statement about this.

Ok, then the patches at #123 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed are the final ones.

Trying to solve the test failure and the coding standards issue.

I do not know (yet) why the tests in #125 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed fail. The tests with the patches in #123 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed still pass. Also note that the patches in #135 have a coding standard issue (line too long).

Removing the old code for doAddlLeaf() and doRemoveLeaf() in SubgroupHandlerBase.php stikk needs some work.

@kristiaanvandeneynde Should not doAddlLeaf() and doRemoveLeaf() in SubgroupHandlerBase.php be abstract protected functions, just like writeLeafData()?

New patches with abstract functions in SubgroupHandlerBase.php. Tested in production for some time now.

The odd thing is that every patch since #111 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed still contains the old (and buggy) code for doAddlLeaf() and doRemoveLeaf() in SubgroupHandlerBase.php. A brief excerpt of classes and functions follows below.

SubgroupHandlerBase.php
abstract class SubgroupHandlerBase extends EntityHandlerBase implements SubgroupHandlerInterface { ... }
contains old doAddlLeaf() and doRemoveLeaf() and
abstract protected function writeLeafData(EntityInterface $entity, $depth, $left, $right, $tree);

GroupSubgroupHandler.php
class GroupSubgroupHandler extends SubgroupHandlerBase { ... }
contains new doAddlLeaf() and doRemoveLeaf()

GroupTypeSubgroupHandler.php
class GroupTypeSubgroupHandler extends SubgroupHandlerBase { ... } contains
protected function writeLeafData(EntityInterface $entity, $depth, $left, $right, $tree) { ... }

@kristiaanvandeneynde Should not doAddlLeaf() and doRemoveLeaf() in SubgroupHandlerBase.php be abstract protected functions, just like writeLeafData()?

Try installing and enabling Flexible Permissions → before doing the update to Group 2.0.x.

The core issue in #35 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed is still getting updates. When I followed the links to the related issues I noticed https://www.drupal.org/project/drupal/issues/2347867#comment-14606651 🐛 Race conditions with lock/cache using non-database storage - add a non-transactional database connection Needs work . Which included the phrase:

I believe #44 is correct: cache operations should not happen inside a transaction.

This made me think. Is there a real reason to put the resetCache($ids_to_update) inside the transaction? No, there seems to be no reason. All statements for the resetCache should be in between the acquireLock() and releaseLock(), but not inside the transaction. It might even be preferable to have the resetCache after the transaction, so resetCache($ids_to_update) will be even be executed when the transaction fails.

New patches attached. I will put the status back again to 'Needs review' until the modified patch is tested for some time on my production server.

The real difference is being made if you have groups with multiple subgroups. There must be some in your system because you have 9500 subgroups and 'only' 9000 groups. Try this as a test:

• Create a new group
• Add a lot (say 20) subgroups under that new group
• Delete the 1st subgroup under the new group

Without the patch subgroup will cycle to all newer subgroups and save them one by one. If you do not have one yet, Create a view with the 'Changed on'

Changed on	Group	The time that the group was last edited.

You will notice that newer subgroups will seem 'edited' after removal of the first one.

With the patch the tree values (left and right) of all the newer groups and of the parent group will be updated with a direct query to the database. No more cycling through all newer subgroups.

In my case I have got only 9 groups. One of those 9 groups has in total 6360 subgroups If I remove one of the first subgroups it will take more than 3 minutes to cycle through all the affected subgroups. With the patch the update is done in about 0.4 seconds.

There is not anything of a real community here, but I am changing the status to RTBC anyway.

The patch in #115 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed applies unchanged to subgroup 2.0.x-dev as well. Tests pass. I do not have a site with subgroup 2.0.x, but am quite confident that it will fix the issue for that version as well.

The patch in #116 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed is in production now for a couple of days. No flaws at all.

I wanted to update the parent subgroup (case) when a new subsubgroup (session) was added and used hook_ENTITY_TYPE_insert() to achieve that:

/**
 * Implements hook_ENTITY_TYPE_insert().
 */
function pcs_module_group_insert(GroupInterface $group) {
  if ($group instanceof GroupInterface) {
    $parent_id = 0;
    $group_type = $group->bundle();
    $group_id = $group->id();
    if ($group_type == 'session') {
      $parent_id = 0;
      // add:  /group/76/content/create/subgroup:session
      // Retrieve an array which contains the path pieces.
      $current_path = \Drupal::service('path.current')->getPath();
      $path_args = explode('/', $current_path);
      if (isset($path_args[2]) && intval($path_args[2])) $parent_id = intval($path_args[2]);
    }
    if ($parent_id) {
      $parent = \Drupal\group\Entity\Group::load($parent_id);
      $parent->changed = time();
      $parent->save();
    }
  }
}

Please be aware that hook_group_insert() is called before the addition of the tree values to the new group. Be really sure that you do not cause PHP errors in that piece of code. Otherwise you will end up with a subgroup that was born as an orphan.

I have put this patch for subgroup version 1.0.2 in production now. The patch is almost identical as the one for subgroup 3.0.x-dev. My own tests all were successful.

Fixing coding standards issues - take 2

Fixing coding standards issues

Did I see it right that you do not update the direct parent anymore on adding a leaf, but only update the right value? My users are getting used to the fact that a case (subgroup) gets an updated timestamp when a new session (subsubgroup) is added to that case. Also comes in handy on views with only subgroups. If so, I will have to add it to my own module.

You might be right about removing the 60 seconds assert. While doing my initial import of 6000+ groups I temporarily removed it. During the importing process I also ran a couple of times into the core issue, even while running it with a single user. Never happened again after the one reported in this issue. Fingers crossed.

Glad to see you are working on porting the patch to 3.0.x-dev. As a BTW: the 3.0.x-dev patch introduced 7 new coding standard messages, which should be avoided.

Thanks. i will review that later this week and report back.

Are you considering a mitigation against a fatal error after storing a new group in the database, but before the leaf values are updated? So we can avoid things like #35 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed . That would require starting a transaction before adding the group. You were considering adding a flag for “Created during this request” in https://www.drupal.org/project/subgroup/issues/3163044#comment-14912931 📌 Leaf lifetime exceeds 60s in large migrations Fixed Could that be the start of a transaction in stead of adding a flag?

I totally agree with you that we should fix the D10 compatibility for Subgroup 1.x. Are you working on a patch?

This patch has been in production for 5 weeks now. No issues detected at all. In the mean time the system has grown to

• 9 projects (groups)
• 945 cases (subgroups) below the projects
• 6570 sessions (subsubgroups) below the cases
• 5477 group content nodes, mostly related to a subsubgroup

FWIW: on a Windows server I still had errors like this with an unpatched Drupal core 9.5.4 on March 7th:

rename(sites/default/files/php/twig/.nnWfgPodASaFePa8_7gFfmFafwU,sites/default/files/php/twig/640195f90bf17_menu.html.twig_xdiNudfk9QDvOGCiabMF4fGea/LMmSAN4RIDKvzVj8xqiNkyiUGaUVCMjw0NifP6zjx-4.php): Access is denied (code: 5) in Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage->save() (line 88 of core\lib\Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage.php)

Corrected 2 typos: "entities whose left value".

Or rather I was wondering what 'pipelyft' was doing there. Logging a lock hit would not be a bad idea. Anyway, I have removed it now. New patch attached. I will update the contents of #83 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed with the latest code after the patch.

I was already wondering what the logging was meant to do. I will remove it and upload a new patch.

The comments in the patch might need an update, depending on the fix for Leaf lifetime exceeds 60s in large migrations 📌 Leaf lifetime exceeds 60s in large migrations Fixed .

The patch in #90 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed Is running in production for a week now. No issues detected.

See the remark by @lind101 in https://www.drupal.org/project/subgroup/issues/3281672#comment-14876123 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed as well:

 protected function doAddLeaf(EntityInterface $parent, EntityInterface $child) {
    // This is a better implementation of the 60 second stuff
    /* @see \Drupal\subgroup\Entity\GroupSubgroupHandler::doAddLeaf() */
    if ($this->isLeaf($child)) {
      throw new InvalidLeafException('The provided entity is already a leaf, cannot add again.');
    }

Looks like he is saying that adding existing groups as a subgroup should just be possible. But you might be right that it creates all kinds of access issues.

Remaining Coding standards issues

Coding standards for using "native" PHP classes like stdClass
https://www.drupal.org/project/drupal/issues/1614186#comment-6475044 →
Classes and interfaces without a backslash \ inside their fully-qualified
name (for example, the built-in PHP Exception class) must be fully
qualified when used in a namespaced file: for example \Exception.

Spelling, coding standards

Simplify the queries. There is no need to check for both the left and the right value > some right value, because the left value is always smaller than the right value.

Todo: update the issue summary.

getLiveTreeValues() gave me the idea how to drupalize the drirect queries. Another patch will follow.

#82 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed Is in production now, with some additional logging (not included in the patch).

Essential part of SubgroupHandlerBase.php now:

   /**
   * Build update queries for left/right values when leaf is added or removed.
   *
   * @param \Drupal\Core\Entity\EntityInterface $entity
   *   ADD: parent of the added leaf, REMOVE: cache of the removed leaf.
   * @param string $action
   *   The action being taken, leaf added or leaf removed.
   *
   * @return array
   *   Array with SQL queries as string.
   *
   * @throws \Drupal\Core\Entity\Sql\SqlContentEntityStorageException
   */
  public function buildValuesUpdateQueries(EntityInterface $entity, string $action) {
    $updates = [];
    $ids_to_update = [];

    switch ($action) {
      case self::LEAF_ADD:
        // Load the values directly from the database to avoid any caching
        // issues when in a race condition.
        $parentValues = $this->getLiveTreeValues($entity);
        $parent_id = $entity->id();
        $parent_right = $parentValues->right;
        $parent_tree = $parentValues->tree;

        // First we are going to update the left values of the affected
        // entities, the aim of this is to make sure we have enough space
        // to grow the current parent.
        //
        // With this query we only want to update siblings (and their
        // subgroups) to give us room to add a new leaf (2 units) to the
        // current parent (i.e. Update the parents right value by 2).
        //
        // To do this only increase the left value of entities whos left value
        // is currently greater than the right value of the parent.
        //
        // If the leaf is a direct path up (i.e.: its left bound was lower than
        // the parent's right value), then we only need to update the right
        // bound. Otherwise, we need to increment the left bound by 2 as well.
        $tablenames = $this->getTableNames(SUBGROUP_LEFT_FIELD);
        foreach ($tablenames as $tablename) {
          $update = "
          UPDATE {" . $tablename . "} l
          JOIN {group__subgroup_right} r ON r.entity_id = l.entity_id
          JOIN {group__subgroup_tree} t ON t.entity_id = l.entity_id
          SET subgroup_left_value = (subgroup_left_value + 2)
          WHERE l.subgroup_left_value > {$parent_right}
          AND r.subgroup_right_value >= {$parent_right}
          AND t.subgroup_tree_value = {$parent_tree}
          AND l.entity_id != {$parent_id}";
          \Drupal::logger('writeLeafData')->notice("updateLeftQuery <pre>{$update}</pre>");
          $updates[] = $update;
        }

        // Then we need to update the right values. The aim of this is to make
        // sure that sibling groups (and their children) are still the same
        // size as when we started the update, and that we grow any parents of
        // the current parent to accommodate the new leaf new are about to add.
        //
        // This is easy, we just need to update all right values by 2 units.
        $tablenames = $this->getTableNames(SUBGROUP_RIGHT_FIELD);
        foreach ($tablenames as $tablename) {
          $update = "
          UPDATE {" . $tablename . "} r
          JOIN {group__subgroup_tree} t ON t.entity_id = r.entity_id
          SET subgroup_right_value = (subgroup_right_value + 2)
          WHERE r.subgroup_right_value >= {$parent_right}
          AND t.subgroup_tree_value = {$parent_tree}
          AND r.entity_id != {$parent_id}";
          \Drupal::logger('writeLeafData')->notice("updateRightQuery <pre>{$update}</pre>");
          $updates[] = $update;
        }

        break;

      case self::LEAF_REMOVE:
        // We have to use the values stored on the Entity object rather than
        // going to the database when deleting groups because by this point in
        // the delete transaction the field values have already been removed
        // from the database.
        // The easiest way to do this is to use the leaf wrapper.
        $leaf = $this->wrapLeaf($entity);

        $leafValues = new stdClass();
        $leafValues->left = $leaf->getLeft();
        $leafValues->right = $leaf->getRight();
        $leafValues->depth = $leaf->getDepth();
        $leafValues->tree = $leaf->getTree();

        $leaf_right = $leafValues->right;
        $leaf_tree = $leafValues->tree;

        // First we are going to update the left values of the affected
        // entities, the aim of this is to make sure we shrink the tree to
        // prepare for a removal of a leaf from the parent.
        //
        // With this query we only want to update siblings (and their
        // subgroups) to shrink back 2 values each in anticipation of
        // removing a leaf (2 units) from the current parent (i.e. decrease
        // the parents right value by 2).
        //
        // To do this only decrease the left value of entities whos left value
        // is currently greater than the right value of the parent.
        //
        // If the leaf is a direct path up (i.e.: its left bound was lower than
        // the parent's right value), then we only need to update the right
        // bound. Otherwise, we need to decrease the left bound by 2 as well.
        $tablenames = $this->getTableNames(SUBGROUP_LEFT_FIELD);
        foreach ($tablenames as $tablename) {
          $update = "
          UPDATE {" . $tablename . "} l
          JOIN {group__subgroup_right} r ON r.entity_id = l.entity_id
          JOIN {group__subgroup_tree} t ON t.entity_id = l.entity_id
          SET subgroup_left_value = (subgroup_left_value - 2)
          WHERE l.subgroup_left_value > {$leaf_right}
          AND r.subgroup_right_value > {$leaf_right}
          AND t.subgroup_tree_value = {$leaf_tree}";
          \Drupal::logger('writeLeafData')->notice("updateLeftQuery <pre>{$update}</pre>");
          $updates[] = $update;
        }

        // Then we need to update the right values. The aim of this is to make
        // sure that sibling groups (and their children) are still the same
        // size as when we started the update, and that we shrink any parents
        // of the current parent to accommodate the removal of leaf.
        //
        // This is easy, we just need to decrease all right values by 2 units.
        $tablenames = $this->getTableNames(SUBGROUP_RIGHT_FIELD);
        foreach ($tablenames as $tablename) {
          $update = "
          UPDATE {" . $tablename . "} r
          JOIN {group__subgroup_tree} t ON t.entity_id = r.entity_id
          SET subgroup_right_value = (subgroup_right_value - 2)
          WHERE r.subgroup_right_value > {$leaf_right}
          AND t.subgroup_tree_value = {$leaf_tree}";
          \Drupal::logger('writeLeafData')->notice("updateRightQuery <pre>{$update}</pre>");
          $updates[] = $update;
        }

        break;

      default:
        throw new InvalidArgumentException('Invalid query amend action supplied');
    }

    return $updates;
  }

  /**
   * {@inheritdoc}
   */
  public function addLeaf(EntityInterface $parent, EntityInterface $child) {
    $this->verify($parent);
    $this->verify($child);

    if (!$this->isLeaf($parent)) {
      throw new InvalidParentException('The entity to use as the parent is not a leaf.');
    }
    if ($child->isNew()) {
      throw new InvalidLeafException('Cannot use an unsaved entity as a leaf.');
    }
    if ($this->isLeaf($child)) {
      throw new InvalidLeafException('The entity to add as the leaf is already a leaf.');
    }

    $this->doAddLeaf($parent, $child);
  }

  /**
   * Actually adds a leaf to a tree.
   *
   * This is called after a few sanity checks and can be easily overwritten by
   * the extending classes.
   *
   * @param \Drupal\Core\Entity\EntityInterface $parent
   *   The entity to use as the parent.
   * @param \Drupal\Core\Entity\EntityInterface $child
   *   The entity to use as the new leaf.
   */
  protected function doAddLeaf(EntityInterface $parent, EntityInterface $child) {
    $parent_leaf = $this->wrapLeaf($parent);
    $parent_tree = $parent_leaf->getTree();
    $parent_left = $parent_leaf->getLeft();
    $parent_right = $parent_leaf->getRight();
    $parent_id = $parent->id();
    $parent_revision_id = $parent->getRevisionId();
    $connection = \Drupal\Core\Database\Database::getConnection();
    $dbquery = "SELECT subgroup_left_value FROM {group__subgroup_left} WHERE entity_id = {$parent_id} AND revision_id = {$parent_revision_id}";
    $query = \Drupal::database()->query($dbquery);
    $results = $query->fetchAll();
    foreach ($results as $result) {
      $parent_left = $result->subgroup_left_value;
    }
    $dbquery = "SELECT subgroup_right_value FROM {group__subgroup_right} WHERE entity_id = {$parent_id} AND revision_id = {$parent_revision_id}";
    $query = \Drupal::database()->query($dbquery);
    $results = $query->fetchAll();
    foreach ($results as $result) {
      $parent_right = $result->subgroup_right_value;
    }
    \Drupal::logger('writeLeafData')->notice("parent = ".$parent_leaf->getLeft()." => {$parent_left}, ".$parent_leaf->getRight()." => {$parent_right}");
    // This is a better implementation of the 60 second stuff.
    if ($this->isLeaf($child)) {
      throw new InvalidLeafException('The provided entity is already a leaf, cannot add again.');
    }

    if ($this->groupTypeHandler->getParent($child->getGroupType())->id() !== $parent->bundle()) {
      throw new InvalidParentException('Provided group cannot be added as a leaf to the parent (incompatible group types).');
    }

    // If two subgroups are created/removed at the same time we run the risk of
    // race conditions to update leaf values here which can lead to invalid
    // left and right values.
    //
    // To mitigate this we lock this process down so only one process can amend
    // tree values at any given time.
    $blocked = TRUE;
    $logged = FALSE;
    do {
      // Try to obtain the tree lock, otherwise wait until it is free
      // (max 30 sec).
      if ($this->isTreeLockAvailable() && $this->getTreeLock()) {
        $blocked = FALSE;
      }
      else {
        if (!$logged) {
          \Drupal::logger('pipelyft_subgroup')->debug($this->t('Subgroup lock hit for parent: %id - %label. Child: %child_id - %child_label', [
            '%id' => $parent->id(),
            '%label' => $parent->label(),
            '%child_id' => $child->id(),
            '%child_label' => $child->label(),
          ]));
          $logged = TRUE;
        }
      }
    } while ($blocked);

    $transaction = NULL;
    try {
      // Load the values directly from the database to avoid any caching issues
      // when in a race condition.
      $parentValues = $this->getLiveTreeValues($parent);

      if (is_null($parentValues)) {
        throw new InvalidArgumentException('Supplied parent has no tree values.');
      }

      // Start a transaction to encompass the following update queries in-case
      // we run into an error half-way through and need to rollback.
      $transaction = $this->database->startTransaction('subgroup_tree_update');

      // Find all the entities that we need to update: this is any entries that
      // have the same or higher right value than the parent, are part of the
      // same tree and are not the parent themselves.
      $ids_to_update = $this->storage->getQuery()
        ->condition($this->getRightPropertyName(), $parentValues->right, '>=')
        ->condition($this->getTreePropertyName(), $parentValues->tree)
        // We deal with the passed in parent, rather than a freshly loaded copy
        // so that any code calling this method does not have an out of sync
        // entity for the rest of its runtime.
        ->condition($this->entityType->getKey('id'), $child->id(), '<>')
        ->accessCheck(FALSE)
        ->execute();
      \Drupal::logger('writeLeafData')->notice("add leaf ".$child->id().", ids_to_update ".count($ids_to_update));

      if (!empty($ids_to_update)) {
        $updates = $this->buildValuesUpdateQueries($parent, self::LEAF_ADD);
        foreach ($updates as $update) {
          \Drupal::database()->query($update);
        }
        // All the affected entities need their memory cache entries cleared
        // (so they reflect the new tree values on their next load).
        $this->storage->resetCache($ids_to_update);
      }

      // Now that we have made room in the tree we can expand the parent.
      $this->writeLeafData(
        $parent,
        $parentValues->depth,
        $parentValues->left,
        $parentValues->right + 2,
        $parentValues->tree
      );
      // Clear the memory cache of the parent.
      // (So it reflects the new tree values on their next load by any user).
      $parent_id_array[] = $parent->id();
      $this->storage->resetCache($parent_id_array);

      // And finally add the new leaf (Using the original right value to work
      // out a 2 unit span).
      $this->writeLeafData(
        $child,
        $parentValues->depth + 1,
        $parentValues->right,
        $parentValues->right + 1,
        $parentValues->tree
      );
    }
    catch (\Exception $e) {
      // If something goes wrong during the update,
      // roll everything back and release the tree lock.
      if ($transaction instanceof Transaction) {
        $transaction->rollBack();
      }
      $this->releaseTreeLock();

      throw $e;
    }

    // Once everything has run successfully, commit the transaction by
    // unsetting it and release the tree lock.
    unset($transaction);
    $this->releaseTreeLock();
  }

  /**
   * {@inheritdoc}
   */
  public function removeLeaf(EntityInterface $entity, $save = TRUE) {
    $this->verify($entity);

    if (!$this->isLeaf($entity)) {
      throw new InvalidLeafException('The entity to remove is not a leaf.');
    }
    if ($this->hasDescendants($entity)) {
      throw new InvalidLeafException('Cannot remove a leaf that still has descendants.');
    }

    $this->doRemoveLeaf($entity, $save);
  }

  /**
   * Actually removes a leaf from a tree.
   *
   * This is called after a few sanity checks and can be easily overwritten by
   * the extending classes.
   *
   * @param \Drupal\Core\Entity\EntityInterface $entity
   *   The entity to remove from the tree.
   * @param bool $save
   *   Whether the entity should be saved.
   */
  protected function doRemoveLeaf(EntityInterface $entity, $save) {
    // If two subgroups are created/removed at the same time we run the risk of
    // race conditions to update leaf values here which can lead to invalid
    // left and right values.
    //
    // To mitigate this we lock this process down so only one process can amend
    // tree values at any given time.
    $blocked = TRUE;
    do {
      // Try to obtain the tree lock, otherwise wait until it is free
      // (max 30 sec).
      if ($this->isTreeLockAvailable() && $this->getTreeLock()) {
        $blocked = FALSE;
      }
    } while ($blocked);

    $transaction = NULL;
    try {
      // We have to use the values stored on the Entity object rather than
      // going to the database when deleting groups because by this point in
      // the delete transaction the field values have already been removed
      // from the database.
      // The easiest way to do this is to use the leaf wrapper.
      $leaf = $this->wrapLeaf($entity);

      $leafValues = new stdClass();
      $leafValues->left = $leaf->getLeft();
      $leafValues->right = $leaf->getRight();
      $leafValues->depth = $leaf->getDepth();
      $leafValues->tree = $leaf->getTree();

      // If the left and right values are 2 and 3 respectively it means we might
      // be removing the last child of a tree root. In this case, we unset the
      // tree altogether.
      if ($leafValues->left === 2 && $leafValues->right === 3 && $this->getDescendantCount($root = $this->storage->load($leafValues->tree)) === 1) {
        $this->clearLeafData($root, TRUE);
      }
      // Otherwise, we need to shrink the surrounding tree.
      else {
        $ids_to_update = $this->storage->getQuery()
          ->condition($this->getRightPropertyName(), $leafValues->right, '>')
          ->condition($this->getTreePropertyName(), $leafValues->tree)
          ->accessCheck(FALSE)
          ->execute();
        \Drupal::logger('writeLeafData')->notice("remove leaf ".$entity->id().", ids_to_update ".count($ids_to_update));

        if (!empty($ids_to_update)) {
          $transaction = $this->database->startTransaction('subgroup_tree_update');
          $updates = $this->buildValuesUpdateQueries($entity, self::LEAF_REMOVE);
          foreach ($updates as $update) {
            \Drupal::database()->query($update);
          }
          // All the affected entities need their memory cache entries cleared
          // (so they reflect the new tree values on their next load).
          $this->storage->resetCache($ids_to_update);
        }
      }

      // We can now remove the leaf from the tree.
      $this->clearLeafData($entity, $save);

    }
    catch (Exception $e) {
      // If something goes wrong during the update, roll everything back and
      // release the tree lock.
      if ($transaction instanceof Transaction) {
        $transaction->rollBack();
      }
      $this->releaseTreeLock();

      throw $e;
    }

    // Once everything has run successfully, commit the transaction by
    // unsetting it and release the tree lock.
    unset($transaction);
    $this->releaseTreeLock();
  }

2 more coding standards issues fixed.

More coding standards fixed.

Testing the attached patch now. This patch should solve a lot of coding standards issues.

1.0.x: PHP 7.3 & MySQL 5.7, D9.5 Build Successful
PHP 8.1 & MySQL 5.7, D9.5.2 Build Successful

Tests of #75 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed were successful! I will address the coding standards later.

I have put #75 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed in production now. After a busy day on the site with #67 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed in place. 25 new groups, ids_to_update varying from 0 (a new subgroup) to 1030 (4 new subsubgroups to an older subgroup.

@lind101 Comment in your code:

// Try to obtain the tree lock, otherwise wait until it is free (Max 30 sec)

How are those 30 seconds implemented? I cannot find the code that limits the time available.

The essential parts of SubgroupHandlerBase.php after the latest patch:

  /**
   * Build update queries to update the left and right values of affected entities when a leaf is added or removed
   *
   * @param EntityInterface $entity
   *  if $action == LEAF_ADD: the parent of the leaf being added
   *  if $action == LEAF_REMOVE: the cache of the leaf being removed
   * @param string $action
   *  the action being taken, leaf added or leaf removed
   *
   * @return array of strings with the queries
   * @throws \Drupal\Core\Entity\Sql\SqlContentEntityStorageException
   */
  public function buildValuesUpdateQueries(EntityInterface $entity, string $action) {
    $updates = array();
    $ids_to_update = array();

    switch ($action) {
      case self::LEAF_ADD:
        // Load the values directly from the database to avoid any caching issues when in a race condition
        $parentValues = $this->getLiveTreeValues($entity);
        $parent_id = $entity->id();
        $parent_right = $parentValues->right;
        $parent_tree = $parentValues->tree;

        /**
         * First we are going to update the left values of the affected entities, the aim of this is to
         * make sure we have enough space to grow the current parent.
         *
         * With this query we only want to update siblings (and their subgroups) to give us room to add a
         * new leaf (2 units) to the current parent (I.e. Update the parents right value by 2).
         *
         * To do this only increase the left value of entities whos left value is currently greater than the
         * right value of the parent.
         */

        // If the leaf is a direct path up (i.e.: its left bound was lower than
        // the parent's right value), then we only need to update the right bound.
        // Otherwise, we need to increment the left bound by 2 as well.
        $tablenames = $this->getTableNames(SUBGROUP_LEFT_FIELD);
        foreach ($tablenames as $tablename) {
          $update = "
          UPDATE {".$tablename."} l
          JOIN {group__subgroup_right} r ON r.entity_id = l.entity_id
          JOIN {group__subgroup_tree} t ON t.entity_id = l.entity_id
          SET subgroup_left_value = (subgroup_left_value + 2)
          WHERE l.subgroup_left_value > {$parent_right}
          AND r.subgroup_right_value >= {$parent_right}
          AND t.subgroup_tree_value = {$parent_tree}
          AND l.entity_id != {$parent_id}";
          $updates[] = $update;
        }

        /**
         * Then we need to update the right values. The aim of this is to make sure that sibling groups
         * (and their children) are still the same size as when we started the update, and that we grow
         * any parents of the current parent to accommodate the new leaf new are about to add.
         *
         * This is easy, we just need to update all the right values by 2 units
         */
        $tablenames = $this->getTableNames(SUBGROUP_RIGHT_FIELD);
        foreach ($tablenames as $tablename) {
          $update = "
          UPDATE {".$tablename."} r
          JOIN {group__subgroup_tree} t ON t.entity_id = r.entity_id
          SET subgroup_right_value = (subgroup_right_value + 2)
          WHERE r.subgroup_right_value >= {$parent_right}
          AND t.subgroup_tree_value = {$parent_tree}
          AND r.entity_id != {$parent_id}";
          $updates[] = $update;
        }

        break;

      case self::LEAF_REMOVE:
        // We have to use the values stored on the Entity object rather than going to the database when deleting groups
        // because by this point in the delete transaction the field values have already been removed from the database
        // The easiest way to do this is to use the leaf wrapper.
        $leaf = $this->wrapLeaf($entity);

        $leafValues = new stdClass();
        $leafValues->left = $leaf->getLeft();
        $leafValues->right = $leaf->getRight();
        $leafValues->depth = $leaf->getDepth();
        $leafValues->tree = $leaf->getTree();

        $leaf_right = $leafValues->right;
        $leaf_tree = $leafValues->tree;

        /**
         * First we are going to update the left values of the affected entities, the aim of this is to
         * make sure we shrink the tree to prepare for a removal of a leaf from the parent.
         *
         * With this query we only want to update siblings (and their subgroups) to shrink back 2 values each in
         * anticipation of removing a leaf (2 units) from the current parent (I.e. Decrease the parents right value by 2).
         *
         * To do this only decrease the left value of entities whos left value is currently greater than the
         * right value of the parent.
         */

        // If the leaf is a direct path up (i.e.: its left bound was lower than
        // the parent's right value), then we only need to update the right
        // bound. Otherwise, we need to decrease the left bound by 2 as well.
        $tablenames = $this->getTableNames(SUBGROUP_LEFT_FIELD);
        foreach ($tablenames as $tablename) {
          $update = "
          UPDATE {".$tablename."} l
          JOIN {group__subgroup_right} r ON r.entity_id = l.entity_id
          JOIN {group__subgroup_tree} t ON t.entity_id = l.entity_id
          SET subgroup_left_value = (subgroup_left_value - 2)
          WHERE l.subgroup_left_value > {$leaf_right}
          AND r.subgroup_right_value > {$leaf_right}
          AND t.subgroup_tree_value = {$leaf_tree}";
          $updates[] = $update;
        }

        /**
         * Then we need to update the right values. The aim of this is to make sure that sibling groups
         * (and their children) are still the same size as when we started the update, and that we shrink
         * any parents of the current parent to accommodate the removal of leaf.
         *
         * This is easy, we just need to decrease all the right values by 2 units
         */
        $tablenames = $this->getTableNames(SUBGROUP_RIGHT_FIELD);
        foreach ($tablenames as $tablename) {
          $update = "
          UPDATE {".$tablename."} r
          JOIN {group__subgroup_tree} t ON t.entity_id = r.entity_id
          SET subgroup_right_value = (subgroup_right_value - 2)
          WHERE r.subgroup_right_value > {$leaf_right}
          AND t.subgroup_tree_value = {$leaf_tree}";
          $updates[] = $update;
        }

        break;

      default:
        throw new InvalidArgumentException('Invalid query amend action supplied');
    }

    return $updates;
  }

  /**
   * {@inheritdoc}
   */
  public function addLeaf(EntityInterface $parent, EntityInterface $child) {
    $this->verify($parent);
    $this->verify($child);

    if (!$this->isLeaf($parent)) {
      throw new InvalidParentException('The entity to use as the parent is not a leaf.');
    }
    if ($child->isNew()) {
      throw new InvalidLeafException('Cannot use an unsaved entity as a leaf.');
    }
    if ($this->isLeaf($child)) {
      throw new InvalidLeafException('The entity to add as the leaf is already a leaf.');
    }

    $this->doAddLeaf($parent, $child);
  }

  /**
   * Actually adds a leaf to a tree.
   *
   * This is called after a few sanity checks and can be easily overwritten by
   * the extending classes.
   *
   * @param \Drupal\Core\Entity\EntityInterface $parent
   *   The entity to use as the parent.
   * @param \Drupal\Core\Entity\EntityInterface $child
   *   The entity to use as the new leaf.
   */
  protected function doAddLeaf(EntityInterface $parent, EntityInterface $child) {
    // This is a better implementation of the 60 second stuff
    if ($this->isLeaf($child)) {
      throw new InvalidLeafException('The provided entity is already a leaf, cannot add again.');
    }

    if ($this->groupTypeHandler->getParent($child->getGroupType())->id() !== $parent->bundle()) {
      throw new InvalidParentException('Provided group cannot be added as a leaf to the parent (incompatible group types).');
    }

    /**
     * If two subgroups are created/removed at the same time we run the risk of race conditions to update leaf values here
     * which can lead to invalid left and right values.
     *
     * To mitigate this we lock this process down so only one process can amend tree values at any given time
     */
    $blocked = TRUE;
    $logged = FALSE;
    do {
      // Try to obtain the tree lock, otherwise wait until it is free (Max 30 sec)
      if ($this->isTreeLockAvailable() && $this->getTreeLock()) {
        $blocked = FALSE;
      } else {
        if (!$logged) {
          \Drupal::logger('pipelyft_subgroup')->debug($this->t('Subgroup lock hit for parent: %id - %label. Child: %child_id - %child_label', [
            '%id' => $parent->id(),
            '%label' => $parent->label(),
            '%child_id' => $child->id(),
            '%child_label' => $child->label(),
          ]));
          $logged = TRUE;
        }
      }
    } while ($blocked);

    $transaction = NULL;
    try {
      // Load the values directly from the database to avoid any caching issues when in a race condition
      $parentValues = $this->getLiveTreeValues($parent);

      if (is_null($parentValues)) {
        throw new InvalidArgumentException('Supplied parent has no tree values.');
      }

      // Start a transaction to encompass the following update queries in-case we run into an error half-way
      // through and need to rollback
      $transaction = $this->database->startTransaction('subgroup_tree_update');

      /**
       * Find all the entities that we need to update: this is any entries that have the same or higher right value
       * than the parent, are part of the same tree and are not the parent themselves
       */
      $ids_to_update = $this->storage->getQuery()
        ->condition($this->getRightPropertyName(), $parentValues->right, '>=')
        ->condition($this->getTreePropertyName(), $parentValues->tree)
        // We deal with the passed in parent, rather than a freshly loaded copy so
        // that any code calling this method does not have an out of sync entity
        // for the rest of its runtime.
        ->condition($this->entityType->getKey('id'), $child->id(), '<>')
        ->accessCheck(FALSE)
        ->execute();

      if (!empty($ids_to_update)) {
        $updates = $this->buildValuesUpdateQueries($parent, self::LEAF_ADD);
        foreach ($updates as $update) {
          if (is_string($update)) \Drupal::database()->query($update);
          else $update->execute();
        }
        // All the affected entities need their memory cache entries cleared
        // (So they reflect the new tree values on their next load)
        $this->storage->resetCache($ids_to_update);
      }

      // Now that we have made room in the tree we can expand the parent
      $this->writeLeafData(
        $parent,
        $parentValues->depth,
        $parentValues->left,
        $parentValues->right + 2,
        $parentValues->tree
      );
      // Clear the memory cache of the parent
      // (So it reflects the new tree values on their next load by any user)
      $this->storage->resetCache(array($parent->id()));

      // And finally add the new leaf (Using the original right value to work out a 2 unit span)
      $this->writeLeafData(
        $child,
        $parentValues->depth + 1,
        $parentValues->right,
        $parentValues->right + 1,
        $parentValues->tree
      );
    } catch (Exception $e) {
      // If something goes wrong during the update, roll everything back and release the tree lock
      if ($transaction instanceof Transaction) {
        $transaction->rollBack();
      }
      $this->releaseTreeLock();

      throw $e;
    }

    // Once everything has run successfully, commit the transaction by unsetting it and release the tree lock
    unset($transaction);
    $this->releaseTreeLock();
  }

  /**
   * {@inheritdoc}
   */
  public function removeLeaf(EntityInterface $entity, $save = TRUE) {
    $this->verify($entity);

    if (!$this->isLeaf($entity)) {
      throw new InvalidLeafException('The entity to remove is not a leaf.');
    }
    if ($this->hasDescendants($entity)) {
      throw new InvalidLeafException('Cannot remove a leaf that still has descendants.');
    }

    $this->doRemoveLeaf($entity, $save);
  }

  /**
   * Actually removes a leaf from a tree.
   *
   * This is called after a few sanity checks and can be easily overwritten by
   * the extending classes.
   *
   * @param \Drupal\Core\Entity\EntityInterface $entity
   *   The entity to remove from the tree.
   * @param bool $save
   *   Whether the entity should be saved.
   */
  protected function doRemoveLeaf(EntityInterface $entity, $save) {
    /**
     * If two subgroups are created/removed at the same time we run the risk of race conditions to update leaf values here
     * which can lead to invalid left and right values.
     *
     * To mitigate this we lock this process down so only one process can amend tree values at any given time
     */
    $blocked = TRUE;
    do {
      // Try to obtain the tree lock, otherwise wait until it is free (Max 30 sec)
      if ($this->isTreeLockAvailable() && $this->getTreeLock()) {
        $blocked = FALSE;
      }
    } while ($blocked);

    $transaction = NULL;
    try {
      // We have to use the values stored on the Entity object rather than going to the database when deleting groups
      // because by this point in the delete transaction the field values have already been removed from the database
      // The easiest way to do this is to use the leaf wrapper.
      $leaf = $this->wrapLeaf($entity);

      $leafValues = new stdClass();
      $leafValues->left = $leaf->getLeft();
      $leafValues->right = $leaf->getRight();
      $leafValues->depth = $leaf->getDepth();
      $leafValues->tree = $leaf->getTree();

      // If the left and right values are 2 and 3 respectively it means we might
      // be removing the last child of a tree root. In this case, we unset the
      // tree altogether.
      if ($leafValues->left === 2 && $leafValues->right === 3 && $this->getDescendantCount($root = $this->storage->load($leafValues->tree)) === 1) {
        $this->clearLeafData($root, TRUE);
      }
      // Otherwise, we need to shrink the surrounding tree
      else {
        $ids_to_update = $this->storage->getQuery()
          ->condition($this->getRightPropertyName(), $leafValues->right, '>')
          ->condition($this->getTreePropertyName(), $leafValues->tree)
          ->accessCheck(FALSE)
          ->execute();

        if (!empty($ids_to_update)) {
          $transaction = $this->database->startTransaction('subgroup_tree_update');
          $updates = $this->buildValuesUpdateQueries($entity, self::LEAF_REMOVE);
          foreach ($updates as $update) {
            if (is_string($update)) \Drupal::database()->query($update);
            else $update->execute();
          }
          // All the affected entities need their memory cache entries cleared
          // (So they reflect the new tree values on their next load)
          $this->storage->resetCache($ids_to_update);
        }
      }

      // We can now remove the leaf from the tree
      $this->clearLeafData($entity, $save);

    } catch (Exception $e) {
      // If something goes wrong during the update, roll everything back and release the tree lock
      if ($transaction instanceof Transaction) {
        $transaction->rollBack();
      }
      $this->releaseTreeLock();

      throw $e;
    }

    // Once everything has run successfully, commit the transaction by unsetting it and release the tree lock
    unset($transaction);
    $this->releaseTreeLock();
  }

I am testing the attached patch now. The direct queries are not generated by \Drupal\Core\Database\Query\Update, but written als 'raw' queries. Examples of the queries, while adding a leaf:

UPDATE {group__subgroup_left} l
JOIN {group__subgroup_right} r ON r.entity_id = l.entity_id
JOIN {group__subgroup_tree} t ON t.entity_id = l.entity_id
SET subgroup_left_value = (subgroup_left_value + 2)
WHERE l.subgroup_left_value > 12125
AND r.subgroup_right_value >= 12125
AND t.subgroup_tree_value = 2
AND l.entity_id != 7237
		  
UPDATE {group_revision__subgroup_left} l
JOIN {group__subgroup_right} r ON r.entity_id = l.entity_id
JOIN {group__subgroup_tree} t ON t.entity_id = l.entity_id
SET subgroup_left_value = (subgroup_left_value + 2)
WHERE l.subgroup_left_value > 12125
AND r.subgroup_right_value >= 12125
AND t.subgroup_tree_value = 2
AND l.entity_id != 7237

UPDATE {group__subgroup_right} r
JOIN {group__subgroup_tree} t ON t.entity_id = r.entity_id
SET subgroup_right_value = (subgroup_right_value + 2)
WHERE r.subgroup_right_value >= 12125
AND t.subgroup_tree_value = 2
AND r.entity_id != 7237

UPDATE {group_revision__subgroup_right} r
JOIN {group__subgroup_tree} t ON t.entity_id = r.entity_id
SET subgroup_right_value = (subgroup_right_value + 2)
WHERE r.subgroup_right_value >= 12125
AND t.subgroup_tree_value = 2
AND r.entity_id != 7237

I really would not know how to generate these queries using \Drupal\Core\Database\Query\Update, but they are more efficient than using WHERE entity_id IN $ids_to_update, with $ids_to_update possibly a large array of ids.

querying for the ids_to_update and afterwards building a query with a condition if the entity_id is IN the ids_to_update array seems a bit redundant. That can probably be done in one query.

Yea my thinking behind doing this first was that it would prevent any potential issues with updating the wrong values if the left/right values had changed in the time that we ran the two queries. I.e in that current setup both queries know exactly what they need to update, regardless of the left/right values. However your probably right the chances of that happening are slim to none I would have thought.

My main objection against using the $ids_to_update array in the query is that there might be scalability issues. There is a chance that I will be migrating a site with over 80,000 groups from plain old PHP to Drupal with the subgroup module. So the query might contain a check for entity_id in an array of 80,000 values. A query like

UPDATE {group__subgroup_left} SET "subgroup_left_value"=IF(subgroup_left_value > :right_value, subgroup_left_value + 2, subgroup_left_value) WHERE "entity_id" IN $ids_to_update;

does not feel good when actually you need something like

UPDATE {group__subgroup_left} SET "subgroup_left_value" = subgroup_left_value + 2 WHERE "subgroup_tree_value" = :tree_value AND "subgroup_left_value" > :right_value;

Of course, this query needs a JOIN for getting the subgroup_tree_value but you will get the idea. And we still need the $ids_to_update for the resetCache.

I am refactoring the complete patch now to make that possible.

Might be an edge case, but I am not really sure thet the cache of the parent is updated after an doAddLeaf. So after

      // Now that we have made room in the tree we can expand the parent
      $this->writeLeafData(
        $parent,
        $parentValues->depth,
        $parentValues->left,
        $parentValues->right + 2,
        $parentValues->tree
      );

I added

      // Clear the memory cache of the parent
      // (So it reflects the new tree values on their next load by any user)
      $this->storage->resetCache($parent->id());

Better safe than sorry.

doRemoveLeaf normally works OK, but there are 2 transactions: 1 for deleting/adding the entity (core) and 1 for updating the leaf values (subgroup). A disruption between the 2 or while executing the 2nd might mean you haven’t updated the leaf values of the higher groups (doRemoveLeaf) or have a group without left/right values (doAddLeaf). The latter happened to me once: #35 🐛 doAddLeaf & doRemoveLeaf: timestamp bug and scalability Fixed .