This repository has been archived by the owner on Aug 18, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
160 remove complex widget #570
160 remove complex widget #570
Changes from 157 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
40f677f
0e7bec1
037c38f
7bda98a
cdde6cd
bd428c6
5db820b
97623d4
c6559a7
6630f00
7b8245a
0076fba
8a35d7a
1ba32f8
68cd03e
fa93ef0
58caa78
29cc980
492825a
9884218
6409300
72fb63d
f8cbf12
e3fd886
7c591a4
79519fc
6236767
e1ce4b0
61967f9
f270eb2
b4be10f
9cc2202
1ea6272
e90010b
1e8d004
17f9b08
f87e4e5
9b2aaae
79931c4
e4f9334
a0c1e6d
3926543
146c778
748e78d
0bd6cc1
e80f686
5a5995a
d32da0b
7c3ce1b
16aad07
fc73c8e
75e3def
5d95ec6
528025d
2b43dec
6951d09
753e719
f1adcc7
1bb4998
437bb31
6494ed3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@MPParsley this one is related to https://github.com/Gizra/og/pull/570/files#r377001979
We try to make sure that if a node must be created inside an OG context, it cannot be created without choosing a group. Again not sure about the correctness of code - but rather on the reason.
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.
I think this code is not working. This is calling
FieldConfigInterface::setRequired()
but never callingFieldConfigInterface::save()
after making the change so this code is basically doing nothing.We should not alter field definitions in
hook_entity_access()
in any case.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.
What is the purpose of this change? It is not fully clear to me since this is not documented.
It looks like we are doing some checks and then set a field to be required. This is probably not the right place to do this. Since this is a
hook_entity_access()
implementation we should only have code here that is necessary for determining theAccessResult
object to return. We should not mess with field definitions here.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.
@amitaibu, can you confirm this or provide some more information where this comes from?
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.
Why is this necessary? This seems brittle to me and a potential security problem. We should not allow to skip access checks unless there is a very good reason for it.
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.
Not sure about the correctness of the code here - but I remember the reason though. When you create a node, it still doesn't belong to a group. It might belong, but if for example the audience field is optional, than that node could be a "global" one (i.e. does not belong to a group).
So we need to check if that node could be created outside of OG context. If it can, then we don't have the final say.
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.
It looks like this is intended to avoid an endless loop, because this is calling itself again later on.
This approach feels very strange to me. The code below calls the access check, passing exactly the same parameters, from inside the access check? What information could this possibly be retrieving that we don't already have at this point.
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.
Why are we again setting fields to be required inside an access hook? This doesn't belong here.
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.
I believe it's related to https://github.com/Gizra/og/pull/570/files#r377001979
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.
We should investigate a different solution, this is a hack. We should not alter any field definitions in
hook_entity_access()
.It looks like this field definition is used as temporary storage for what could be a simple
$is_required
flag.