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.
Meanwhile, here's a "tool" that works with the spec, and is broken by our Drupalism version:
git log --pretty=format:%s 11.2.4..11.2.5 | sort
If we followed the spec, this command would group all commits between two releases by what kind of commit they were. If we put the issue number at the front, this is broken.
Re #54: the thing we came up with clearly does not conform to the spec. The thing I’m proposing here is a trivial change that clearly does conform to the spec (and IMHO is just as readable / scannable).
Instead of trying to get reports about possible tools in the wild, and whether or not they support the spec, let’s simply follow the spec. if the footer in the contribution records page is going to say:
The message follows conventional commits format.
Then let’s do that. 😅
I have no idea what @vladimiraus is talking about with "D7 tasks". This module has never supported D7 (since D7 views provided this functionality itself). Totally unhelpful and confusing comments. Please don't close issues where you don't know what you're talking about. 😅
Anyway, yes, I absolutely plan to fix this (and 📌 Enable GitLab CI and cleanup build pipelines Active ) and get another release out that supports D11. I'm finally getting through a giant backlog of other responsibilities, and hope to move this forward next week. 🤞
I pushed a commit to the issue fork from @lostcarpark in the parent issue's MR branch. All the LegacyHook stuff should hopefully now be resolved here. However, there are still other phpstan errors when testing against 10.5.x core, so calling this NW for now.
https://git.drupalcode.org/project/group/-/merge_requests/270 was to make sure we don't break anything on 2.3.x branch, where this also needs to be backported (since ✨ Support object-oriented hooks in version 3.x Active landed there, too).
The pipelines on both branches are green now, except for the warning about phpstan on 10.5. See 📌 Fix PHPStan for previous major Needs work
3.3.x: https://git.drupalcode.org/issue/group-3555468/-/pipelines/645991
2.3.x: https://git.drupalcode.org/project/group/-/pipelines/646082
I know I'm only a co-maintainer in this project to support the legacy branches, and these are the actively supported branches. However, given the severity of the problem, the fact this *is* supporting legacy (core) branches, that the pipelines are happy, and that @kristiaanvde is currently on vacation, I'm going to merge these and push them upstream. Seems better to ask for forgiveness than permission. 😂 The fact #3548434 was committed in the first place tells me that @kristiaanvde sees a need for at least some technical debt to allow "bridge releases" that are compatible across multiple core versions, so option B from the summary isn't really the way forward here.
FYI 🐛 Recent commits to *.3.x branches break BC Active
Also, if we're following semantic versioning, as a new feature, this really should have only gone into a new minor branch (3.4.x and 2.4.x). I know this particular change is irrelevant for the 4.0.x branch, which only supports 11.2.x and up, so I understand the appeal of putting it in the older branches, but a new minor for both of those would have been safer. Oh well. Thankfully #3555468 will prevent trouble before any other tagged releases, so only folks using -dev are likely to be nailed by this.
Tentatively willing to raise my hand for content_moderation. Used it on a few sites and submitted some fixes. About to adopt it for another regular client, so I’ll have at least some paid time to work on fixes and changes that site might need.
Currently the only active maintainer for update.module, and unofficial co-maintainer for a bunch of other parts of core (views, date*, etc).
If anyone else is in a better position, happy to let them take it. But willing to put my name in, since I believe an over-committed and intermittently available human is better than a ? 😂
Thankfully, none of this has made it into a tagged release. But I'm leaving this 'critical' so we resolve it before any other releases are tagged.
Meanwhile, I've got an MR up that I think will work. I'll keep an eye on the pipelines.
Re: #26: yeah, per the end of #22, that could all get messy and weird. That’s why I’m proposing this policy issue should be “include everything in CLDR”, and we can have a follow-up (if needed) to explore being even more inclusive.
Since Group 8.x-1.x does not support D11, the 1.0.x branch here won't, either.
Thanks for the bug report, clear description of the problem and solution, and the patch!
Agreed with remaining tasks needing some test coverage for this bug. Tagging as such, and moving to "Needs work".
Also, it'd be great to convert this into a GitLab merge request so that the automated tests and other tooling will run.
Once we have an MR with a test that fails with the current code in Git, and passes with the proposed fix, this can probably be committed.
Thanks again!
-Derek
Thanks for all the work in here since my last review!
Apologies I haven’t had a chance to re-review and RTBC. Ben super slammed but just finished some intensive (transformational) work over the weekend, and should be able to circle back here in the next few days.
Re #98, sounds good. Perhaps a new CR is most clear. But what about editing the existing CR that introduced the new field UI stuff? On my phone, not easy to find right now.
Re #48: thanks for saying so. Glad you agree. This seems like our last real chance to fix this.
Updating the proposed resolution to actually match the standard. 😅
Good idea. Thanks. I dare say this is “novice” material at this point.
Haven't directly experienced this, but it sounds really similar to what was reported at 🐛 D11/Address 2.0: Administrative area ('province') not showing for Germany (DE), but works for 'state' (e.g. US) Active . I think we might have a real bug that if folks use the field overrides to display a given element such as administrative_area, that if the country format doesn't include that field, we don't show it. We probably should. Whether or not Google's address database includes specific values for a field shouldn't determine if folks can collect/store that field at all.
Copy/paste fail. I was looking at 🐛 Field data lost after form (re)submission because form elements are missing Active
Thanks for this bug report and proposed fixed!
Even though it's the older issue, I just marked 🐛 Warning: Undefined array key "#disabled" in Drupal\address\Element\Country::processCountry() (line 100 of src/Element/Country.php). Active as duplicate, since the MR over there only includes the fix, while this one also includes test coverage.
Haven't actually reviewed yet, so I'm not going to commit, but this seems promising on a very quick skim. The fix itself looks right. Not sure about the tests.
This is the older issue, so normally I'd keep this one open and close the newer. But 🐛 Setting #disabled on Address element is ignored by Country element Active has the identical fix, except the MR over there also has test coverage. Therefore, closing this one as the duplicate. Anyone still interested in this bug should focus efforts over there.
Thanks!
-Derek
Apologies, just now seeing this. Took me a moment, but this looks like you're trying to patch the commerceguys/addressing library, not the Address Drupal module. Therefore, this change needs to be a merge request against https://github.com/commerceguys/addressing if it's going to happen.
Thanks for working on this. However, let's focus efforts in the earlier issue about this same problem: 🐛 Mismatched field definitions after address upgrade Needs review
To be clear, the 8.x-1.9 version did not render a form element for subdivision for Germany. There's no regression. If you do the suggested thing and provide your own subdivision data for Germany, it should all just work. If you try it, and it doesn't work, open a new issue about it with copies of your event subscriber and related code.
Cheers,
-Derek
I happen to have an old site running 8.9.20, and address 8.x-1.9. If I try to create a test node there with an address using Germany as the country, there's no province. This is not a "regression", it's always been like this.
Indeed, see #3167402: State or Province area for German addresses not exposed →
For the record, this was merged as commit 7360754cf197e8917108d81e3aeaf9c01fe3cb94 but without an issue NID in the commit message, there's no link from there to here.
Apparently this caused some grief for folks. See #3269345: Regression: hidden address fields are no longer present in the array →
Definitely not going to revert it at this point, only posting FYI.
Yeah, bummer, that issue landed as commit 7360754cf197e8917108d81e3aeaf9c01fe3cb94 but without an issue NID, etc.
Sorry for only seeing this now. I think the train has long left the station for this "regression". Maybe that issue should have been postponed for the 2.0.x series, since it is a bit of an "API" change. But what's done is done. And technically, since it was on the 8.x-1.x branch, without real semantic versioning, every release is a new minor version.
Apologies for the disruption, but I think we've got to call this "works as designed".
Agreed, this seems totally duplicate. Not sure why this one was submitted.
Thanks,
-Derek
Thanks for working on this!
The DI changes here are duplicate with 📌 Use DI not \Drupal in src/Plugin/views/filter/AdministrativeArea.php Active
I'm not sure how I feel about the other changes.
Sorry, I don't understand the request here. You're talking about an entity reference field. Those can only reference entities. This module provides a field type, which can be added to whatever entities you want. It does not provide its own field type. So I fail to see how address module could possibly know or care about your entity reference fields.
The specific example you're talking about, an address field on a product (perhaps more appropriate as an order_item field?) and having a hard-coded list of available address values you can pick from, makes no sense. There's nothing about address.module that supports something like that. Perhaps a more appropriate solution would be an entity reference field, not an address field, and have that reference point to your store entities. Then you can lookup the store address by following the entity reference field. Or something. But there's no way address itself can support a pre-defined list of available address values to select from.
Would you be willing to provide some more context and information about what you're talking about? Otherwise, I'll have to close this as "works as designed".
Thanks/sorry,
-Derek
Thanks for this. See #3460590-16: AddressFieldBuilder::build is not compatible with diff 2.0.x's CoreFieldBuilder::build → . I manually tested that issue with both diff 8.x-1.9 and 2.0.0-beta4, and it was working fine. So I cherry-pick'ed the commit and pushed to 2.0.x.
I'd call this fixed, but curious if there are other bug fix or non-disruptive tasks that should be backported from 2.x to 2.0.x. I don't have time to review the linked commit listings right now. I can look more next week.
p.s. I should note for the record that I tested this locally with a 10.5.x site, installed diff 8.x-1.9 and address 2.0.x, created a node type with an address field, made some edits / revisions, and was safely able to view the diffs between the revisions without trouble. I then upgraded diff to 2.0.0-beta4, and diff all kept working fine.
Per 🐛 Disparity between 2.x and 2.0.x Active , I cherry-pick'ed this and pushed to 2.0.x. Not sure why that's not automatically showing up here. But it's at:
https://git.drupalcode.org/project/address/-/commit/ba10f2ea735dc6f64e50...
So this will go out in the 2.0.5 release, whenever that happens.
This is looking really good. Thanks! Opened some MR threads for consideration. Not sure any of them qualify as "Needs work", but I wanted to at least bump back for review before this is committed.
Confirmed. Ran into this on a client project where I'm trying to upgrade a site from 8.9.x to modern Drupal. An intermediate step is getting from D9 to D10, and that requires the 8.x-1.x branch. That branch doesn't fully work with D10, even though the .info.yml file says so.
I rebased the MR on the latest 8.x-1.x, and pushed a commit to get the tests passing on both branches (at least locally).
Note that
✨
Add separate 'Save' and 'Save and add fields' to Add Content, Comment, Media and Block Type Forms
Fixed
somewhat annoyingly (and IMHO, out of scope) changed the button label on the content type form from 'Save content type' to just 'Save'. That's why we need the version_compare() in XmlSitemapMultilingualNodeTest.php.
Pipelines all failed since #3290112: Automated Drupal 10 compatibility fixes → isn't done, but at least pipelines are now running. Merged this to 8.x-1.x.
🐛 Active toolbar tray has weak affordance and fails WCAG color criteria Needs work changed direction since I opened this issue, and now handles this problem without needing separate icon files to swap out. This should all Just Work(tm) now, without any special effort here.
The blocker, 📌 Remove $authorized param to UpdateMailTest::testUpdateEmail() Active , is now in 11.x. I rebased MR 12950 to clean that up. This should be ready for review now.
Thanks @longwave! Only reason to backport this would be if the bug fix that prompted it, 🐛 Correct & improve update status email subject Postponed , is likely to be backported, too. However, as a string change, I imagine that one is unlikely to get backported very far. Let's call this fixed for now, and we can always re-open if we decide we do want to backport it. I'm happy to re-roll if needed / desired.
is there a reason not having this in the latest release 2.0.4
Because this is a new feature, and we’re responsible maintainers that actually follow semantic versioning (“semver”). This will be released in 2.1.0. The 2.0.x releases are only for bug fixes and non-disruptive tasks, not new features.
Cheers,
-Derek
Okay, I took patch #37 (what I had previously committed), and turned it into an MR against 2.0.x.
Interdiff in #56 was helpful, so I used that.
I had to fix quite a bit of other stuff to get this to pass locally (on a 10.5 test site).
The pipeline is failing here, but it seems our pipelines are all sorts of messed up right now 😬 😅
https://git.drupalcode.org/project/address/-/pipelines
Probably need to pivot to cleaning that up before I get much further with this.
But the MR's plain diff is now applying happily to a local clone of a client site where I need this fixed. And it seems to work as expected, so that's all positive. 😂
Unassigning myself for now, and moving to NR, although there might be additional work to get this actually committable...
Re: #64: I respectfully disagree. This *needs* to fix the UI of the actual address element, too, not just the country element. If you add a required address field to an entity, and do not provide a default value, the UI is quite bad. It defaults to selecting the first country in the list, which is typically not the right behavior. In all other cases, if you have a required field without a default, the form loads in a state that forces you to make a choice, and if you don't, you get a validation error. Here's a series of screenshots about it:
Current UI
Initial state of proposed UI
Browser validation error if you don't select a country
Working UI once you select a country
Sure, this isn't backwards compatible, and commerce core (and other modules that make use of address fields) might need to be modified to work with this behavior. I'm okay with putting this into a 2.1.x branch. But I'm not okay saying that only the country element can be changed, and that we're going to leave the current default-to-the-first-in-the-list behavior for address elements indefinitely.
For right now, I'm going to work on turning this into an MR, make sure it all applies cleanly to 2.0.x branch, and all the address tests are working fine. If we create a 2.1.x branch for this (and the other RTBC new features), this will merge to that branch instead of 2.0.x. I pinged folks in #commerce Slack about this, and I'm willing to coord with the Commerce Core maintainers to make sure we handle this in a way that doesn't break anyone's sites. But I'm not okay leaving this broken "forever". 😅
Thanks,
-Derek
FWIW, group 8.x-1.6 is the "final" Group V1 release, which supports D10. Could be nice to have a D10 compatible version of the 8.x-1.x branch here, too, and then deal with the Group V2+3 stuff in the 2.0.x branch.
Thanks for working on this project and this issue. However, "self-RTBC" isn't really a thing. 😅 Drupal values peer review. Moving back to NR for now. I hope to have a chance to actually review this properly and if it's truly ready, to RTBC it.
Cheers,
-Derek
For anyone else coming here wondering what happened, this was committed to 8.x-1.x, but the commit message didn't follow any of our conventions so there's no issue NID in the history, and therefore, no link from this issue. But it's this one:
commit bd726657071c0afd75cb51fd718406ea7907e199
Author: Andrew Sereda <38277-sic@users.noreply.drupalcode.org>
Date: Mon May 6 13:41:39 2024 +0000
Date only widget test; offset fix; increment setting changes for date only
https://git.drupalcode.org/project/datetime_extras/-/commit/bd726657071c...
That commit is included in the 8.x-1.2 release. Phew. 😉
This issue came up as a random triage target via the #bugsmash initiative. I haven't directly tried to reproduce, but from the description, it sounds potentially valid. Reminds me of other issues where these elements are not rendered properly. Adding 🐛 Datetime and Datelist elements should render as fieldsets Needs work as related. I wonder if fixing that issue would solve this one "for free"...
That said, the specific "steps to reproduce" in the summary are somewhat bogus. When creating a form, you don't need/want to individually try to render parts of the form like that. If this is specifically about using AJAX, it'd be great to see valid steps to reproduce involving the use of AJAX. As it is, this mostly looks like user error.
Also, we already had the whole discussion about the UI needing "immediate" feedback (which is not true). Nothing about project browser will be "immediate", anyway. The UI already has to handle "waiting to download", so it can also handle "waiting to install". See #3346707-30: Add Alpha level Experimental Package Manager module → for example.
Yes please! I thought we already did this months / years ago? #3346707-18: Add Alpha level Experimental Package Manager module → and later comments resulted in 📌 Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed (among others).
There was a request in the #ux Slack channel for a "technical" review. I tried my best. Couldn't help myself from also bikesheding on some of the new UI text. 😅 But I also tried to find any technical concerns.
This is mostly looking really good. Definitely some big improvements over what we've already got!
Disclaimer: I only reviewed the MR diff. I didn't try to apply locally, test in the UI, etc.
Thanks,
-Derek
Re #47: you already can. See #2953489: Add Twig support in Subject, Email, and Message fields →
Major gratitude and appreciation for all the UX/UI help, wisdom and expertise you've provided over the years! I've learned so much from working with you, both on Drupal Core and Drupal.org itself. You'll be sorely missed. Blessings to you on all your future endeavors!
FWIW, exactly this feature already exists in https://www.drupal.org/project/datetime_extras →
Trivial MR.
Issue summary and title match the diff.
Pipeline is green, other than the PHP 8.5 warnings.
Tagging to be smashed.
Nothing else to do or improve that I see, unless we want a follow-up to add something to automatically flag further regressions to using the legacy group?
Possible ways to implement a collective solution:
- Add another heading to the default issue body template called “Draft commit message”.
- Adopt the convention that the body of the MR (or at least part of it) should be a draft commit message.
- Use GitLab commit templates and hopefully get a bunch of the trailers and other goodies automagically per the end of comment #36.
For 1 and 2, core committer copy/pastes from the issue or MR, not the credit UI. For 3, maybe we start merging via GitLab?
Sorry, misread description of “chore”. It’s for non src + test. With build, ci, docs, etc all split out, that doesn’t leave much else a commit could touch. 😂 Maybe “chore” is fine for those cases, and we don’t actually need “task”? 🤔
One possible governance mechanism could be to consider the suggested formatting of git history under the auspices of coding standards. We’ve got an active process for that, it includes core people and non core, etc.
Many / most of our issues are a combination of many of those types. At least test + X. Sometime perf, fix, test, docs all in one. I suppose if we have sufficiently tight scope, some issues could be a single one. But that’s pretty rare. We’d have to assume that fix, feat, perf all imply the corresponding test, docs, etc.
Meanwhile, we use “task” as a catch-all issue category, but we could often be more specific for the commits. So I appreciate the specificity of the list. Yet I wonder if we should use “task” instead of “chore” for Drupal?
#5 looks good for the UI, thanks!
Finally, I’m still interested in hearing about a collective solution instead of something committers have to do entirely on their own. I’m sad we’re proposing to simplify this message so much, only to make the bespoke tooling a little easier, instead of seeing about ways to make this whole task part of the work of getting an issue ready to be committed and then being able to take fully advantage of the improved format for the benefit of out Git history.
Thanks,
-Derek
I wasn’t sold on deviating from conventional commits to put the issue number at the front of the line. My original proposals in the initial issue were all to put the issue number after the colon, as the start of the description, since that’s fully compliant with the standard. So long as we don’t get too creative with the type keywords, and mostly use fix, task and feat, it should still be pretty scannable by humans, and totally readable by tools written for the spec. Even with the issue number the front, they’re not all the same number of digits, so they won’t perfectly line up, anyway.
If you’re looking at a rebase or some other action where you only see the 1-line summaries, it’d be something like:
feat: [#12345] Whatever something nice to say
fix: [#12346] Some other important thing
task: [#1234789] Clean up the whatever
Vs.
[#12345] feat: Whatever something nice to say
[#12346] fix: Some other important thing
[#1234789] task: Clean up the whatever
IMHO, the 2nd isn’t fundamentally more scannable by humans, but it’s much less scannable by tools written to support the spec.
#34 is not bikeshedding over hypotheticals. Without email addresses (even hard-coding the no reply if we have to), it’s going to really be hard for any other tooling to be helpful, or for GitLab to do its own magic. Can we prevent letting perfect be the enemy of good and do a best-effort for emails, even if there are edge cases where it doesn’t work flawlessly?
Sorry, only on my phone. Hard to closely review. But found a few concerns to start.
Yup, I’ll hopefully get something out next week. Been working at a music camp and offline for a while, but starting to catch up on virtual work now…
Re emails, usernames, etc: sounds like there are some edge cases where we don’t know everything. But a little work could go a long way to providing as much info as we have in the vast majority of cases, no? Eg if I could set my GitLab email address to match the email address I have my local git configured to use, and I always pushed commits with the same address, everything would Just Work(tm), no? Worst case that someone borked this and the same user had 2 different emails? Big deal, we either see both of them or the last one or whatever. It’s still better than having no way to link anyone to a commit other than the Author.
(From the summary):
Any further modifications can be done after the copy/paste of the suggested message.
This is part of the approach to this whole problem that I find a bit problematic. I’d rather this wasn’t entirely dependent on core conmitters to do manually. That’s already a bottleneck. I’m advocating for a solution where at least subsystem maintainers, if not anyone contributing to an issue, can collectively improve this git history in advance. Committers can always override and further edit before they actually push the commit, but we could lighten the load significantly with a collective solution.
TL;DR: I’d rather have no information than wrong information.
-1 to Co-authored-by, +1 to By.
If my heuristic for automatically differentiating co-authors from reviewers in the other issues (if you push commits to the MR, default to co-author, else, default to reviewer) isn’t feasible for technical reasons and/or we’re worried about people debating it case-by-case, MR suggestions blurring the line, etc, I think I’d prefer just By: . Both for copyright and for honesty, I’d rather not call every participant in an issue / MR a co-author. By is bland and vague, just like we use now, but doesn’t imply everyone wrote the change.
When I was originally spearheading this commit format change for core, I was hoping to use it as an opportunity not just to adopt useful 1-line summaries without an incomprehensible wall of usernames, but also to more accurately reflect the reality of who contributed to the change and how. If we call everyone a co-author, it’s less accurate, not more.
Since doing the most accurate things automatically are revealing some technical hurdles and other resistance, the other approach I keep pondering / suggesting is to put the draft commit message to use in the MR body and/or as another section of the issue template and let folks craft / refine the draft commit message collectively. We already have a culture of encouraging people to keep the issue summary accurate. We already have a “field” in the template to collectively write a release note. However, I know folks don’t always do this. Core committers could start refusing to commit RTBC things if they don’t have this message already written. We do that for CRs and release notes. We’ll delay issues for weeks/months nit picking code comments, but then we leave the urgent task of the Git history entirely up to committers to have to do on their own.
The other option, if relying on MR/issue text isn’t going to fly, would be if the credit UI saved state and we could draft a “better” message via that UI. But that’s probably a whole other can of worms.
If there’s no draft, we need a quick / easy way for committers to get something useful, which is what this issue aims to provide. So if every attempt at reflecting reality is going to be ruled out, I vote for something vague and true, not specific and (partly) false, even if GitLab would provide some limited “magic” automatically if we used Co-authored-by.