Incorrect return in ElggRelationship::save() #10373

Closed
hypeJunction opened this Issue Oct 6, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@hypeJunction
Contributor

hypeJunction commented Oct 6, 2016

ID is expected and bool is returned

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Oct 6, 2016

Member

Maybe we should remove the feature like @mrclay suggested in #5990

Member

jdalsem commented Oct 6, 2016

Maybe we should remove the feature like @mrclay suggested in #5990

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Oct 6, 2016

Contributor

Fine by me. But it would make sense for add_entity_relationship() to return an id.

Contributor

hypeJunction commented Oct 6, 2016

Fine by me. But it would make sense for add_entity_relationship() to return an id.

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Oct 6, 2016

Member

Probably a dupe of #9019 then

Member

jdalsem commented Oct 6, 2016

Probably a dupe of #9019 then

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 6, 2016

Member

Different issue. #9019 is about incorrect usage, this is wrong return type.

Member

mrclay commented Oct 6, 2016

Different issue. #9019 is about incorrect usage, this is wrong return type.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 6, 2016

Member

This was broken in 1.0. I suggest we just fix the phpdocs in 1.12 and fix in master.

Member

mrclay commented Oct 6, 2016

This was broken in 1.0. I suggest we just fix the phpdocs in 1.12 and fix in master.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 6, 2016

Member

On 2nd thought, this is a real issue for the internals of ElggRelationship::save

Member

mrclay commented Oct 6, 2016

On 2nd thought, this is a real issue for the internals of ElggRelationship::save

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Oct 6, 2016

Contributor

I think out API and tests assume that all ::save methods return an id on success.

Contributor

hypeJunction commented Oct 6, 2016

I think out API and tests assume that all ::save methods return an id on success.

@mrclay

This comment has been minimized.

Show comment
Hide comment
Member

mrclay commented Oct 6, 2016

PR #10375

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