Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert Og to a proper service that extends OgInterface #290

Closed
amitaibu opened this issue Aug 5, 2016 · 4 comments
Closed

Convert Og to a proper service that extends OgInterface #290

amitaibu opened this issue Aug 5, 2016 · 4 comments

Comments

@amitaibu
Copy link
Owner

amitaibu commented Aug 5, 2016

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
Copy link
Owner Author

amitaibu commented Aug 5, 2016

@pfrenssen sounds right to you? ^^

@amitaibu
Copy link
Owner Author

amitaibu commented Aug 5, 2016

Started work on #291

@pfrenssen
Copy link
Collaborator

pfrenssen commented Aug 6, 2016

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 #226.

@amitaibu
Copy link
Owner Author

amitaibu commented Aug 6, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants