amitaibu / og

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

add a context provider #282

Open chx opened 7 years ago

chx commented 7 years ago

Our site is pretty much split into subsites by group. Displaying the relevant blocks on a group node or a node belonging to a group is possible with this context provider. If it's not generic enough just won't fix this and I will dump it into a separate sandbox module. Here's how placing a block with * "node" = @ContextDefinition("entity:node", label = @Translation("Node"), required = FALSE), looks:

selection_315

amitaibu commented 7 years ago

I believe there's a similar effort by @RoySegall on #252 . The idea is indeed to provide a Context provider that is being fed by OgContext plugins.

amitaibu commented 7 years ago

Maybe worth joining efforts on that PR?

RoySegall commented 7 years ago

My PR was a shallow one - just plugin that return the first group without all the special logic. I added a test that send the user to a group but it fails for some reason which I'm not aware of yet.

If @chx want to join in and help with the tests - he is more than welcome.

chx commented 7 years ago

I do not quite know what that PR is about but it's not about this. I tried to understand but failed -- and when I searched for ContextProvider then I found none. Please help me understand, I do not quite understand the core context system but I read #252 and I haven't found any core interfaces implemented or tagged services so I don't get it.

amitaibu commented 7 years ago

Sure, I'm not too familiar with #252, so I will explain the idea in generel, which is porting Og-Context module to OG8.

Here's the logic:

Here's the relevant code from OG7:

@chx , Hope it clarifies a bit the intention :)

chx commented 7 years ago

So #252 is WIP and not yet integrating with core but eventually it wants to...?

amitaibu commented 7 years ago

Indeed it's WIP.

But as @RoySegall stated, he's open to collaboration, so if you'd like to tackle this task differently in terms of steps to reach the goal, that's of course possible to discuss.

chx commented 7 years ago

I still do not know whether we have the same goals :) I am providing a context for core, mostly for blocks. It uses a different logic to what you described:

  1. Get a node from URL; if it's a group return it
  2. If it belongs to a group, return the first one.

That's it. But, it's a core context provider. I simply do not see what is the end goal of yours, I am guessing it might also want to be a ContextProvider but I do not know.

amitaibu commented 7 years ago

I think we're close, however your use case is just hardcoded to getting groups out of nodes (although agreed it will be a common case).

So my goal is that instead of hardcoding the specific check for nodes in the core's ContextProvider, we delegate the job of getting that groups to OgContext plugins. So you could have one plugin that gets the context from a node, and another arbitrary one that will do it by the time of the day.

Those plugins pass their groups to the ContextProvider that will indeed select the "best matching" one, and return it.

In short: The current logic you put in place seems like a great start, and the next step is baking the OgContext plugins in.

chx commented 7 years ago

So you want to add a ContextProvider to #252 to actually use the logic created there. And then I guess add the logic from here to there as well?

chx commented 7 years ago

And to clarify further: I have no idea what hook_og_context_negotiation_info is, what module it integrated with and ... anything at all. I do not see the connection points to the rest of Drupal in the D7 version either.

RoySegall commented 7 years ago

252 need to handle a smaller scope - return the first group that the context handler will find. After the merge then we can move to higher scope. This is the case of the PR as I can see it.

chx commented 7 years ago

So let's clarify: there's no such thing as a "context handler". You are making a horribly confusing mess by calling it "OgContext" when core has a context subsystem.

amitaibu commented 7 years ago

I haven't gone over #252 properly yet, so I just know there's the effort but can't say much about the code there.

If you are ok and have time to work on the above goals, we can take your PR as a starting point as-well. I'm sure @RoySegall won't be insulted :)

And to clarify further: I have no idea what hook_og_context_negotiation_info is, what module it integrated with and ... anything at all.

No problem, I just wanted to link to the OG7 code that does the same. The gist of it is, that we are porting the same concepts, only with D8 code.

amitaibu commented 7 years ago

You are making a horribly confusing mess by calling it "OgContext"

Yes, we can of course change it. It's mostly since it was called in D7 og-context module. Context is only second to the word Content in Drupal when it comes to over using and abusing words 😉

chx commented 7 years ago

OK, so let's get back here. #252 has nothing to do with this PR then :) except perhaps I could reuse the "entity" OgContext plugin.

amitaibu commented 7 years ago

252 has nothing to do with this PR then :)

I guess that's debatable 😉 I think they are both related to the end goals -Having a pluggable context provider. They just tackle it from different ends. I wouldn't mind tackling this task in small PRs as long as the end goal is clear (unless of course you think there are better ways).

