🇺🇸United States @dww

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

Not sure why the bot opened so many D11 issues for this project. Let's consolidate on 📌 Automated Drupal 11 compatibility fixes for ckeditor_blockquote_attribution Needs review

Not sure why the bot opened so many D11 issues for this project. Let's consolidate on 📌 Automated Drupal 11 compatibility fixes for ckeditor_blockquote_attribution Needs review

Not sure why the bot opened so many D11 issues for this project. Let's consolidate on 📌 Automated Drupal 11 compatibility fixes for ckeditor_blockquote_attribution Needs review

Not sure why the bot opened so many D11 issues for this project. Let's consolidate on 📌 Automated Drupal 11 compatibility fixes for ckeditor_blockquote_attribution Needs review

D10 support was added to the 8.x-2.x branch. 8.x-1.x is EOL.

D10 support was added to the 8.x-2.x branch. 8.x-1.x is EOL.

Upon further testing, #24 is fairly broken, at least on D10.5. Here's a slimmed down version that does a better job of the UX I'm aiming for. It'll still remember an active tray (if set in localStorage), and be closed if there's nothing active. It "just" removes the case of trying to open the "first" tab if there's 0 toolbar state and it's the first time a user is interacting with the toolbar.

On the site where I care about this, the logout link is inside a toolbar tab. I don't want that tab to auto-open whenever someone logs back in. So I'm doing some custom JS treachery to clear out toolbar stuff from localStorage and sessionStorage whenever folks click a logout link.

Yeah, I'm not sure the templates are doing anything wrong. I was doing something kinda weird. 😅 It was unexpected behavior, but I figured it out and worked around it.

But thanks much for opening this issue! Curious to see how it unfolds.

That was a little more fun that I bargained for. ;) The GitLab CI pipelines from the DA do all sorts of funny business with the composer.json file. I needed to add some brains to both the test and the .gitlab-ci.yml file to save a copy of the original composer.json as composer.json.orig and for the test to parse that copy, if it exists.

Once that was done, the tests are passing fine on 2.0.x branch, and failed on 8.x-1.x where the two requirements were actually still different. 😅 Pushed another commit to fix that, and we're back to all green.

dww → created an issue.

Rebase did the trick (and a manual re-run on a random fail FunctionalJavascript job), so we're back to an all-green pipeline.

I haven't reviewed this in detail, yet, so I'm not (yet) going to restore RTBC.

Rebased the issue fork against latest 11.x to see if that resolves some of the CI weirdness...

Seems to be CI weirdness where the phpstan (and phpcs) job can't download everything they need and time out...

PHPCS

Uploading artifacts for successful job
00:33
Uploading artifacts...
vendor/: found 12419 matching artifact files and directories 
composer.lock: found 1 matching artifact files and directories 
composer/Metapackage/: found 8 matching artifact files and directories 
test-artifacts/phpcs: found 2 matching artifact files and directories 
ERROR: Uploading artifacts as "archive" to coordinator... 413 Request Entity Too Large  correlation_id=3874120170 id=6932768 responseStatus=413 Request Entity Too Large status=413 token=64_YgNZ3a
FATAL: too large      

PHPStan

The following exception is caused by a process timeout
Check https://getcomposer.org/doc/06-config.md#process-timeout for details
In Process.php line 1205:
                                                                               
  The process "'git' 'clone' '--mirror' '--' 'https://github.com/phpstan/phps  
  tan.git' '/root/.composer/cache/vcs/https---github.com-phpstan-phpstan.git/  
  '" exceeded the timeout of 300 seconds.                                      

PHPUnit on PHP 8.3

The following exception is caused by a process timeout
Check https://getcomposer.org/doc/06-config.md#process-timeout for details
In Process.php line 1205:
                                                                               
  The process "'git' 'clone' '--mirror' '--' 'https://github.com/phpstan/phps  
  tan.git' '/root/.composer/cache/vcs/https---github.com-phpstan-phpstan.git/  
  '" exceeded the timeout of 300 seconds.                                      

....

Pipeline is now green everywhere. Gonna merge this.

Working on this.

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

Thanks @diaodiallo and @phjou for getting this started! This will now be included in the next 2.0.x release.

