-
Notifications
You must be signed in to change notification settings - Fork 133
Remove OG complex widget #181
Changes from 96 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 |
---|---|---|
|
@@ -143,6 +143,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 already been here or want to skip the access checking. | ||
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(); | ||
|
@@ -174,10 +179,16 @@ function og_entity_create_access(AccountInterface $account, array $context, $bun | |
continue; | ||
} | ||
|
||
$handler = Og::getSelectionHandler($field_definition); | ||
// Check if the field need to be required. | ||
// todo: check how this affect rest request and check if we have two fields. | ||
$field_definition->setRequired(!\Drupal::entityTypeManager() | ||
->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()) { | ||
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 | ||
|
@@ -248,7 +259,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; | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -261,6 +281,7 @@ function og_field_widget_info_alter(array &$info) { | |
* via RouteBuilder::setRebuildNeeded. | ||
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 keep this documentation here so that people that end up here know what is being altered in this hook implementation without having to read the code. |
||
*/ | ||
function og_entity_type_alter(array &$entity_types) { | ||
|
||
/** @var \Drupal\Core\Entity\EntityTypeInterface $entity_type */ | ||
foreach ($entity_types as $entity_type_id => $entity_type) { | ||
$entity_type->setLinkTemplate('og-admin-routes', "/group/$entity_type_id/{{$entity_type_id}}/admin"); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -227,9 +227,22 @@ public function getGroupsForEntityType($entity_type_id) { | |
* An associative array, keyed by entity type, each value an indexed array | ||
* of bundle IDs. | ||
*/ | ||
public function getAllGroupBundles($entity_type = NULL) { | ||
public function getAllGroupBundles() { | ||
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.
Fix grammar: "We've already been here or we want to skip the access check."
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.
This change must be removed. There are no good reasons to ever skip access checking, introducing this is very risky since it opens the door for access vulnerabilities. Also by returning
NULL
this violates the API which requires anAccessResult
to be returned.