Essentially, the membership loader has bothered me for a while now. Even though it enables scenarios where you can swap out the service to load external memberships (like Active Directory), the key problem that arises is that our query access relies on said information to be stored in Drupal's database.

It feels like a broken promise to allow runtime membership info to come from an external source, but to rely on database info when checking query access. Therefore, we should take one step of abstraction back and make the system "know" that membership info always comes from the DB. In a later phase we could try and come up with a way for external systems like AD to populate the DB with the same necessary information.

Now that #3382831: Allow shared bundle classes for group relationships using the same plugin has landed, we can already introduce a shared bundle class for memberships, but still keep the old membership loader and wrapper. This ensures full BC and in 4.0.0 we can then drop the membership loader and wrapper and start directly using the new shared bundle classes. But in the meantime, people who want to start using them already, can.

Issue fork group-3382559

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

kristiaanvandeneynde created an issue. See original summary.

kristiaanvandeneynde opened merge request !100

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Active » Needs review

This still needs tests, especially for the shared bundle class concept.

The logic we need to test is:

  • No shared bundle class, no bundle class: Entity is instance of GroupRelationship
  • No shared bundle class, bundle class: Entity is instance of the bundle class
  • Shared bundle class, no bundle class: Entity is instance of the shared bundle class
  • Shared bundle class, bundle class: Entity is instance of the bundle class

We should also test the loaders because we never had tests for that in the past. They're just a few simple loaders, but it doesn't hurt to have some tests.

Related:

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

It is some form of BC break, but I reckon the fallout should be minimal to none because we never typehinted for the return value and allowed people to return just about anything in the past as we had no interface. As long as you had the right methods, you were a-okay and that still holds true with the changes suggested here.

So old code should still keep working as long as we don't start typehinting for GroupMembershipInterface before v4.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Perhaps we shouldn't remove the old GroupMembership wrapper just yet, though. That's a bit overzealous of me, will add that back in and then look at the test fails.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Gonna split this up into two issues:

  • Create the new concept of bundle classes + test this (new issue)
  • Implement a GroupMembership bundle class and deprecate the membership loader (this issue)
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Related issues: +#3382831: Allow shared bundle classes for group relationships using the same plugin
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Issue summary: View changes
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Title: Get rid of the membership loader in favor of something others can use too » Deprecate the membership loader in favor of shared bundle classes

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs review » Fixed

mvogel’s picture

mvogel commented

Hi,
nice rewrite, thank you.
One question: Will the Group entity method `getMember()`be rewritten as well in version 2.x, 3.x?


  public function getMember(AccountInterface $account) {
    return $this->membershipLoader()->load($this, $account);
  }

  public function getMembers($roles = NULL) {
    return $this->membershipLoader()->loadByGroup($this, $roles);
  }
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

That would be a BC break. For now, I need to keep calling the membership loader because people may have decorated it or swapped it out. Inside of the membership loader, we are already using the new code. Starting from 4.0.0 we will call the new code directly.

mvogel’s picture

mvogel commented

Ok, got it. Thanks for the quick answer

Status: Fixed » Closed (fixed)

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