Cannot prevent relationships from being created #8927

Closed
beck24 opened this Issue Sep 13, 2015 · 10 comments

Comments

Projects
None yet
3 participants
@beck24
Member

beck24 commented Sep 13, 2015

Since 1.9 it appears you cannot prevent relationships from being created by using an event handler.

The relevant code is at
https://github.com/Elgg/Elgg/blob/1.9/engine/lib/relationships.php#L119
https://github.com/Elgg/Elgg/blob/1.12/engine/classes/Elgg/Database/RelationshipsTable.php#L127
https://github.com/Elgg/Elgg/blob/2.x/engine/classes/Elgg/Database/RelationshipsTable.php#L120

The result of the event is boolean, but it passes that value into delete_relationship if the return is false. The delete_relationship function expects the id of a relationship.
I'm either missing something or this is probably breaking a lot of things (and making my plugin upgrades impossible)

@juho-jaakkola

This comment has been minimized.

Show comment
Hide comment
@juho-jaakkola

juho-jaakkola Sep 13, 2015

Member

We can fix the next Elgg 1.12.x to pass in the id instead of the boolean, right?

Member

juho-jaakkola commented Sep 13, 2015

We can fix the next Elgg 1.12.x to pass in the id instead of the boolean, right?

@beck24

This comment has been minimized.

Show comment
Hide comment
@beck24

beck24 Sep 13, 2015

Member

Yes, but that means version 1.9 - 1.11 are inherently broken - I feel like this couldn't have gone undiscovered this long - has nobody tried to prevent someone from joining a group or becoming friends?

Member

beck24 commented Sep 13, 2015

Yes, but that means version 1.9 - 1.11 are inherently broken - I feel like this couldn't have gone undiscovered this long - has nobody tried to prevent someone from joining a group or becoming friends?

beck24 added a commit to beck24/Elgg that referenced this issue Sep 13, 2015

fix(relationships): can now prevent relationships using event handler
Passes the relationship id into delete_relationship() instead of the event handler return

Fixes #8927
@beck24

This comment has been minimized.

Show comment
Hide comment
@beck24

beck24 Sep 13, 2015

Member

todo - are there no tests for this?

Member

beck24 commented Sep 13, 2015

todo - are there no tests for this?

@juho-jaakkola

This comment has been minimized.

Show comment
Hide comment
@juho-jaakkola

juho-jaakkola Sep 13, 2015

Member

that means version 1.9 - 1.11 are inherently broken

Well yes, but I don't see why we would need to add the fix to them when its so easy to just upgrade to 1.12.x.

Member

juho-jaakkola commented Sep 13, 2015

that means version 1.9 - 1.11 are inherently broken

Well yes, but I don't see why we would need to add the fix to them when its so easy to just upgrade to 1.12.x.

@beck24

This comment has been minimized.

Show comment
Hide comment
@beck24

beck24 Sep 14, 2015

Member

My own selfish reasons - I'm upgrading plugins for a university that works at the pace of an inefficient government and I'm under contract to give them 1.9 AND 1.12 compatible plugins, which is currently impossible :(

Member

beck24 commented Sep 14, 2015

My own selfish reasons - I'm upgrading plugins for a university that works at the pace of an inefficient government and I'm under contract to give them 1.9 AND 1.12 compatible plugins, which is currently impossible :(

beck24 added a commit to beck24/Elgg that referenced this issue Sep 14, 2015

fix(relationships): can now prevent relationships using event handler
Passes the relationship id into delete_relationship() instead of the event handler return

Fixes #8927
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Sep 14, 2015

Member

For broken versions you could prevent these actions via removing UI elements, and/or intercept the action URLs.

Yeah, a bummer we're missing tests here.

Member

mrclay commented Sep 14, 2015

For broken versions you could prevent these actions via removing UI elements, and/or intercept the action URLs.

Yeah, a bummer we're missing tests here.

@mrclay

This comment has been minimized.

Show comment
Hide comment
Member

mrclay commented Sep 15, 2015

PR #8928

beck24 added a commit to beck24/Elgg that referenced this issue Sep 15, 2015

fix(relationships): can now prevent relationships using event handler
Passes the relationship id into delete_relationship() instead of the event handler return
Note: Previous versions of Elgg 1.9 - 1.12.3 have broken relationship event handlers

Fixes #8927

beck24 added a commit to beck24/Elgg that referenced this issue Sep 15, 2015

fix(relationships): can now prevent relationships using event handler
Passes the relationship id into delete_relationship() instead of the event handler return

Fixes #8927
@beck24

This comment has been minimized.

Show comment
Hide comment
@beck24

beck24 Sep 15, 2015

Member

Unfortunately it's not quite that easy, some of the UI elements are bulk user management tools where the event is necessary to stop someone slipping an unauthorized person into a group with the batch. Still need the batch UI available, need to let in the 14 authorized people and keep the 1 unauthorized person out. Action intercepts can potentially work, but there's so many to worry about across plugins on a heavily modified site. This event was the gatekeeper... (well the 'join', 'group' event was, but we apparently got rid of that as a useable prevention method)

Member

beck24 commented Sep 15, 2015

Unfortunately it's not quite that easy, some of the UI elements are bulk user management tools where the event is necessary to stop someone slipping an unauthorized person into a group with the batch. Still need the batch UI available, need to let in the 14 authorized people and keep the 1 unauthorized person out. Action intercepts can potentially work, but there's so many to worry about across plugins on a heavily modified site. This event was the gatekeeper... (well the 'join', 'group' event was, but we apparently got rid of that as a useable prevention method)

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Sep 15, 2015

Member

Workaround: manually delete the unwanted relationship before returning false (or schedule it to be deleted in shutdown). Not ideal, but neither is running unsupported software.

Member

mrclay commented Sep 15, 2015

Workaround: manually delete the unwanted relationship before returning false (or schedule it to be deleted in shutdown). Not ideal, but neither is running unsupported software.

@beck24

This comment has been minimized.

Show comment
Hide comment
@beck24

beck24 Sep 15, 2015

Member

Nice, that's a workaround that should work - thanks for the idea!

Member

beck24 commented Sep 15, 2015

Nice, that's a workaround that should work - thanks for the idea!

@beck24 beck24 closed this in 9a275d9 Sep 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment