-
Notifications
You must be signed in to change notification settings - Fork 133
Remove OG complex widget #181
Changes from 152 commits
a658b0e
bce0c5e
bad2c09
c75b654
d49ac2b
1d061ee
d8591fb
97fb566
77eb562
9bb346e
ce13e41
d317614
0e10e5f
793715e
3d4013f
9c4858b
5346d09
1551922
3f69920
ca3bc5d
3e3c90d
78a427e
11f4bd2
8c94013
e98354c
1508540
afd3f71
586e677
c5fdf87
328c913
68a9abd
a117c7b
0940d99
521b508
ef4af0d
d5d34d6
7334e9c
9ac5241
71e903a
00525d9
a60a300
cb75f16
f7b0d29
43f8acd
cf30f3a
94db0c1
a2bdbab
e0c2655
ba47934
4d65046
2104177
4f47cc0
a988745
470e6ef
5c939ec
0fcf95e
81f1b28
f33eb3d
7762e4f
3a1929b
8f241b2
1b18142
1d5b92d
ef4d2db
8dc9aa1
d817fc4
d64af00
e2e82b9
ee625d3
2fdf2bd
a21eb83
85849ae
a1cc01e
7a18d5c
780b54a
352551d
7fafaef
093324c
8aa95f5
24a1221
6bdaa20
b174e23
4bb4819
0741340
ae94377
12838fb
8ab5433
55b3fcf
a2271be
7744bc3
9f96bc4
7ecdedd
86dee21
f20e279
3134064
8abc283
8c7cadb
564682f
a2d92a2
74e677d
2c5af11
df17662
779efee
1450634
961eb7d
0e030db
15debda
f43add7
2257ece
35b019f
12cc927
365da2f
f3894cd
16ff320
b7e2d5e
b7ac85b
3493f91
3439183
fd83a5d
c304aea
0625119
5fa4b7a
352ba22
57dfc44
fab2fe6
3d3365a
2faaad5
214f5cd
9c94943
84a85fc
c194f0e
e5b3b60
fde09bc
86301b0
5622d85
85d87ae
e9e2152
991ea33
d6ceee6
7e58f33
ffd5afe
412f880
aa82d18
34ffca4
bdc92db
417d548
0f87dbe
c980ace
38a25a7
d0b227f
6f6a733
fd95fdb
0439c0e
eff1923
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,6 +123,7 @@ function og_entity_delete(EntityInterface $entity) { | |
* Implements hook_entity_access(). | ||
*/ | ||
function og_entity_access(EntityInterface $entity, $operation, AccountInterface $account) { | ||
|
||
// Grant access to view roles, so that they can be shown in listings. | ||
if ($entity instanceof OgRoleInterface && $operation == 'view') { | ||
return AccessResult::allowed(); | ||
|
@@ -159,6 +160,21 @@ function og_entity_access(EntityInterface $entity, $operation, AccountInterface | |
/** @var \Drupal\Core\Access\AccessResult $access */ | ||
$access = \Drupal::service('og.access')->userAccessEntity($operation, $entity, $account); | ||
|
||
$audience_fields = \Drupal::service('og.group_audience_helper')->getAllGroupAudienceFields($entity_type_id, $bundle_id); | ||
if (count($audience_fields) === 1) { | ||
$field_definitions = \Drupal::service('entity_field.manager')->getFieldDefinitions($entity_type_id, $bundle_id); | ||
foreach ($field_definitions as $field_name => $field_definition) { | ||
|
||
/** @var \Drupal\Core\Field\FieldDefinitionInterface $field_definition */ | ||
if (!in_array($field_definition->getName(), array_keys($audience_fields))) { | ||
continue; | ||
} | ||
|
||
// Check if the field needs to be required. | ||
$field_definition->setRequired($access); | ||
} | ||
} | ||
|
||
if ($access->isAllowed()) { | ||
return $access; | ||
} | ||
|
@@ -179,6 +195,11 @@ function og_entity_access(EntityInterface $entity, $operation, AccountInterface | |
function og_entity_create_access(AccountInterface $account, array $context, $bundle) { | ||
$entity_type_id = $context['entity_type_id']; | ||
|
||
if (!empty($context['skip_og_permission'])) { | ||
// We've already been here or we want to skip the access check. | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would try to see if it is possible to find a solution that avoids adding this section. Since this hook is dealing with checking access it is important for security reasons and introducing a way to skip the checks might have unintended consequences. It is also not allowed to return |
||
} | ||
|
||
if (!Og::isGroupContent($entity_type_id, $bundle)) { | ||
// Not a group content. | ||
return AccessResult::neutral(); | ||
|
@@ -203,17 +224,32 @@ function og_entity_create_access(AccountInterface $account, array $context, $bun | |
// @see \Drupal\og\Plugin\EntityReferenceSelection\OgSelection::buildEntityQuery() | ||
$required = FALSE; | ||
|
||
/** @var \Drupal\og\OgGroupAudienceHelperInterface $audience_helper */ | ||
$audience_helper = \Drupal::service('og.group_audience_helper'); | ||
$audience_fields = $audience_helper->getAllGroupAudienceFields($entity_type_id, $bundle); | ||
$field_definitions = \Drupal::service('entity_field.manager')->getFieldDefinitions($entity_type_id, $bundle); | ||
|
||
foreach ($field_definitions as $field_name => $field_definition) { | ||
/** @var \Drupal\Core\Field\FieldDefinitionInterface $field_definition */ | ||
if (!\Drupal::service('og.group_audience_helper')->isGroupAudienceField($field_definition)) { | ||
if (!in_array($field_definition->getName(), array_keys($audience_fields))) { | ||
continue; | ||
} | ||
|
||
$handler = Og::getSelectionHandler($field_definition); | ||
if (count($audience_fields) === 1) { | ||
// Set the field to required only if we have a single audience field. | ||
// If we have multiple fields then the require logic will perform at the | ||
// entity constraint level. | ||
$field_definition->setRequired(!\Drupal::entityTypeManager() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the change made to |
||
->getAccessControlHandler($entity_type_id) | ||
->createAccess($bundle, $account, ['skip_og_permission' => TRUE])); | ||
} | ||
|
||
if ($handler->getReferenceableEntities()) { | ||
return AccessResult::neutral(); | ||
$handler = Og::getSelectionHandler($field_definition); | ||
if ($handler->countReferenceableEntities()) { | ||
// At least one of the fields has referenceable groups. That mean that | ||
// the user can create group content when referencing the group content in | ||
// at least one of the fields. | ||
return AccessResult::allowed(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the user gets access based on OG, and doesn't have site wide permission to create content, we need to set the audience field to be required. Things become more complicated when we might have multiple audience fields :/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could form_alter() and if no group was selected, fail the form. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two ideas for simplification, that we need to think about:
(1) Is removing some advanced features, which I have honestly never seen used, like /cc @pfrenssen @RoySegall There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thought about it more. (1) sounds like going back to D6, but in a bad way. (2) sounds ok, but we won't gain much. In short, I think we should keep those features in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @RoySegall is there a test to validate:
|
||
} | ||
|
||
// Allow users to create content outside of groups, if none of the | ||
|
@@ -284,7 +320,16 @@ function og_field_formatter_info_alter(array &$info) { | |
* Implements hook_field_widget_info_alter(). | ||
*/ | ||
function og_field_widget_info_alter(array &$info) { | ||
$info['options_buttons']['field_types'][] = OgGroupAudienceHelperInterface::GROUP_REFERENCE; | ||
$field_types = [ | ||
'entity_reference_autocomplete', | ||
'entity_reference_autocomplete_tags', | ||
'options_buttons', | ||
'options_select', | ||
]; | ||
|
||
foreach ($field_types as $field_type) { | ||
$info[$field_type]['field_types'][] = OgGroupAudienceHelperInterface::GROUP_REFERENCE; | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -297,9 +342,24 @@ function og_field_widget_info_alter(array &$info) { | |
* be rebuilt via RouteBuilder::setRebuildNeeded. | ||
*/ | ||
function og_entity_type_alter(array &$entity_types) { | ||
|
||
/** @var \Drupal\Core\Entity\EntityTypeInterface $entity_type */ | ||
foreach ($entity_types as $entity_type_id => $entity_type) { | ||
|
||
// Add link template to groups. We add it to all the entity types, and later | ||
// on return the correct access, depending if the bundle is indeed a group | ||
// and accessible. We do not filter here the entity type by groups, so | ||
// whenever GroupTypeManager::addGroup is called, it's enough to mark route | ||
// to be rebuilt via RouteBuilder::setRebuildNeeded. | ||
$entity_type->setLinkTemplate('og-admin-routes', "/group/$entity_type_id/{{$entity_type_id}}/admin"); | ||
|
||
// We won't check if an entity might be group content because we need to | ||
// check if that entity has any bundle that could be a group entity and this | ||
// would cause a recursion. The constraint will check there if the entity is | ||
// group content or not. | ||
if ($entity_type->entityClassImplements(ContentEntityInterface::class)) { | ||
$entity_type->addConstraint('ValidOgMembershipMultipleReference'); | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -191,8 +191,22 @@ public function getGroupBundleIdsByEntityType($entity_type_id) { | |
* {@inheritdoc} | ||
*/ | ||
public function getAllGroupBundles($entity_type = NULL) { | ||
// Todo - should be remove since this method don't do any thing. | ||
return $this->getGroupMap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's undo this change, it's out of scope for this issue and it causes a fatal error because now the signature is different from the one defined in There is already an issue to remove this method: #372, let's do it in there. |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't make sense to me, why have a method that doesn't do anything but wrap another method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, should be removed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add todo for that one. |
||
|
||
/** | ||
* Get group bundles of an entity type. | ||
* | ||
* @param string $entity_type_id | ||
* The entity type ID. | ||
* | ||
* @return array | ||
* An associative array of bundle IDs, or an empty array if none found. | ||
*/ | ||
public function getGroupBundlesByEntityType($entity_type_id) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove this method, it exactly duplicates the code from |
||
$group_map = $this->getGroupMap(); | ||
return !empty($group_map[$entity_type]) ? $group_map[$entity_type] : $group_map; | ||
return isset($group_map[$entity_type_id]) ? $group_map[$entity_type_id] : []; | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no documentation for this change, but from reading the code it seems that it is doing the following:
If this code is intended to make a field required or not it does not belong in
hook_entity_access()
. This hook is intended for returning a boolean to indicate whether the user has access. It is not a good idea to hijjack it to make changes to fields. This should be moved to a more relevant location.On top of the code being in the wrong place, we should add some documentation to it. From reading the code it is unclear why this is needed. Why does the field need to become required when the user has access? Why is this only needed when there is 1 single field, why not when there are multiple fields?
The reasoning behind this is explained by the test
OgSelectionWidgetOptionsTest::testNonRequiredAudienceField()
:So apparently there are certain cases where a user is not allowed to post site wide content, and then the group audience field should become required. Let's document this clearly, and also move this code out of
hook_entity_access()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pfrenssen, I would suggest a custom constant or would you prefer a form_alter?