Yay, https://git.drupalcode.org/project/group_notify/-/pipelines/665530 is happy everywhere. I'm going to merge this.

Huzzah. So glad I added GroupNotifyTest. 😅 This was indeed broken, and not sending any notifications with Group V2. That's because in Group V3 we rely on group_notify_group_relationship_insert() to notice when something is added to a group. But that entity doesn't exist in Group V2, and we need to also implement group_notify_group_content_insert():

https://git.drupalcode.org/project/group_notify/-/merge_requests/13/diff...

With that change, the test is passing locally. Let's see what the bot thinks...

dww → created an issue.

MR !13 resolves all my concerns in #10:

1. It's an MR 😂
2. Confirmed that the schema definition from V3 will also work with V2. That didn't change in Group Core. 🎉
3. Added helper functions instead of embedding version_compare() everywhere.
4. Adds the right constraints to composer.json

Also expanded .gitlab-ci.yml to add a bunch of manual build/test jobs for various combinations, and added a default build/test for D10.5 + Group 2.3.x.

All that seems to be fine. However, this is concerning:

https://git.drupalcode.org/project/group_notify/-/jobs/7360162

Fails locally, too. 📌 Add basic notification test coverage Active is failing with:

There was 1 failure:
1) Drupal\Tests\group_notify\Functional\GroupNotifyTest::testGroupNotify
Failed asserting that actual size 0 matches expected size 2.
/builds/project/group_notify/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/builds/project/group_notify/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:56
/builds/project/group_notify/tests/src/Functional/GroupNotifyTest.php:213
/builds/project/group_notify/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
FAILURES!

That's where the test posted an article to a group, and expected notifications to go out to 2 group members, but no emails were found. Perhaps the test itself is broken in Group V2, but perhaps something else more sinister is broken...

I'll keep digging for now.

I'm working on this now, stay tuned.

Merged. Huzzah! This gives me a lot more confidence that our pipelines will catch actual bugs now. Still a lot more to cover via #3164923: [META] Add test coverage for Group Notify → but this is certainly better than what LoadTest.php alone tells us.

Thanks for this! Agree this is probably worth doing. However, before this can be committed, this needs some work:

1. This needs to be converted into a merge request
2. Given 🐛 Missing schema for NotifyGroupNode configuration settings Active we might need to do something funky with the config schema so that it works between both V2 and V3. TBD.
3. I don't love embedding the version_compare() call everywhere. I'd rather add a helper method to group_notify.module that returns the right string and then use that helper everywhere.
4. We now have a composer.json file so we can do more accurate version constraints. We need to alter that, instead of messing with group_notify.info.yml

Thanks again! I hope to get this resolved before the next 2.0.0 release (which I aim to be rc1).

Okay, groovy. The 8.x-1.x version of this is now green on all 3 platforms:

https://git.drupalcode.org/project/group_notify/-/pipelines/664652

Meanwhile, I opened another branch to port this to the 2.0.x branch and Group API V3. See https://git.drupalcode.org/project/group_notify/-/merge_requests/12
That revealed I had messed up the V3 schema changes for 🐛 Missing schema for NotifyGroupNode configuration settings Active , so I pushed another commit to fix that.
Once that was done, the 2.0.x branch is now passing with Group 3.3.x on both D10.5 and D9.5:

https://git.drupalcode.org/project/group_notify/-/pipelines/664666

Going to leave this here for now and call it a night, but I'll plan to merge this tomorrow morning unless there are any concerns.

Super glad I did this, fleshed out all sorts of problems! 🎉

Cheers,
-Derek

https://git.drupalcode.org/project/group_notify/-/pipelines/664595 failed as expected (sort of). When we try to submit the form to configure group_notify for a given group type, we get a 500 instead of a 200, since that page throws an Exception about missing config schema.

So I spawned off 🐛 Missing schema for NotifyGroupNode configuration settings Active for resolving that part. That's now merged, so rebased in here.

Now the test is revealing other fun. Since we're actually hitting some group_notify code now, the 8.9 job with PHP 7.4 is failing with syntax errors. Tee hee.

