amitaibu / og

A fork to work on OG8
https://github.com/Gizra/og
29 stars 16 forks source link

Convert Og to a proper service that extends OgInterface #290

Closed amitaibu closed 7 years ago

amitaibu commented 7 years ago

This one is mostly coming from my wish not to do so much mocking in the unit tests when calling for example Og::IsMember()

If we could have a prophecy of OgInterface, it will be much simpler, and remove some ugly parts from the tests.

To still provide a quick access to regular OG functions we could have

function og() {
  return \Drupal::service('og');
}

function foo() {
  og()->isMember($group, $user);
}
amitaibu commented 7 years ago

@pfrenssen sounds right to you? ^^

amitaibu commented 7 years ago

Started work on #291

pfrenssen commented 7 years ago

I am VERY much in favour of this idea! For me the most important aspect of this is also to move away from the static methods, and relying fully on dependency injection so that our unit tests will be properly able to mock the methods.

It will be a big task though. We might need to split this in smaller PRs.

function og() {
  return \Drupal::service('og');
}

function foo() {
  og()->isMember($group, $user);
}

For me this is a very good approach. It is easy to use from the procedural side. Another possibility is to keep Og with its static methods, but move all actual logic out to respective services. This would be very recognizable for developers since it works exactly the same as the very well known \Drupal class. All static methods in Og would then be simple wrappers around the actual services. It would look something like this:

public static function isMember($group, $user, $states) {
  return \Drupal::service('og.membership_manager')->isMember($group, $user, $states);
}

To call the code from the procedural side it would remain the same as it is now (and identical to how it is done in \Drupal):

function foo() {
  Og::isMember($group, $user);
}

The advantage of this is that we will be able to move the logic into separate services, which each are responsible for their own domain (i.e. their own entity type). We already have GroupManager and PermissionManager. I'd love to also have RoleManager and MembershipManager.

In my mind there is not really Og specific functionality, everything is somehow more directly related to groups, group content, memberships, roles etc. So Og should not have any actual logic. I might be overlooking some things though.

For some guidance on how to approach this conversion: we have converted other classes containing mainly static methods to services before. An example is https://github.com/amitaibu/og/pull/226.

amitaibu commented 7 years ago

I like your suggestion better! - Keeping Og a static class wrapping other services, will also allow us to take it in smaller steps.

So, I could start with a og.membership_manager service, that will have all those ::getMemberships related methods.