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

WIP: First sketch of the OG context module. #252

Merged
merged 36 commits into from
Nov 8, 2016
Merged

WIP: First sketch of the OG context module. #252

merged 36 commits into from
Nov 8, 2016

Conversation

RoySegall
Copy link
Collaborator

@RoySegall RoySegall commented Jul 17, 2016

Goal

Port og-context module to OG8.

GroupResolver are plugins that can extract group for the pages or entities they are acting on. One can get the groups from the router, and another arbitrary one that will do it by the time of the day.

Those plugins pass their groups to the OgContextProvider that will select the "best matching" one, and return it. The best matching logic is currently (by order):

  1. Show a group a user has access to
  2. If user visited on previous page content under group B, and they are now visiting a content under group A & Group B - we will show them group B as context - for continuity.
  3. User is member of the group

Naming

  • Context provider : class OgContext implements ContextProviderInterface {
  • Plugin manager: class GroupResolverManager extends DefaultPluginManager {
  • Single Plugin: class GroupResolver extends GroupResolverInterface {

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.

@@ -0,0 +1 @@
og_context:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo here: neogation instead of negotation. It's probably also not necessary to prefix the config ID with ogcontext because the config system already automatically provides the prefix og_context..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10x

@RoySegall
Copy link
Collaborator Author

@amitaibu Before I'll start implementing the changes don't you want me to moe og context plugin into OG core?

@RoySegall
Copy link
Collaborator Author

I moved OG context into core. Now I can say I started the work on OG context.

@RoySegall
Copy link
Collaborator Author

Another update - Now we have the next two cool, IMO, API:

    $plugins = Og::contextHandler()->getPlugins();
    $plugin = Og::contextHandler()->getPlugin('entity');

* "uuid" = "uuid"
* },
* links = {
* "collection" = "/admin/structure/visibility_group"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might not need to this PR....

@RoySegall
Copy link
Collaborator Author

OK. I got the rename of the classes. Now I need to extend the OgContext form ContextProviderInterface

@amitaibu
Copy link
Owner

What's the status here?

@RoySegall
Copy link
Collaborator Author

I started to work on extending from ContextProviderInterface and I got interrupted. I'll try to get back to it tonight.

@RoySegall
Copy link
Collaborator Author

@amitaibu I extended OgContextInterface from ContextProviderInterface and I there are two methods I need to implements but I still can't see how they suppose to fit in.

If some one can throw some tips that would be awesome.

@RoySegall
Copy link
Collaborator Author

I dig a bit in the code and it seems that we need to create a service, in addition to the context group resolver, that will look like this:

  og.og_route_context:
    class: Drupal\og\ContextProvider\GroupRouteContext
    arguments: ['@current_route_match']
    tags:
      - { name: 'context_provider' }

and that service will use the logic I created so far. After that we can have in any plugins something like this:

/**
 * Provides a 'User Role' condition.
 *
 * @OgTask(
 *   id = "people",
 *   label = @Translation("People"),
 *   context = {
 *     "group" = @ContextDefinition("group", label = @Translation("Current group"))
 *   }
 * )
 */

But IMO this will be another PR. In the mean while I'll keep working in the tests.

@RoySegall
Copy link
Collaborator Author

I added a skeleton for the test and this time I did not got any weird errors from PHPUNIT.

@amitaibu
Copy link
Owner

Great. Can you please re-roll, and re-open this PR against Gizra/og

@RoySegall
Copy link
Collaborator Author

One the tests will work as expected I'll create new PR against gizra/OG. In the meanwhile don't close the PR.

10x.

@pfrenssen
Copy link
Collaborator

@RoySegall would you be OK if I continue on this? I have time to work on it this week.

I would like to start with adopting the naming scheme that @amitaibu suggested in #282 (comment):

  • Context provider : class OgContext implements ContextProviderInterface {
  • Plugin manager: class GroupResolverManager extends DefaultPluginManager {
  • Single Plugin: class GroupResolver extends GroupResolverInterface {

And then secondly get the tests green.

*
* @package Drupal\og
*/
interface GroupResolverHandlerInterface {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this class? The documentation doesn't explain it. Why do we need an additional class to handle GroupResolver plugins? Isn't this the task of GroupResolverManager?

This is confusing to me since this class is only used in tests so far, there is no real code calling into it. It would be really helpful if the purpose of this class was documented.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK so this is a service that is responsible for looping over the context plugins and resolving the group that best matches the current state. It's a bit strange that most of the methods here are directly related to retrieving and updating plugins, which sounds like the job of the plugin manager to me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be much clearer if this was renamed to GroupResolverInterface, the 'handler' part is what makes it confusing to me, it brings up associations with EntityHandlerInterface :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe OgGroupResolverInterface?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@pfrenssen
Copy link
Collaborator

I had a look through the code but it seems to me that the chosen approach is more complex than needed. Maybe I am not seeing the use cases that require this complex approach, but the way I imagined this to work was like this:

  • Some code needs to know the group context and calls OgContext::getRuntimeContexts().
  • OgContext::getRuntimeContexts() loads the og.group_resolvers config to get the list of enabled group resolvers in the right order.
  • OgContext::getRuntimeContexts() requests the enabled group resolver plugins from GroupResolverManager::getInstance().
  • OgContext::getRuntimeContexts() calls each plugin in the right order, and if it finds one that returns a single result, it loads this group and returns it as context.

In my mind the whole procedure would be like 50 lines of code + a couple hundred lines for the default config, the individual plugins and tests.

This approach with the separate handler and saving plugin configuration in a dedicated GroupResolverNegotiation entity, and RETURN_ONLY_IN_STORAGE seems to be quite complicated for what we are trying to achieve.

@amitaibu
Copy link
Owner

I always enjoy seeing your code review!

I think your analysis is very good, with a single remark:

OgContext::getRuntimeContexts() calls each plugin in the right order, and if it finds one that returns a single result, it loads this group and returns it as context.

OgContextProvider::getRuntimeContexts() will call the OgGroupResolver plugins, and let them return all their values. That is, it will not break on the first group that is found.

Then, it will apply some simple (hardcoded) rule to get the "Best matching" group, by order:

  1. User has that group in the session, because in the previous page requests they go it as context + user has access to it. This is for cases when yuu previously visited a group content that belonged to group A, and now you visit a group content that belongs to groups A and B -- so we should be nice and keep you in the A context.
  2. User has view access to the group
  3. The first rule

@pfrenssen
Copy link
Collaborator

Thanks for the feedback! With your permission I will continue this work and try to bring it to conclusion. I'll make a new PR with the simplified approach.

@amitaibu
Copy link
Owner

👍

@RoySegall
Copy link
Collaborator Author

@pfrenssen Any new on this one? I kind of lost context on this one in the past few weeks.

@amitaibu amitaibu merged commit e7cad2d into 8.x-1.x Nov 8, 2016
@pfrenssen pfrenssen deleted the 242 branch December 13, 2016 12:16
RoySegall pushed a commit that referenced this pull request Apr 12, 2017
Adding Drupal CS fixes.
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

Successfully merging this pull request may close these issues.

4 participants