And the 10.5.x job with PHP 8.3 is failing since it turns out Group V1 was never ported to support PHP 8.2, much less PHP 8.3. See 🐛 Deprecated function: Use of "static" in callables is deprecated - triggered when adding user to group Closed: duplicate . So I'm going to downgrade the PHP version for our default D10 job to PHP 8.1 in the 8.x-1.x branch.

Whoops, those commits should have said "fix" not "task". Bummer.

Oh well, at least this is fixed and 📌 Add basic notification test coverage Active can proceed...

dww → created an issue.

I started by stealing some group-related test traits from another module I maintain, entitygroupfield. Added some basic test coverage of creating a few groups + users, adding an article node (outside of any groups), configuring group_notify on the group type, then creating more articles in various groups, and making sure the right users got the right emails (at least the right email subjects).

Definitely not exhaustive coverage of all the configuration options, but it's a start.

Bonus, the act of doing this revealed that we're not defining the schema for our modified gnode content enabler plugin config settings. So the tests already found 1 bug. ;)

dww → created an issue.

Ugh, I was going to try to fix this for the forthcoming 8.x-1.7 release. However:

I don't (yet) use this module in a scenario with comment notifications, so I don't have an easy way to manually test this.
We have basically 0 automated test coverage.
Looking more closely at group_notify_comment_insert() and I'm spotting what appear to be other bugs / limitations. In particular, it seems that we only process the "first" group that a given commented node belongs to. If a node is in multiple groups, it doesn't seem that notifications would go out to all the people in all the groups.

So I'm reluctant to change this at all without real tests. I don't want to break anything by trying to fix this only by looking at the code.

From #maintainers Slack:

@mglaman: FYI drupal-mrn is "broken" by the new commit format which added @. I missed that part of the change https://github.com/mglaman/drupal-mrn/issues/444
@dww: Also note, technically these are GitLab usernames, not d.o usernames. 😢 eg in cases where d.o user has a space in their name, on GitLab it’s smooshed together. Not sure about other possible differences between the two sets of usernames. But beware.

This raises a somewhat serious problem with this GitLab-centric approach. All previous release note generating tools that tried to do something with users mentioned in By would link to their d.o profile.

1. Are we now expecting release notes to link to GitLab profiles?
2. Can tools like mrn assume that https://git.drupalcode.org/$GitLabUsername will always work?
3. Is there a way to do an API request with an array of GitLab usernames and get back results that include the Contact field ?

Eg https://git.drupalcode.org/dww points to https://www.drupal.org/u/dww → but I’m in the simple case where the usernames match exactly. For folks where they don’t, mentions in release notes with be broken by default unless the tools are ported to understand the distinction and either only link to the GitLab profile, or jump through some hoops to get from there back to the right d.o profile.

Weird, I had saved the contribution record. Absolutely meant to credit you. But apparently it reset itself once I marked this fixed? Anyway, just re-saved it.

Yeah, if we need no interface, happy to not need a CR here at all. Agree we should be consistent.

That said, I think the CR isn’t documenting what anyone might care to know. It should be discussing the addition of core/modules/update/src/MailHandlerInterface.php not talking about “deprecation” since yeah, we’re not deprecating anything, we’re refactoring internal code.

I’m not so sure. It’s a function that starts with _ which means it’s explicitly not API. No one should be calling this manually. If they are, they’re “doing it wrong” and if their code breaks when we remove it, it’s on them for calling an internal, non-API function.

Merged to 8.x-1.x, cherry-picked to 2.0.x.

Thanks!
-Derek

Perfect, thanks! That's exactly what I was thinking was needed, and where the check should go.

Normally, I'd ask for test coverage, but since this project basically has no automated tests (see #3164923: [META] Add test coverage for Group Notify → if you want to help with that), I'm not going to hold this up. 😅

I started a merge via GitLab, so it should post here soon.

Thanks again!
-Derek

Can't comment on the 2.0.x branch, but I've been using this module in production with the 8.x-1.x branch and queue_mail for years, and it works just fine. Recently did another round of local manual testing, and it's still using queue_mail, as expected.

I'll test the 2.0.x branch before making another 2.0.x release, but I'm wondering if this isn't a bug, but some confusion on correctly configuring queue_mail?

Can someone provided detailed steps to reproduce the problem?

Otherwise, not sure there's anything to fix here.

Thanks!
-Derek

