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

[5.5] Add closure-based relationship setup #18313

Closed
wants to merge 9 commits into from
Closed

[5.5] Add closure-based relationship setup #18313

wants to merge 9 commits into from

Conversation

KKSzymanowski
Copy link
Contributor

This allows for explicit and elegant setup of relationships:
Instead of

$this->belongsToMany(Foo::class, 'table', 'foreignKey', 'relatedKey', 'parentKey', 'localKey', 'relation');

you can say:

$this->belongsToMany(Foo::class, function ($relationship) {
    $relationship->table('table');
                 ->foreignKey('foreignKey')
                 ->relatedKey('relatedKey')
                 ->parentKey('parentKey')
                 ->localKey('localKey')
                 ->relation('relation');
});

This is most useful if you need to overwrite one default value:
Instead of

$this->belongsToMany(Foo::class, null, null, null, null, null, 'relation');

you can say:

$this->belongsToMany(Foo::class, function ($relationship) {
    $relationship->relation('relation');
});

This functionality is available for all relationships:

  • hasOne
  • morphOne
  • belongsTo
  • morphTo
  • hasMany
  • hasManyThrough
  • morphMany
  • belongsToMany
  • morphToMany
  • morphedByMany

The convention is, the first optional parameter of a method(from the list above) can be a Closure and if it is, the values set up in it are used as if they were original function arguments. If the first optional argument is a Closure, all succeeding are ignored. If, in a Closure, a value is not specified, it is treated as null for further processing which is equivalent to not providing the argument in the old relationship setup process.

@KKSzymanowski KKSzymanowski changed the title [5.5] Add closure-base relationship setup [5.5] Add closure-based relationship setup Mar 13, 2017
@taylorotwell
Copy link
Member

Your Setter class probably isn't needed. You could probably just use Illuminate\Support\Fluent.

@KKSzymanowski
Copy link
Contributor Author

KKSzymanowski commented Mar 14, 2017

@taylorotwell I thought about that, but with Fluent I cannot enforce correct parameter names. If user makes a typo he won't be notified of it right away and receive unexpected behaviour sometime later.
If there was some kind of method autocompletion, this aspect would be negligible, but there isn't a way to do this as far as I know.

Do you think this kind of enforcement isn't necessary?
If it is, I can reuse Fluent by creating its strict variant and overwriting __call and __set methods to only work when the key in $attributes already exist.

@taylorotwell
Copy link
Member

Yeah you could perhaps extend Fluent or something.

@KKSzymanowski
Copy link
Contributor Author

@taylorotwell How does it look like now?

@JosephSilber
Copy link
Member

Why aren't the setters directly on the relationship class? Why do we need the closure?

$this->belongsToMany(Foo::class)->foreignKey('...')->localKey('...');

@KKSzymanowski
Copy link
Contributor Author

@JosephSilber Because for example in BelongsToMany joins and wheres are added upon instantiation so they are ready to be used. If you'd want to set these values after creating a relationship you'd need to essentially rebuild the whole relationship.

That's at least how I understand it, correct me if I'm wrong.

@thecrypticace
Copy link
Contributor

That is correct. I think being able to build relationship queries lazily would be useful. I've had cases where I wanted to slightly modify a relationship query but can't because something has already been added that I want to remove. Changing that might be non-trivial though due to the way Relation::noConstraints is intended to work.

@taylorotwell
Copy link
Member

I made some changes to belongs to many and morph to many. Can you update for that? #18380

@KKSzymanowski
Copy link
Contributor Author

Sure, I'll have it done by tomorrow morning UTC, because I don't have access to my PC right now.

@KKSzymanowski
Copy link
Contributor Author

KKSzymanowski commented Mar 17, 2017

@taylorotwell Travis builds are failing because of some dependency incompatibilities. It's not my fault, is it?

@taylorotwell
Copy link
Member

I kind of agree with Joseph that we may want to eventually pursue just chaining these methods onto the relationship itself somehow. I think that is the ideal end goal.

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.

4 participants