Skip to content
This repository has been archived by the owner on Aug 18, 2024. It is now read-only.

Og membership state cache context #163

Merged
merged 20 commits into from
Sep 20, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions og.services.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ services:
arguments: ['@entity_type.manager', '@og.access']
tags:
- { name: access_check, applies_to: _og_user_access_group }
cache_context.og_membership_state:
class: 'Drupal\og\Cache\Context\OgMembershipStateCacheContext'
arguments: ['@current_user', '@current_route_match', '@og.group_type_manager', '@og.membership_manager']
tags:
- { name: 'cache.context'}
og.access:
class: Drupal\og\OgAccess
arguments: ['@config.factory', '@current_user', '@module_handler', '@og.group_type_manager', '@og.permission_manager', '@og.membership_manager']
Expand Down
8 changes: 4 additions & 4 deletions src/Annotation/OgDeleteOrphans.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,18 @@ class OgDeleteOrphans extends Plugin {
/**
* The human-readable name of the plugin.
*
* @ingroup plugin_translatable
*
* @var \Drupal\Core\Annotation\Translation
*
* @ingroup plugin_translatable
*/
public $label;

/**
* A short description of the plugin.
*
* @ingroup plugin_translatable
*
* @var \Drupal\Core\Annotation\Translation
*
* @ingroup plugin_translatable
*/
public $description;

Expand Down
122 changes: 122 additions & 0 deletions src/Cache/Context/OgMembershipStateCacheContext.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
<?php

namespace Drupal\og\Cache\Context;

use Drupal\Core\Cache\CacheableMetadata;
use Drupal\Core\Cache\Context\CacheContextInterface;
use Drupal\Core\Routing\RouteMatchInterface;
use Drupal\Core\Session\AccountInterface;
use Drupal\og\GroupTypeManager;
use Drupal\og\MembershipManagerInterface;
use Drupal\og\OgMembershipInterface;

/**
* Defines a cache context service, for "membership state" caching.
*
* Cache context ID: 'og_membership_state'
*/
class OgMembershipStateCacheContext implements CacheContextInterface {

/**
* The string to return when no context is found.
*/
const NO_CONTEXT = 'none';


/**
* The group type manager service.
*
* @var \Drupal\og\GroupTypeManager
*/
protected $groupTypeManager;


/**
* The membership manager service.
*
* @var \Drupal\og\MembershipManagerInterface
*/
protected $membershipManager;

/**
* The route match service.
*
* @var \Drupal\Core\Routing\RouteMatchInterface
*/
protected $routeMatch;

/**
* The current user.
*
* @var \Drupal\Core\Session\AccountInterface
*/
protected $user;

/**
* Constructs a new UserCacheContextBase class.
*
* @param \Drupal\Core\Session\AccountInterface $user
* The current user.
* @param \Drupal\Core\Routing\RouteMatchInterface $route_match
* The current route match service.
* @param \Drupal\og\GroupTypeManager $group_type_manager
* The group type manager service.
* @param \Drupal\og\MembershipManagerInterface $membership_manager
* The membership manager service.
*/
public function __construct(AccountInterface $user, RouteMatchInterface $route_match, GroupTypeManager $group_type_manager, MembershipManagerInterface $membership_manager) {
$this->user = $user;
$this->routeMatch = $route_match;
$this->groupTypeManager = $group_type_manager;
$this->membershipManager = $membership_manager;
}

/**
* {@inheritdoc}
*/
public static function getLabel() {
return t('OG membership state');
}

/**
* {@inheritdoc}
*/
public function getContext() {
if (!$route_contexts = $this->routeMatch->getRouteObject()->getOption('parameters')) {
// No "parameters" defined in the route.
return self::NO_CONTEXT;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending directly on the route parameters severely limits the usefulness of this cache context. It will for example not be possible to use this on group content pages, because they won't match the route of the group to which they belong.

An example: in our project all group content has a header that displays the group to which the content belongs. In this header we have some local actions the user can take (for example join the group if they are not a member yet). So we need to vary by membership state cache context on our group content pages.

I am wondering if we can add the current group to the generic route context.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible to add a 'runtimeContext' to the route (ref ContextProviderInterface::getRuntimeContexts()) - here we can dynamically set the right group that matches the current route. It should be possible to do this plugin based like was already being worked on.

We should use this runtimeContext to determine the cache context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the pointer on :: getRuntimeContexts, I'll check it out

It should be possible to do this plugin based like was already being worked on.

My recent thought about OG context is, for the sake of simplicity not port along with Plugins, but rather have a single ContextProvider.

So maybe, In your case, instead of the plugins you could extend the cache context and add your logic. How does it sound?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well now it only works on a very specific subset of routes that follow very specific rules (like being formatted as node/%). By leveraging the OG context it would work out of the box on all routes that have a group associated with them. That sounds more useful to me. It would of course also work on node/%).

I will have to look into this though, I start to understand caching a bit better lately but I am still unsure about some aspects of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, OG context might be needed after all. /cc @RoySegall

On Sep 7, 2016 3:25 PM, "Pieter Frenssen" notifications@github.com wrote:

In src/Cache/Context/OgMembershipStateCacheContext.php
#163 (comment):

  • /**
  • * {@inheritdoc}
  • */
  • public static function getLabel() {
  • return t('OG membership state');
  • }
  • /**
  • * {@inheritdoc}
  • */
  • public function getContext() {
  • if (!$route_contexts = $this->routeMatch->getRouteObject()->getOption('parameters')) {
  •  // No "parameters" defined in the route.
    
  •  return 'none';
    
  • }

Well now it only works on a very specific subset of routes that follow
very specific rules (like being formatted as node/%). By leveraging the
OG context it would work out of the box on all routes that have a group
associated with them. That sounds more useful to me. It would of course
also work on node/%).

I will have to look into this though, I start to understand caching a bit
better lately but I am still unsure about some aspects of it.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/Gizra/og/pull/163/files/ad169ad87998684209857708e306f07c7e3f4f14#r77811136,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAHrC0GOkqYzFCV9gjj8r2ZmHigaKB-Gks5qnq0jgaJpZM4J0nZ-
.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I think the case is simpler. The cache context is added only in the formater of a field attached directly to a group (subscribe to group). So getting the group in this case is tightly coupled with the route, thus og context would not be needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

(Although in general, and not related to this pr, og context might still be needed - not sure)

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This field-specific cache context would work fine for this use case, but it would be obsolete when we get the generic cache context that uses OgContext, since that will also cover its use case. So this implementation is temporary at best.

In our project we now need the generic cache context for a group header that shows some information about the group. This information is different depending on the membership status so we need this cache context.

I can work on this now. I'll have a look at @RoySegall 's ticket, maybe I can help there to move things forward.

Copy link
Member Author

Choose a reason for hiding this comment

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

but it would be obsolete when we get the generic cache context that uses OgContext,

Indeed, I think we can have it as a follow up, once we have OgContextProvider in place


if (!$entity_type_ids = array_keys($this->groupTypeManager->getAllGroupBundles())) {
// No group entities.
return self::NO_CONTEXT;
}

if (!$entity_type_ids = array_intersect(array_keys($route_contexts), $entity_type_ids)) {
// No parameters that match the group entities.
return self::NO_CONTEXT;
}

// Take just the first entity type ID.
$entity_type_id = reset($entity_type_ids);
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work if there are groups using multiple entity types. In our project for example we have groups of type node, but also groups of type rdf_entity.

Copy link
Member Author

Choose a reason for hiding this comment

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

It takes the first entity type ID that is a group (see the array_intersect above)


$group = $this->routeMatch->getParameter($entity_type_id);
$states = [
OgMembershipInterface::STATE_ACTIVE,
OgMembershipInterface::STATE_PENDING,
OgMembershipInterface::STATE_BLOCKED,
];

/** @var OgMembershipInterface $membership */
$membership = $this->membershipManager->getMembership($group, $this->user, $states);
return $membership ? $membership->getState() : self::NO_CONTEXT;
}

/**
* {@inheritdoc}
*/
public function getCacheableMetadata() {
return new CacheableMetadata();
}

}
6 changes: 3 additions & 3 deletions src/GroupContentOperationPermission.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ class GroupContentOperationPermission extends Permission {
/**
* The group content entity type ID to which this permission applies.
*
* @var $string
* @var string
*/
protected $entityType;

/**
* The group content bundle ID to which this permission applies.
*
* @var $string
* @var string
*/
protected $bundle;

Expand All @@ -29,7 +29,7 @@ class GroupContentOperationPermission extends Permission {
*
* Examples: 'create', 'update', 'delete'.
*
* @var $string
* @var string
*/
protected $operation;

Expand Down
6 changes: 3 additions & 3 deletions src/GroupTypeManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ class GroupTypeManager {
* Do not access this property directly, use $this->getGroupRelationMap()
* instead.
*
* @var array
* An associative array representing group and group content relations.
*
* This mapping is in the following format:
* @code
* [
Expand All @@ -104,9 +107,6 @@ class GroupTypeManager {
* ],
* ]
* @endcode
*
* @var array $groupRelationMap
* An associative array representing group and group content relations.
*/
protected $groupRelationMap = [];

Expand Down
2 changes: 1 addition & 1 deletion src/OgAccess.php
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public function userAccess(EntityInterface $group, $operation, AccountInterface
// the minimal caching data.
$config = $this->configFactory->get('og.settings');
$cacheable_metadata = (new CacheableMetadata())
->addCacheableDependency($config);
->addCacheableDependency($config);

if (!$this->groupTypeManager->isGroup($group_type_id, $bundle)) {
// Not a group.
Expand Down
2 changes: 1 addition & 1 deletion src/OgDeleteOrphansInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public function process();
* @param \Drupal\Core\Form\FormStateInterface $form_state
* The current state of the form.
*
* @return array $form
* @return array
* The renderable form array representing the entire configuration form.
*/
public function configurationForm(array $form, FormStateInterface $form_state);
Expand Down
4 changes: 2 additions & 2 deletions src/OgMembershipInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public function setUser(AccountInterface $user);
/**
* Gets the membership's owner.
*
* @return \Drupal\Core\Session\AccountInterface $user
* @return \Drupal\Core\Session\AccountInterface
* The user object referenced by the membership.
*/
public function getUser();
Expand All @@ -101,7 +101,7 @@ public function setGroup(EntityInterface $group);
/**
* Gets the group associated with the membership.
*
* @return \Drupal\Core\Entity\EntityInterface $group
* @return \Drupal\Core\Entity\EntityInterface
* The group object which the membership reference to.
*/
public function getGroup();
Expand Down
6 changes: 2 additions & 4 deletions src/Plugin/Field/FieldFormatter/GroupSubscribeFormatter.php
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,8 @@ public static function create(ContainerInterface $container, array $configuratio
public function viewElements(FieldItemListInterface $items, $langcode) {
$elements = [];

// We cannot use the field cache, as the formatter changes according to the
// user. Furthermore, this is not an expensive check, so we remove the cache
// entirely.
$elements['#cache']['max-age'] = 0;
// Cache by the OG membership state.
$elements['#cache']['contexts'] = ['og_membership_state'];

$group = $items->getEntity();
$entity_type_id = $group->getEntityTypeId();
Expand Down
2 changes: 1 addition & 1 deletion tests/src/Kernel/Entity/SelectionHandlerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class SelectionHandlerTest extends KernelTestBase {
/**
* The selection handler.
*
* @var \Drupal\og\Plugin\EntityReferenceSelection\OgSelection.
* @var \Drupal\og\Plugin\EntityReferenceSelection\OgSelection
*/
protected $selectionHandler;

Expand Down
2 changes: 1 addition & 1 deletion tests/src/Unit/OgAdminRoutesControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ public function testRoutesWithAccess() {
* @param bool $allow_access
* Indicate of access to the routes should be given.
*
* @return array The render array.
* @return array
* The render array.
*/
protected function getRenderElementResult($allow_access) {
Expand Down
Loading