Thanks for your interest in group_notify!

This sounds more like a bug than a support request to me. I'm out of time for today, but would definitely love to see an MR that implements the proposed changes. If no one gets to it before me, I'll try to get this fixed next week before another round of releases...

Taking another look at this, I don't believe we want this at all.

According to https://api.drupal.org/api/drupal/core%21core.api.php/function/hook_mail... the right way to stop a message from being sent is to do this in your hook_mail_alter() implementation:

      $message['send'] = FALSE;

In principle, there's no reason an email can't have an empty subject. I think it'd be more confusing to silently eat such messages than to send them out and let someone notice that their token configuration is broken, or whatever.

If you've botched your configuration, that's up to you as the developer to test it and sort it out. But I don't think this module should be adding yet more complication and yet another way to not get the emails you're expecting. 😅

Thanks for the report! This sounds more like a bug report than a feature request to me. 😅 I'd be willing to put this in both 8.x-1.x and 2.0.x branches.

In the absence of real automated tests, I did some manual local testing for this. It's working fine in 8.x-1.x branch. It's basically identical code for 2.0.x branch (the file just moved), so I have high confidence it works there, too. 😅 Pipelines are all green, so I know I didn't botch the phpstan baseline stuff.

Thanks for your interest in group_notify.

Are you proposing to fold this functionality into the group_notify module itself? Or is this just here as an FYI? Not sure what to do with this issue as it stands.

Thanks for your interest in group_notify!

I think I understand your request. However, the patch in #2 isn't going to work for a few reasons:

1. It's reversed. You want to add those lines, not remove them. 😅
2. It's doing more than just adding author to the params, you're also changing the message itself, which we don't want here.

However, instead of hard-coding just the author like this, if we're going to pass additional stuff around in the mail params (good idea!), seems better to add an entity key and pass either the full node or the comment (as appropriate). Then folks trying to alter these messages can get whatever info they need from the entity, instead of coming back here to ask for some other useful bit of context added to params. Something similar with passing a user entity via params is documented in the hook_help API docs, so I know this sort of thing is the intended usage of params.

Also, before this could be committed, it needs to be a merge request so that the GitLab CI pipelines can run and verify nothing is broken by the change.

Did some digging. Looks like GitLab usernames have no spaces, after all:

https://git.drupalcode.org/amberhimesmatz

So probably the contribution record is smart enough to pull in the GitLab user, not the d.o user, and it will all Just Work(tm) if folks actually copy/paste the message. 😂

I still think this isn't great for all the reasons I've already said, but at least it's not going to be actively broken for some subset of our users with spaces in their names. I think. 😅

Whoops, totally didn't mean to assign this. 😂

Meanwhile, pedantic nit about #71:

It's a practice started by github/gitlab to compensate the lack of attribution tools in their system

This is not true. As I understand it, these footers (like Git itself) were developed by Linus and the Linux kernel folks. They've been using very detailed footers to track who wrote, reviewed, and signed-off on basically every change to the Linux kernel for decades. As but one recent example:

commit 506aa235f6e0baa00bf792df82a5e9f618b7a5d8
Author: Christoph Hellwig <hch@lst.de>
Date:   Tue Oct 7 11:06:28 2025 +0200

    block: move bio_iov_iter_get_bdev_pages to block/fops.c

    Keep bio_iov_iter_get_bdev_pages local with the callers, as blindly
    looking at the bdev logical block size is often not the best idea
    unless on a block device.

    Signed-off-by: Christoph Hellwig <hch@lst.de>
    Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
    Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
    Reviewed-by: Keith Busch <kbusch@kernel.org>
    Reviewed-by: Qu Wenruo <wqu@suse.com>
    Signed-off-by: Jens Axboe <axboe@kernel.dk>

The very earliest commits in the canonical Linux repo include Signed-off-by: since 2005. Reviewed-by: first appears in 2007, and Co-authored-by: (as a stand-alone footer) in 2013. All of this pre-dates conventional commits, and at least Reviewed-by: predates GitHub (which launched publicly in April 2008). 🤓

This reinforces a point I tried to make earlier. Our Git history will outlive any particular Git hosting provider. I'd really rather we weren't relying on things that only GitLab understands.

Pipeline is green. phpstan helped find another D10 porting problem that the update bot missed.

Merged to 8.x-1.x. I'll triage the issue queue and make sure there's nothing else urgent, then cut a 8.x-1.7 release for this.

Yay, very happy we're all in agreement on actually complying with the conventional commits spec.

Also happy we're currently just using By if we're not going to be more precise about the roles each participant had. And will be happy to revisit this if GitLab can do a better job creating these footers automagically in the future.

I'm still not "happy" we're relying on GitLab @ magic instead of emails. In particular, GitLab seems confused by the @ if the username contains a space:

https://git.drupalcode.org/project/group_notify/-/commit/c8d077eb463bc18...

Not sure if that's just me being stupid, but I believe this is (another) problem with trying to rely on the GitLab slickness. Maybe there are no such users, and I screwed up trying to attribute that commit to 'project update bot'. But it sure looks like we've got folks like "amber himes matz", "gábor hojtsy" and "wim leers" as core (subsystem) maintainers. Haven't tried to credit any of them, but I think @amber himes matz will cause GitLab to try to link to just @amber.

Maybe there's magic under the hood, their GitLab usernames don't match their d.o usernames, the contribution record pulls in their real GitLab usernames, and it all just works. But I'd love to know for sure before we call this "fixed".

Thanks!
-Derek

📌 Add GitLab CI pipelines and get all jobs passing Active is now done and merged. Rebased the issue fork from latest 8.x-1.x. This should be pretty close to ready now.

Merged to both branches. Huzzah! Onward...

dww → created an issue.

Actually, I want to add GitLab CI and fix the builds at 📌 Add GitLab CI pipelines and get all jobs passing Active , first. Then I'll circle back here.

dww → created an issue.

Okay, cool. That still doesn’t give me detailed steps to reproduce the problem. Without those steps, I can’t do anything else here. You must provide those steps if you want this to move forward.

Thanks,
-Derek

Given that #2873212: Add a status to the group (Publish/Unpublish) → has been fixed for a long time, why is this issue even still open? If there are specific follow up requests to build upon the existing published feature, those need to be in separate new issues with clear scope. But this issue is no longer helpful for anyone. 😅

Whee, re-roll of #284 for 10.5.x

Whee, reroll of #170 for 10.5.x 😅

dww → created an issue.

Apologies for the confusion here. Indeed https://www.drupal.org/project/daterange_compact → is now stable and working fine. I'm using that on the site where I originally cared about this functionality. It's not going to be merged into datetime_extras at this point. Folks should use DateRange Compact, instead.

Merged to 8.x-1.x branch.

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

Re: #13:

we can no longer test against those versions now that Drupal CI is gone.

This is totally false. See https://www.drupal.org/project/views_taxonomy_term_name_into_id → as a counter example. That's a contrib I maintain that supports D8 through D11. It's testing on all 4 core major versions in its GitLab CI pipelines, e.g.:

https://git.drupalcode.org/project/views_taxonomy_term_name_into_id/-/pi...

It's absolutely possible to support older versions, and test them.

Whether we want to do that here in IEF is another question. 😅 I'm still torn. But such a decision should be based on true considerations like technical debt vs. ease of use for sites that rely on this module, not bogus claims that it's impossible to have automated tests with older versions of core.

8.x-1.0 is out with D11 support. This project is so small, I don't think I need a co-maintainer at this point.

Thanks anyway,
-Derek

Thanks for reporting this issue and your interest in fixing it.

Something was wrong with your MR, it was adding merge conflicts. I don't think it was based on the latest 8.x-1.x branch.

Meanwhile, instead of indenting the whole function an extra 2 spaces (probably causing the merge conflicts), I believe it's clearer and more readable to do this at the top of the function:

    if ($argument === NULL || $argument === '') {
      // Nothing to validate, bail early.
      return FALSE;
    }

So I rebased your issue fork branch to revert your commit and add that. I believe it's the same desired effect. Pipeline is now green:

https://git.drupalcode.org/project/views_taxonomy_term_name_into_id/-/pi...

However, before committing this, I'd like to see some automated tests to cover this case. My initial attempts at recreating the problem aren't resulting in any fatal errors. For example, I pushed a trivial change to the end of ArgumentValidatorTest.php to fetch the test view's path with no arguments at all. That test view is configured to provide a summary in that case. It's working fine. Views itself seems to not bother trying to validate an empty argument.

https://git.drupalcode.org/project/views_taxonomy_term_name_into_id/-/pi... is also passing, and the test only job passed (which is unexpected if this is an actual bug).

So I need real steps to reproduce this bug, ideally in the form of an automated test that demonstrates the problem. At the bare minimum, you could attach the views.view.?.yml file from the view that's triggering the WSOD in your case.

So far, I think it's all working as designed.

Thanks,
-Derek

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

https://www.drupal.org/project/views_taxonomy_term_name_into_id/releases... → is now out.

Indeed, it's still useful. 😅 Just put out a stable 8.x-1.0 release with D11 compatibility.

Merged to 8.x-1.x branch. I'll tag a stable 8.x-1.0 release next.

Thanks for the offer. However, I'm not interested in your help at the moment:

1. Your comments at 📌 Drupal 11 compatibility Active were incorrect, confusing, and unhelpful.
2. Your changes at 📌 Enable GitLab CI and cleanup build pipelines Active are out of scope for that issue, and needlessly break compatibility with older versions of core.

So I don't think this is going to be a good fit for collaboration. At least not at this time.

Thanks anyway,
-Derek

MR 7 is the way forward. Let's see what the bot thinks. 😅

I don't want to drop support for old versions like this. I also don't want/need a composer.json here. I'm going to start a new branch and a new MR that's the way I want to handle this.

I reverted all the commits in the MR since #3. They're out of scope here (D11 porting) or totally unnecessary / unhelpful (adding a composer.json file).

I pushed new commits to explicitly test the core versions we're supporting. Also, fixed phpcs, phpstan, cspell.

If the latest pipeline is green on all 3 configurations, I'll merge this.

I reverted all the commits in the MR since #3. They're out of scope here (D11 porting) or totally unnecessary / unhelpful (adding a composer.json file).

I pushed new commits to explicitly test the core versions we're supporting. Also, fixed phpcs, phpstan, cspell.

If the latest pipeline is green on all 3 configurations, I'll merge this.

P.s. since this bug was never in a release, I wouldn’t expect to see it listed in the next release notes.

@kristiaanvandeneynde: Apologies if anything caused you annoyance on this. I'm glad you're mostly feeling supported and happy that I/we have your back, even when you're on vacation! Indeed, you're not alone.

I certainly wasn't going to tag new releases for this, especially since it was only broken in -dev, not a tagged release. I definitely wanted you to have a chance to see what I/we had done before any releases went out.

That said, I put the reasons for considering this a critical bug in the summary, and I stand by my assessment that this was critical. Given that it was breaking support for the "legacy" core branches, it was within the bounds of our agreement for me to commit it. Sure, -dev releases can be broken, and so long as they're fixed before the next tagged release, no real harm is done to end users. But since the documented criteria for critical were satisfied on 2 counts, I decided to proceed without your participation. In Drupal core, if "HEAD is broken", it's generally considered a critical bug, and folks will "drop everything" to get it working again. If you came back from vacation and had to tag a security release or something, the "stable" branches should be in a state where you could tag another release at any time. I didn't want this to be missed before another tag, so I moved forward at full speed.

Anyway, I'm very glad you're not more pissed off, don't want to revoke my maintainership, etc. 😅 I really meant no harm or offense. I simply had a need, and since I had the powers to fix it, I did.

Happy to chat more about it if desired, although living on opposite sides of the world makes the timezones fun for synchronous communication. 😂 Mostly, want to reaffirm that we're on the same team, and that I believe we want the same things: a happy and stable Group core. I highly value our mental and emotional well-being, and want to be a supportive collaborator, not a pain in the ass. Thanks!

In service,
-Derek

FYI: https://www.drupal.org/project/entitygroupfield/releases/2.0.0-rc1 → and https://www.drupal.org/project/entitygroupfield/releases/1.0.0-rc1 → are both now out. Testing welcome. I'd like to put out the corresponding stable releases before the end of 2025, ideally before the end of November. 😅 Thanks!

