The group context process is bootstrapped here:
function og_context_init() {
og_context();
}
But since the $group_type
argument defaults to node
in og_context()
nodes are the only group type that can be automatically detected.
So, what we need is not to make any assumptions on group types.
The attached patch fixes this and slightly changes the og_context_determine_context()
function to return a full $context
array that we can read the group type out of if nothing was passed into og_context()
.
This would be considered an API break I guess, but we have quite a few OG modules installed, and og_context_determine_context()
was only called from within og_context()
, so I guess it should be fine anyhow.
Patch coming soon.
Comments
Comment #1
dixon_Here we go. I haven't looked into the tests yet. But I probably should. At some point.
Comment #2
dixon_Making myself a bit more clear.
Comment #3
amitaibuWouldn't it be enough for to add a variable - something like:
Comment #4
amitaibuActually better:
Comment #5
dixon_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.
Comment #6
amitaibuOk #1 looks good, @RoySegall is working on adding a test for this.
Comment #7
RoySegall CreditAttribution: RoySegall commentedI 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.
Comment #8.0
(not verified) CreditAttribution: commentedFixed spelling mistake.
Comment #9
joachim CreditAttribution: joachim commentedI 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 :(
Sinmpletest mistake number 0: not enabling the right modules. (Happens to me too. Takes me ages to spot.)
Typo in the bundle name passed to og_create_field(), so the terms were never ending up as groups.
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.
Comment #12
joachim CreditAttribution: joachim commentedI 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.
Comment #13
joachim CreditAttribution: joachim commentedThis 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().
Comment #14
joachim CreditAttribution: joachim commentedI'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:
after:
og_context_determine_context_info() is a terrible name though. Any better suggestions?
Comment #15
joachim CreditAttribution: joachim commented> 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.
Comment #16
amitaibuInstead of the code change, we could have a variable that decides if we return early in
og_context_init()
, before callngog_context()
-- giving you the option to call it yourself with whatever group type you want.Would that help?
Comment #17
tom friedhof CreditAttribution: tom friedhof commentedThe patch in #12 works for me and my use case. Thanks for the patch!
Comment #18
dixon_Unassigning myself since I've left the project that made use of this...
Comment #19
joachim CreditAttribution: joachim commented> 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 :/
Comment #20
amitaibu> 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.
Comment #21
joachim CreditAttribution: joachim commentedAgreed, but having to set a variable to be able to use non-node contexts would be pretty bad DX :(
Comment #22
amitaibuI think that API change VS a variable for more advanced installation, the variable wins. Open to discussion.
Comment #23
joachim CreditAttribution: joachim commentedIt'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.
Comment #24
joachim CreditAttribution: joachim commented> 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.
Comment #26
amitaibuCommitted, 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 ;)
Comment #27
joachim CreditAttribution: joachim commentedDone, but if you could check I've got the details right that would be great: https://drupal.org/node/2231399.
Comment #29
amitaibuSorry, I've reverted the commit.
Problem is that if you set the context programtically
og_context('node', $node),
and re-call it withog_context()
, you won't get the context, as$context['group_type'] == $group_type
is now FALSEComment #30
azinck CreditAttribution: azinck commentedIt seems like we could just change those lines to read:
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?
Comment #31
azinck CreditAttribution: azinck commentedPatched according to my suggestion in #30
Comment #32
joachim CreditAttribution: joachim commented> 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:
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().
Comment #33
azinck CreditAttribution: azinck commentedI made some notes on your other issue to explain the condition.
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.
Comment #34
azinck CreditAttribution: azinck commentedYou 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):
...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.
Comment #35
azinck CreditAttribution: azinck commentedAmitaibu: can you weigh in on the questions I raised in #34? How is this intended to work?
Comment #36
shushu CreditAttribution: shushu commented@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.
Comment #37
shushu CreditAttribution: shushu commentedComment #38
azinck CreditAttribution: azinck commentedImagine 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.
Comment #39
azinck CreditAttribution: azinck commentedI 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.
Comment #40
sheise CreditAttribution: sheise commentedPatch in #31 is working for me.
Comment #41
azinck CreditAttribution: azinck commentedHoping to get more reviews of #31.
Comment #42
sheise CreditAttribution: sheise commentedI've been using #31 on a live site for about a year now it's be working great.
Comment #43
sheise CreditAttribution: sheise commentedThe patch was no longer applying cleaning to the latest release. Here's a re-rolled patch. It could use review.