Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added optional param to get friends method to be able to get a number of friends per page #15

Merged
merged 1 commit into from
Mar 6, 2016

Conversation

stephane-monnot
Copy link
Contributor

No description provided.

@stephane-monnot
Copy link
Contributor Author

In the future, I think it will be a good idea if we change the visibility of the "getFriendsQueryBuilder" method to benefit from the power of the Eloquent Builder.

What do you think about this ?

@hootlex
Copy link
Owner

hootlex commented Mar 6, 2016

Sure, it sounds good. It will be useful for extending queries with limit, order, etc.

I was thinking to rewrite the Friendable trait to use scopes in the future.

This way it could be used like this:

$user->friends()->get();
$user->friends()->whereRole('moderator')->get();

$user->pendingFriendships()->get();
$user->pendingFriendships()->limit(10)->get();

But I am not sure if this will be useful or just a waste of time.
What do you think?

hootlex added a commit that referenced this pull request Mar 6, 2016
Added optional param to get friends method to be able to get a number of friends per page
@hootlex hootlex merged commit 3336969 into hootlex:master Mar 6, 2016
@stephane-monnot
Copy link
Contributor Author

It sounds good but it's not backward compatible.

Actually, the friends method returns a morphMany relation of friendship:

return $this->morphMany(Friendship::class, 'sender');

But in your example, it seems you want to use a Friend Model scope:

$user->friends()->whereRole('moderator')->get();

Maybe a new major version is a good idea, isn't it ?

If yes, I imagined a use like this for friendships (pending request) :

// requests from others users than me (incomingScope)
$user->friendships()->incoming(); 

// requests of current users to others users  (outgoingScope)
$user->friendships()->outgoing(); 

I don't know if we need to apply a default filter on status pending to friendships or not. As you said, it could be better to create a pendingFriendships method. A bit too long name, but more explicite. Moreover, we keep the ability to get accepted and blocked friendships.

Or only use scope like this :

// all requests from others users than me (pending/accepted/blocked)
$user->friendships()->incoming(); 

 // requests of current users to others users  (pending/accepted/blocked)
$user->friendships()->outgoing();

 // requests of current users to others users  (only pending)
$user->friendships()->pending()->outgoing();
// just an alias of $user->friendships()->whereStatus(Status::PENDING)->outgoing()

Of course, we can also create pendingFriendships() method, an alias of

$user->friendships()->whereStatus(Status::PENDING);

@hootlex
Copy link
Owner

hootlex commented Mar 6, 2016

Yeah, it should be a major release since it demands a lot of work to make all these changes backwords compatible.

Of course we could make something like

public function getPendingFriendships(){
    return $this->friendships()->pending()->get()
}

but it would be a pain!

In my scenario by $user->friends() I was referring to current $user->getFriendsQueryBuilder() function.

I like your idea. I believe $user->friendships()->pending() is much cleaner than $user->pendingFriendships()

Scopes incoming() and outgoing() seem a bit confusing but may be useful.

I cant decide if this package needs a major release :/

For changes like these it has to be written almost from zero.

@stephane-monnot
Copy link
Contributor Author

The only problem is the friends() method, this may get confused because it will return a friend model instead of a friendship model as now.

Scopes incoming() and outgoing() maybe need a vote. New major release too :) .

@stephane-monnot stephane-monnot deleted the feature/paginateAndLimit branch March 6, 2016 13:45
@hootlex
Copy link
Owner

hootlex commented Mar 6, 2016

I think friends will be less confusing this way.

//returns friendships
$user->friendships()->whatever()->get(); 

//returns friends
$user->friends()->get(); 

This package is too small to make a vote yet.
We have to decide :)

@hootlex
Copy link
Owner

hootlex commented Mar 7, 2016

What do you say about accepting/denying friend requests?

//current way
$user->acceptFriendRequest($friend);
$user->denyFriendRequest($friend);


//possible new way
$user->friendships()->accept($friend);
$user->friendships()->deny($friend);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants