"node" group types are the only context that can be automatically detected

Wouldn't it be enough for to add a variable - something like:

/**
 * Implement hook_init().
 */
function og_context_init() {
  og_context(variable_get('og_context_init_entity_type', 'node'));
}

Actually better:

function og_context($group_type = NULL, $group = NULL) {
  $group_type = $group_type ? $group_type : variable_get('og_context_default_group_type', 'node');

  // ...

Ok #1 looks good, @RoySegall is working on adding a test for this.

I saw that the patch don't contain any taxonomy term handler so i wrote one. I start work on the test but in the simple test the og_context function don't return any thing.

I'm attaching a path with a test - the test will be failed but we anyone can take from here.

I can confirm the patch fixes the problem for me using a custom entity type as group type.

Here's a patch which fixes the tests.

This is what was causing the problems in the test. All horrible little mistakes that are easily made and hard to spot :(

1. +++ b/og_context/og_context.test
   @@ -0,0 +1,62 @@
   +    parent::setUp('og');
   

   Sinmpletest mistake number 0: not enabling the right modules. (Happens to me too. Takes me ages to spot.)

2. +++ b/og_context/og_context.test
   @@ -0,0 +1,62 @@
   +    $vocabulary->machine_name = 'terms';
   +    taxonomy_vocabulary_save($vocabulary);
   +    og_create_field(OG_GROUP_FIELD, 'taxonomy_term', 'term');
   

   Typo in the bundle name passed to og_create_field(), so the terms were never ending up as groups.

3. +++ b/og_context/og_context.test
   @@ -0,0 +1,62 @@
   +    debug(og_context());
   

   The problem here is that when you're calling this here, you're not 'in' the site you're testing. You're not 'on' the page you've requested -- your in the code that's running the test and 'looking' at the test browser HTML. So that's not going to work at all, unfortunately.

   The solution -- at least the solution I've come up with -- is to have a test module which implements hook_entity_view(), and in that, outputs a message about the OG context it can find.

I've also simplified og_context_handler_taxonomy_term() -- the work it was doing can just as easily be done by _group_context_handler_entity(), provided you tell it what menu position it should look in to get an entity.

I really don't know why the testbot is complaining -- the patch applies to HEAD with only 1 line of fuzz.

Here's a reroll on HEAD.

This will definitely need a change notice -- I just got bitten by the API change in one of my custom modules that was calling og_context_determine_context().

I'm wondering whether to prevent API break we should add a new function and have og_context_determine_context() as a wrapper to preserve the old behaviour.

So:

before:

og_context_determine_context() {
  return $gid;
}

after:

og_context_determine_context_info() {
  return $context_info_array();
}

og_context_determine_context() {
  $context = og_context_determine_context_info();
  return $context['gid'];
}

og_context_determine_context_info() is a terrible name though. Any better suggestions?

> I'm wondering whether to prevent API break we should add a new function and have og_context_determine_context() as a wrapper to preserve the old behaviour.

Here's a patch that does that.

The fixed function is og_context_determine_context_info().

The existing function og_context_determine_context() is now just a wrapper around that, so that we don't break any code that calls it.

We probably still need a change notice to alert people that og_context_determine_context() has been marked as deprecated.

The problem with that is that we sometimes have node contexts and sometimes term contexts that needs to be automatically detected. So I don't think "hard coding" it in a variable is a good solution to this problem.

Instead of the code change, we could have a variable that decides if we return early in og_context_init(), before callng og_context() -- giving you the option to call it yourself with whatever group type you want.

Would that help?

The patch in #12 works for me and my use case. Thanks for the patch!

> Instead of the code change, we could have a variable that decides if we return early in og_context_init()

That would then mean that on sites that use non-group nodes, you'd need to know to set that variable. It would also be faffy to have to deploy that in strongarm :/

> That would then mean that on sites that use non-group nodes, you'd need to know to set that variable

Which to my knowledge is a pretty small percentage. A change record and a README can help here.

Agreed, but having to set a variable to be able to use non-node contexts would be pretty bad DX :(

I think that API change VS a variable for more advanced installation, the variable wins. Open to discussion.

It's been a while since I worked on the patch, but I think that #15 avoids the API break. Instead, we wrap the existing function, with the intention of eventually deprecating it when it's safe to do so.

> but I think that #15 avoids the API break. Instead, we wrap the existing function, with the intention of eventually deprecating it when it's safe to do so.

I've just re-applied the patch to an update of OG in my project's codebase, and I can confirm this.

The parameters and return of og_context_determine_context() are unchanged. A new function, og_context_determine_context_info() is added, which provides more comprehensive data in its return value. OG is changed to make use of this instead, and og_context_determine_context() is changed internally to use the new function, so it becomes just a wrapper for backwards-compatibility.

That means that contrib modules and custom code that use og_context_determine_context() will not see a change, though og_context_determine_context() is marked as deprecated and they should change over to the new function in due course. og_context_determine_context() can be changed for D8.

Committed, thanks. Setting to needs work for the change record.

@joachim, any chance for help with the change record? -- I guess your English would be better than mine ;)

Done, but if you could check I've got the details right that would be great: https://drupal.org/node/2231399.

Sorry, I've reverted the commit.

Problem is that if you set the context programtically og_context('node', $node), and re-call it with og_context(), you won't get the context, as $context['group_type'] == $group_type is now FALSE

  if (empty($group) && $context !== FALSE && (empty($context)) || (!empty($context['group_type']) && $context['group_type'] == $group_type)) {
    return $context;
  }

It seems like we could just change those lines to read:

  if (empty($group) && $context !== FALSE && (empty($context)) || (!empty($context['group_type']) && ($context['group_type'] == $group_type || $group_type == NULL))) {
    return $context;
  }

Strictly speaking, this is still an API change as an argument-less call to og_context() might be explicitly seeking a node context, but maybe we could count that as highly unlikely?

Patched according to my suggestion in #30

> as an argument-less call to og_context() might be explicitly seeking a node context, but maybe we could count that as highly unlikely?

Unfortunately, I strongly doubt that is the case. The most popular use of OG AFAICT is with both groups and groups content as nodes. In OG itself there are three calls to og_context() without any parameters.

I'm not sure what we can do here. For a start though, I am completely unable to figure out what this condition is doing:

  if (empty($group) && $context !== FALSE && (empty($context)) || (!empty($context['group_type']) && $context['group_type'] == $group_type)) {
    return $context;
  }

I simply can't parse that, sorry. It needs to be broken up and commented, probably in a separate helper issue that we can get committed and then return to this one: #2270863: refactor scary condition in og_context().

I made some notes on your other issue to explain the condition.

Unfortunately, I strongly doubt that is the case. The most popular use of OG AFAICT is with both groups and groups content as nodes. In OG itself there are three calls to og_context() without any parameters.

I agree that using it with nodes is a common use-case. My suggestion would almost certainly not break most use-cases. My suggestion would only run into trouble if you had multiple valid OG contexts on the same page; each based on different entity types and your call to og_context needed to be sure to target only the node OG contexts.

You know, the more I think about this the more I think the current (pre-patch) logic is broken. Any calls to og_context that result in no matches will cause any future calls to og_context that don't supply a $group to return an empty context, even if there might be a valid context available.

Imagine calling og_context('taxonomy_term'). If that results in no matches (because, say, there are no taxonomy_term group contexts on the page) then $context will be statically cached as an empty array. Future calls to og_context('node'), or og_context('user'), or og_context('your_arbitrary_entity_type_here'), or just og_context() for example, will hit the conditional in question (copied here in its pre-patch form):

if (empty($group) && $context !== FALSE && (empty($context)) || (!empty($context['group_type']) && $context['group_type'] == $group_type)) {
    return $context;
  }

...and return the statically-cached $context prematurely even if there's a valid context for your specified $group_type on the page. It seems to me og_context_determine_context() should be getting called instead.

Is this the way the API is intended to function? It seems quite...hard to understand. If it's not the way it's intended to function then I strongly suggest we change the way we're using static caching.

Amitaibu: can you weigh in on the questions I raised in #34? How is this intended to work?

@azinck, can you help me define a simple scenario is which the current code will cause real use case problem ?
I think that once we get a clear use case it will be simpler to push forward a fix.
Thanks.

Imagine a site that has groups based on both nodes and taxonomy terms. There's no reliable way to get the group context. Perhaps you would first call og_context('node') to see if there's a group context of type 'node'. If there's not, then you would want to call og_context('taxonomy_term') to see if there's a taxonomy-term group context. Trouble is, even if there is and should be a valid taxonomy_term group context it will not be returned in this case. The first call to og_context('node') that finds no contexts effectively pollutes any further calls to og_context.

If, as in our example, no groups are returned in the first call then no groups will ever be returned in subsequent calls, no matter what type of group you're looking for.

I would like to re-propose my suggestion from #30.

We could change the logic so that an argument-less call to og_context returns any context, regardless of type.

Additionally, I propose that we refactor the static caching to cache on a per-type basis.

I would argue that this would be clarifying a previously unclear API rather than truly breaking it.

I also don't see any reasonable alternatives.

Patch in #31 is working for me.

Hoping to get more reviews of #31.

I've been using #31 on a live site for about a year now it's be working great.

The patch was no longer applying cleaning to the latest release. Here's a re-rolled patch. It could use review.