Problem/Motivation

I'm maintaining entitygroupfield. I'm trying to get some new releases out, and fixing up GitLab CI woes. Recent commits to the *.3.x branches (both 2.3.x and 3.3.x) have broken the group module on both 11.1.x and 10.5.x core.

#3533155: Fix CI after core updating to newer version of phpunit. makes it impossible to even install group 3.3.x-dev on 11.1.x or 10.5.x core. See https://git.drupalcode.org/project/group/-/pipelines/645738 (a manual pipeline I created with the OPT_IN_TEST_PREVIOUS_[MAJOR|MINOR] flags set to 1).

However, more troubling is that if I manually resolve that locally, if I try to run any tests on 10.5.x with 3.3.x of group, everything is a fatal error along these lines:

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "Drupal\group\Hook\EntityHooks".

That's because #3548434: Support object-oriented hooks in 3.3.x and 2.3.x branches added the LegacyHook stuff like so:

/**
 * Implements hook_entity_bundle_info().
 */
#[LegacyHook]
function group_entity_bundle_info() {
  return \Drupal::service(EntityHooks::class)->entityBundleInfo();
}

But none of these Hook classes are actually services automatically. We also need to tell the world about them in group.services.yml.

Calling this critical since it qualifies under both of these points:

  • Render a site unusable and have no workaround.
  • Cause tests to fail in HEAD on the automated testing platform for any supported environment (including random failures), since this blocks all other work.

Steps to reproduce

  1. Install the 3.3.x-dev branch of group on a 10.5.x core site.
  2. Try to do anything. 😅

Proposed resolution

  • Enable these test_previous jobs in the default GitLab CI config here and test the BC.
  • In the case of the LegacyHook stuff, add the right entries to group.services.yml per the CR about OOP hooks.
  • Broaden composer requirement for jangregor/phpstan-prophecy so we can still install on older versions of core.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork group-3555468

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dww created an issue. See original summary.

dww opened merge request !268

dww’s picture

dww
we/he/they
commented
Status: Active » Needs review

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.

dww opened merge request !269

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

Looks right to me!

I confirmed the core module hook classes all have the right services entry.

lostcarpark made their first commit to this issue’s fork.

lostcarpark’s picture

lostcarpark commented

I attempted to fix phpstan (previous major) with a sed script. I eventually overcame the issues with my script and got it removing attributes correctly, however there were some errors not related to attributes. Agreed with @dww that we should revert from this change and move into a follow-on issue.

dww opened merge request !270

dww closed merge request !270

dww’s picture

dww
we/he/they
commented

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 #3548434: Support object-oriented hooks in 3.3.x and 2.3.x branches landed there, too).

The pipelines on both branches are green now, except for the warning about phpstan on 10.5. See #3555505: Fix PHPStan for previous major

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.

  • dww committed c709cb55 on 3.3.x 
    fix: [#3555468] Restore D10 compatibility:

- Register Hook classes as...

  • dww committed 1b504565 on 2.3.x 
    fix: [#3555468] Restore D10 compatibility:

- Register Hook classes as...

dww closed merge request !268

dww closed merge request !269

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Okay so reading up on Slack and the issues over the weekend, I want to get the most important thing out of the way first: I am glad people are looking after my software in my absence. Let that be the key takeaway here, regardless of what I write next. So thanks, everyone!

With that said, the only thing I was a bit surprised about was the speed and urgency this was dealt with. It's my understanding that dev releases are allowed to break and can never be critical unless someone spotted a supply chain attack or something that needs to be addressed ASAP. So seeing this issue marked as Critical and resolved before I could have a chance at reviewing it did make me raise an eyebrow. The "Seems better to ask for forgiveness than permission" mantra has a time and place for it, but I don't think the building was that much on fire here.

Still, it's my mistake to begin with for adding the OOP hook backport to previous versions without realizing that those were no longer testing on D10, even though that version is still supported. I'm sure there's an excuse to make about not having as much contrib time as I used to, but at the end of the day it's still my responsibility to make sure all checks have passed.

Nice catch finding the "group_modules_installed" mistake, by the way. I also appreciate that you went and "put out the fire" but left the phpstan stuff for another issue as there is indeed less pressure to fix those warnings.

So all in all, I'm more happy than annoyed about what happened. For the first time in over a decade I've got the feeling that I'm not alone in looking after Group. So that heavily outweighs the fact that I feel the urgency was a bit exaggerated here :)

Either way, thanks!

dww’s picture

dww
we/he/they
commented

@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

dww’s picture

dww
we/he/they
commented

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

Status: Fixed » Closed (fixed)

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