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:
- It's reversed. You want to add those lines, not remove them. 😅
- 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.
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.
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. 😅
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.
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
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:
- Your comments at 📌 Drupal 11 compatibility Active were incorrect, confusing, and unhelpful.
- 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
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 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.
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:
- Is something like the patch here a reasonable upstream solution to the described downstream bug?
- Is there now a better way?
- Would you be willing to commit something like this to Group Core if it had tests?
- 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.
I'd like to tag stable 2.0.0 and 1.0.0 releases. But this bug is giving me pause. I'm going to try again to add automated test coverage for the autocomplete widget that either reveals the bug or proves it's no longer there (presumably, fixed upstream in Group somehow?).
The "migration" is really only about form modes and view modes. Since no one has written anything automatic in the last 5 years, I think it's safe to say no one ever will. 😅
I still doubt this is going to happen, but if it did, it'd be based on the 2.0.x branch. I still suspect this should be "postponed" at least until #3152724: Improve architecture per kristiaanvandeneynde → is done.
Thanks for the report and the fix! Confirmed the bug. Tested with both claro and seven. The new CSS doesn't break seven, and fixes claro. Rebased the MR to get working GitLab pipelines again. Pushed a few commits to make the stylelint job happy. Once the pipeline is green, I'll be merging this.
Thanks for the bug report, and the attempts at fixing this.
However, I think this is problematic in all sorts of ways. First of all, yeah, fatal errors with the latest patch, trying to use a render array as the value in #title. That's relatively easy to resolve.
More significant is that I think the approach here is entirely wrong. 😅 If the field is marked as required, the thing you're adding/editing must be added to 1 or more groups. But once it's been added, the specific form element from the widget (select or autocomplete) is not required.
I'm not sure if there's a clean way to solve this. But basically, the widget element is only required if the entity hasn't yet been added to any groups. Once it's added (even in progress, via AJAX), the widget element is optional.
Thanks for the bug report!
Hrm, git blame on those lines points to the original patch from @jidrone (
#2813405-91: Add a field to view and edit content's groups →
). There's no comment in the code about it. I really have no idea what the intention was, other than to specifically forbid the use case you're describing. 😅
If this patch was converted into a merge request (MR), and some tests about adding groups to other groups were added, I could commit the change to remove those lines.
Thanks for the bug report. However, I'm unclear on how to trigger this bug.
A simple unset of the tested var key while visiting a node using the group field will cause the ternary else statement to execute showing a WSOD error.
Huh? 😅 A "simple" [sic] unset (?) of a key... that doesn't sound like something end users can do. Are there ways to trigger this only via the UI? I don't know how you'd unset keys in the regular operation of a site.
I'm entirely open to the possibility there's a real bug here. I'd just need to know how to trigger it (and ideally have a test that demonstrates it) before we can fix it.
Thanks,
-Derek
Agreed this is confusing, and worth fixing.
One aspect that I found on a project where I'm using this is that if the group relationship entity has no fields, you can probably remove the update own group_membership content (for users) or update any group_node:private_page relationship (for a node) group permission from most roles. If there are no fields to edit, there's no need for this permission. If that permission isn't granted, the Edit button disappears from the entitygroupfield widgets.
The 1.0.x MR is fine. To be safe, trying again with 2.0.x. I'll commit this to both branches once pipelines are all green.
I want to get this in before next round of releases. First, need to make it an MR and ensure pipelines are still cool.
This has been bothering me for a very long time. 😂 I want to get this in before the next round of releases. I don't see any possible disruption in simply removing UI noise.
Merged. Thanks, everyone!
My plan is to do some bug triage over the next few days, fix up a few more things, and tag another round of releases. Stay tuned.
@quietone: for most of that policy issue, the proposals complied with the Conventional commits spec. Late in the game, @bbrala proposed a non-compliant version. I should have made a bigger deal at the time, but I was so excited about progress that I caved in for compromise. But since we’re not implementing the original decision, and changing this “for technical reasons”, I believe it’s worth fixing this mistake. @bbrala agrees in #48. The only “benefit” of the Drupalism is supposedly better scanning by the human eye. But it definitely isnot compliant with the spec, and breaks (at least some) tools that support the spec.
If we need yet another policy issue, so be it. However, We’ve got regular participation from many core committers in here. I assert the intention of the previous policy decision was “adopt Conventional commits and move attributions to footer / trailer lines”. This issue is trying to implement that decision.
Either way, I’d love to keep this effort moving forward ASAP. So if we need to postpone this on another policy issue, can we do that extremely quickly?
Thanks,
-Derek
Sweet! Pipeline is now all green on all these combinations:
D11.2, G 3.3
D11.2, G 2.3
D10.5, G 3.3 (default config)
D10.5, G 3.2
D10.5, G 2.3
D9.5, G 3.2
D9.5, G 2.2
Huzzah! I think this is now ready to merge. Wouldn't mind if anyone else wants to do some mild testing or reviews.
Also, the summary here isn't really accurate. We do still need variationcache, but only conditionally on the older versions we still support.
Thanks,
-Derek
Thanks, but let's fix this at ✨ Widget select list sort alphabetically (not by group ID) Needs work and I'll back/forward port as appropriate.
Rebased and started fixing up the MR. Let's see what the bot says.
Huzzah, that's working. 🎉 The MR is now not touching any code, only the phpstan config and .gitlab-ci.yml. So I think this is safe and entirely non-disruptive. However, I'll definitely wait for someone else to RTBC before I commit this.
Pushed some commits for that. Let's see what the bot thinks now...
Hrm, looking at phpstan.neon, it's clear that we're trying to silence deprecation warnings about all this GroupMembership and GroupMembershipLoader stuff:
# Can only remove use of membership loader in 4.0.0
...
So I kinda think we're "barking up the wrong tree" here. The phpstan job is also failing (only in 10.5.x) due to:
Ignored error pattern #^Access to constant on deprecated class
Drupal\\group\\GroupMembership\:# was not matched in reported errors.
Ignored error pattern #^Access to constant on deprecated interface
Drupal\\group\\GroupMembershipLoaderInterface\:# was not matched in
reported errors.
Ignored error pattern #^Call to method [a-zA-Z]+\(\) of deprecated class
Drupal\\group\\GroupMembership\:# was not matched in reported errors.
Ignored error pattern #^Call to method [a-zA-Z]+\(\) of deprecated
interface Drupal\\group\\GroupMembershipLoaderInterface\:# was not
matched in reported errors.
This was most recently touched in 📌 Fix CI by updating "jangregor/phpstan-prophecy" Active :
diff --git a/phpstan.neon b/phpstan.neon
index e659a0d..8f661e5 100644
--- a/phpstan.neon
+++ b/phpstan.neon
@@ -11,8 +11,10 @@ parameters:
# Ignore common errors for now.
- "#Drupal calls should be avoided in classes, use dependency injection instead#"
# Can only remove use of membership loader in 4.0.0
- - "#^Fetching class constant class of deprecated class Drupal\\\\group\\\\GroupMembership\\:#"
- - "#^Fetching class constant class of deprecated interface Drupal\\\\group\\\\GroupMembershipLoaderInterface\\:#"
+ - "#^Access to constant on deprecated class Drupal\\\\group\\\\GroupMembership\\:#"
+ - "#^Access to constant on deprecated interface Drupal\\\\group\\\\GroupMembershipLoaderInterface\\:#"
+ - "#^Call to method [a-zA-Z]+\\(\\) of deprecated class Drupal\\\\group\\\\GroupMembership\\:#"
+ - "#^Call to method [a-zA-Z]+\\(\\) of deprecated interface Drupal\\\\group\\\\GroupMembershipLoaderInterface\\:#"
- "#has typehint with deprecated class Drupal\\\\group\\\\GroupMembership\\:#"
- "#has typehint with deprecated interface Drupal\\\\group\\\\GroupMembershipLoaderInterface\\:#"
- "#^Constructor of class Drupal\\\\group\\\\GroupMembershipLoader has an unused parameter#"
So instead of changing the test code at all, I think we need to consider something like https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/phpstan-... and have a separate phpstan.neon include file for D10 vs. D11 so we're accurately ignoring the right warnings depending on the phpstan version we're running. More or less.
Added this to proposed resolution:
- Comply with the Conventional commits spec by moving the issue ID to the start of the description portion of the first line. The commit type must be the first word or we violate the spec and can break tools (eg #61) that would work.
I have seen all those issues. I understand the difficulties. Hence compromise #64.
However, @ is indeed a wide standard. it’s already a step up from our existing “bylines”. So I’m willing to concede #62 and #64. That’s why I didn’t even put them in the summary.
What about #60, #61 and #63? I think that makes the current summary RTBC and we could move forward, without delay.
p.s. To be clear, Group 4 support will only happen in the 2.1.x series, whenever that starts. This needs to focus on Group 3.3.x and 2.3.x support.
Apologies for all the delays. I'm finally back to working on this project again. 😅
I fixed
📌
Fix GitLab-CI configuration on both 1.0.x and 2.0.x
Active
so we've got green pipelines again. In the process, I fixed GroupAutocompleteFormElementTest to work with group *.3.x branches. See ee349a4f.
If we rebase the MR branch in the issue fork from the latest in 2.0.x branch, we should:
- Have working pipelines to start from
- Have an easy place to add D11-specific testing combinations to our GitLab CI matrix
After that, we can assess what else, if anything is needed in here.
I'm done for tonight, but I might work on this more over the weekend. Certainly early next week, if not.
Thanks!
-Derek
At last! 😅 It's finally doing what I want. 🎉
When the "canary" build or test fails, none of the matrix builds start:
https://git.drupalcode.org/project/entitygroupfield/-/pipelines/646232
When it all works, it all works in the right order:
https://git.drupalcode.org/project/entitygroupfield/-/pipelines/646237
Phew!
In the process, I fixed GroupAutocompleteFormElementTest to work with group *.3.x branches. See ee349a4f.
No chance of merging this one. The automated fix is insufficient and unhelpful. I'll be proceeding with 📌 Drupal 11 Compatibility Active instead.
Re: #62: My compromise proposal would be:
task: [#999999] Convert MediaSource plugin discovery to attributes
Co-authored-by: @sorlov <xxx@no-reply...>
Co-authored-by: @quietone <yyy@no-reply...>
Co-authored-by: @smustgrave <zzz@no-reply...>
...
Then we get @ slickness from GitLab, but still have something to fall back on if that doesn't work at some point in the future.
Here's a draft of a more actionable proposed resolution. Has @todo about upgrading from 8.x-1.*.
Perhaps direct links to the most relevant release notes would be helpful?
Maybe mention stuff about juggling flexible_permissions and variationcache at the right moments?
Anything else?
Thanks!
-Derek
Both 3.3.5 and 2.3.2 stable releases support D11. If there are any bugs or further changes needed, we need new issues about them. Closing.
Thanks,
-Derek
To be clear, #61 (if we followed the spec and it worked) would be a trivial and easy way to generate release notes, no fancy tools needed.
Re: footers -- even if GitLab itself supports something useful with @dww (yay!), are we absolutely certain we're going to use GitLab for the rest of Drupal's life? I don't know how anyone could claim that with any certainty. Git, the whole world round, supports email addresses to associate commits with people. It always has, and it always will. That much I feel comfortable asserting. If we're going to be adopting standards for our commit messages (yay!!), and we're going to have useful history moving forward (YAY!!), can we future-proof that history (😂) by using email addresses to help identify people? Then, someday 5-10 years from now, if we decide that GitHub became better, or "GitMonster" (invented) is the new hottest slickness, or whatever, we're not stuck with history that's unhelpful because the new hotness doesn't know anything about GitLab usernames.
I'm okay with the no-reply addresses, if we have to. I still don't fully believe we can't do better than that in many cases, but I don't want to keep delaying this change by climbing up that hill to die on. 😅
But this truly is our last good chance to change this format for a very long time, and I'd love it if we "never" had to change it again. And, I'd love it if our 10 years from now selves aren't upset that we went "all-in" for GitLab specific features. The Git history is forever, and independent of anyone's particular Git tooling at any given moment in time.