FYI: https://www.drupal.org/project/entitygroupfield/releases/2.0.0-rc1 → is now out. Testing welcome. I'd like to put out the 2.0.0 stable release before the end of 2025, ideally before the end of November. 😅 Thanks!

Merged. Thanks, y'all!

Merged so I can tag stable releases. We can clean this up more at #3556732: [pp-1] Filter out groups where the user does not have permission to add content from the returned autocomplete choices → .

Opened MRs for both branches, and updated our existing test coverage accordingly.

Fixed the 'administer members' case.
Also opened #3556732: [pp-1] Filter out groups where the user does not have permission to add content from the returned autocomplete choices → and added @todo/@see comments.

dww → created an issue.

FWIW, I'm pretty dubious about a hook being "required" by some specific admin theme. What's this hook for? Why does Gin need it but every other admin theme does not? What does Gin care about routes that are considered "content forms"? Doesn't the routing system already handle routes that are marked as wanting to use the admin theme?

I'd marked this "Postponed (maintainer needs more info)", but that's more for @kristiaanvandeneynde to say.

Personally, I'd probably mark it "won't fix" and tell Gin to figure this out on its own, without requiring every contrib module to implement a hook that seems to duplicate the info we already declare to the routing system.

Hi @kristiaanvandeneynde -- I'm finally getting around to fixing the bug in entitygroupfield that prompted this proposed API change to Group core. See 🐛 User without permission can add entity to group through autocomplete widget. Needs work . I'm ashamed to admit I haven't been following Group Core development closely enough to know if this is even the right approach anymore. 😅 Before I invest effort in writing tests for this and getting it ready, I'd love to get your quick feedback on:

1. Is something like the patch here a reasonable upstream solution to the described downstream bug?
2. Is there now a better way?
3. Would you be willing to commit something like this to Group Core if it had tests?
4. What branch(es) should we target?

Would you be willing to answer those questions, then unassign yourself?

Thanks very much!
-Derek

Just realized the "fix" would be broken in the case of group memberships (users). We need to special-case the permission checks if ($entity_plugin_id == 'group_membership')...

But I'm done for tonight, I'll pick this up again tomorrow.

#3163943: Allow GroupQueryAccessHandler to accept any permission as an operation → would indeed make this better. 😅 I'll ping @kristiaanvde about it and try to get some input on that approach.

But probably for the purposes of a short-term fix that doesn't require any upstream changes, what I've got is Good Enough(tm) to actually close the bug. We can open a follow-up and add an @todo about making it more slick, once something like #3163943 is working in Group core.

Phew! That was fun.

Indeed, I still can't reproduce the thing where someone without permission to view groups they don't belong to can do anything bad with the autocomplete widget.

However, I definitely was able to get the test to fail in the case where an "outsider" role has 'view group' permission. In this case, someone can view groups, but isn't supposed to be able to add content to those groups. The autocomplete widget shows them all the groups they have access to view, so they can select whatever groups they want. Then the widget (wrongly) lets them add content to those groups.

I pushed a fix to both branches. I'm not sure it's the best fix. 😅 Ideally, we'd alter the query for what results the autocomplete is returning to filter by groups the current user has permission to add stuff to. But frankly, I haven't been following upstream Group changes enough, all the flexible_permissions stuff, etc. I don't know The Right Way(tm) to tie into that situation anymore. So for right now, simply to stop the bleeding, I added a EntityGroupFieldSelection::validateReferenceableEntities() method (which is plumbing from the upstream EntityReferenceSelection/DefaultSelection world. The autocomplete widget sets the right things to trigger this to further validate the selection that not only does the group exist, but the given user has permission to add relationships to it.

The tests, which were all failing in this can-view-but-can't-add-stuff case (failing because the test user was able to add the content), are now all passing (because the test user is no longer able to add content to groups they can see but don't have the right permission to add group relationship entities).

Opened MRs for both 1.0.x and 2.0.x (so I can see all the possible group / core versions from both pipelines) with my patch from #7. As I understand it, that should fail if this bug exists.

However, upon further review, maybe #12 is pointing to the difference. In that scenario, the user is a member of the "forbidden" group, but their group role isn't supposed to let you add stuff to the group. I'll expand the test coverage in the MRs to try that case, too.