In terms of names, to prevent confusion with the core context how about:

chx commented 7 years ago

The names sound good. There are some challenges left if you want to indeed create class OgContext implements ContextProviderInterface. But I agree we eventually need to.

amitaibu commented 7 years ago

The names sound good.

Thanks for pointing out the ones I thought off sucked 😄

There are some challenges left if you want to indeed create class OgContext implements ContextProviderInterface

What difficulties do you foresee?

chx commented 7 years ago

I just discussed some of this with @timplunkett and in particular

public function getAvailableContexts() {
  $context = new Context(new ContextDefinition('entity:node', $this->t('Node from URL')));
  return ['node' => $context];

}

this makes me wonder as core does not have a term from URL, a user from URL etc context but og will have to have one as groups are any entity type so while of course it's very doable we are sort of on uncharted territory.

amitaibu commented 7 years ago

we are sort of on uncharted territory.

My favorite place 😉

timplunkett commented 7 years ago

The ones that exist in core were the result of straight ports of parts of the block visibility system. User/Term from URL could easily be added to core in 8.3 or any minor.

chx commented 7 years ago

Thanks for shoving up! What about an EntityContextProvider which does the same as NodeContextProvider but for every entity? And yes, it's relevant to this discussion because that's essentially what we would be doing sprinkled with some og logic

amitaibu commented 7 years ago

What about an EntityContextProvider which does the same as NodeContextProvider but for every entity?

Yes, that's exactly what OG will need. It would be wonderful if it could extended a core EntityContextProvider

pfrenssen commented 7 years ago

In our project we are using the OgRouteContext context provider that is bundled with OG Menu. It loops over all parameters on the route and checks if they are a group, regardless of their entity type.

I think that version would be a good starting point. It needs some tweaks though, it cannot deal with routes that have multiple group parameters.

https://github.com/ec-europa/og_menu/blob/8.x-1.x/src/ContextProvider/OgRouteContext.php

pfrenssen commented 7 years ago

Ping @sandervd who wrote the ContextProvider in OG Menu.

chx commented 7 years ago

That code is alright but compare to #252 and see the discussion above and the code in here. We do not just want a group entity to appear as the context value but the group the per route entity belongs to.

amitaibu commented 7 years ago

We do not just want a group entity to appear as the context value but the group the per route entity belongs to.

In the end the OgContextProvider will return a single group (what I call "the best matching"), but indeed it should get all the context candidates from the GroupResolver plugins

chx commented 7 years ago

Did you do the rename already to GroupResolver :) ?

amitaibu commented 7 years ago

Only in my brain, not in code yet :)

sandervd commented 7 years ago

I really like plugin based approach of @RoySegall. I would have probably invoked an event from inside ContextProvider, but the plugin approach is much more considerate towards end users (configurable weight etc.)...

If I'm not mistaking, the next step would be to wrap all of this inside a ContextProvider, right?

However, when we do so, will there be only one Og context generated, or should it be possible to create multiple 'contexts' (with individual settings)? (Let's say when using multiple group types on one site?)

amitaibu commented 7 years ago

If I'm not mistaking, the next step would be to wrap all of this inside a ContextProvider, right?

Yes. I'll add some overview to #252, so it's easier to pickup the work there.

However, when we do so, will there be only one Og context generated, or should it be possible to create multiple 'contexts' (with individual settings)?

The GroupResolver plugins will return multiple groups, but eventually OgContextProvider will select only a single one. The logic is that when your theme/language/whatever needs to change according to context it should act only on a single context.

@chx I'll close this one, and lets move collaboration and discussion to #252

amitaibu commented 7 years ago

I'd like to circle back to this one. OG8 is taking the approach oh simplifying stuff from OG7, and after some thought I think that we can un-port OG-Context module and just have a non-node specific context provider in.

The reason is that the context from the route is the most common one, and if a dev would like to extend it, they could have their own context provider.

@RoySegall @pfrenssen will that work for your use cases?

pfrenssen commented 7 years ago

I'm not familiar with what OGContext was doing in D7, but what I understood from previous discussions is that it was able to figure out the group context of things like group content that belongs to multiple groups, depending on the user session.

For me it makes sense to have a plugin based system, I see this similar to how for example language negotiation works. My context could depend on:

For complex sites a developer will need custom rules, but this would be another plugin that can complement the ones that are already there.

RoySegall commented 7 years ago

For me it makes sense to have a plugin based system

My PR provided the infrastructure for that. I'll get back to this one this week.