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

Aggregate saves #6437

Closed
kirkbushell opened this issue Nov 19, 2014 · 24 comments
Closed

Aggregate saves #6437

kirkbushell opened this issue Nov 19, 2014 · 24 comments

Comments

@kirkbushell
Copy link
Contributor

This is both an "issue" and discussion piece.

It would be nice if we were able to save aggregates via their parents/children.etc when dealing with hasMany/belongsTo and other assorted relationships. Eg.

$user = new User(['email' => $email]);
$post = new Post(['title' => 'Some new post by a user']);

$user->posts()->insert($post);
$user->save();

It would be nice if this behaviour could have a waterfall effect whereby it saves the user, then the post.etc. I don't see this as particularly more complex than the current arrangement, but wondering if maybe there could be a block preventing this from happening?

In addition, it should work the other way:

$post = new Post(['title' => 'Some new post by a user']);
$user = new User(['email' => $email]);

$post->author()->associate($user);
$post->save();

This would enable us to setup all sorts of related objects and relate them before doing a final save, which would then do the trickle-down effect. It also has a rather nice side-effect of cleaning up the code.

In terms of adding this functionality, I'm wondering if, when setting up related objects - if no otherKey is available, perhaps it could defer the save until the parent/child save method is called, which could then cycle through the related objects, checking for deferred saves and save those when possible?

@jonathanpmartins
Copy link

Nice proposal of the second one! Is the first example related to this issue? #6440

@kirkbushell
Copy link
Contributor Author

Na separate issue. I responded to #6440 as well. In that case it should just be working - in the proposal above, it actually requires a possible architectural shift and/or change to how related objects are saved. Aka, there's no easy solution.

@jonathanpmartins
Copy link

Sorry, noob here! Hungry for knowledge! hehehehe

@GrahamCampbell
Copy link
Member

I'm just going to comment here to attach myself to this issue so github will email me as this progresses. :)

@kirkbushell
Copy link
Contributor Author

I've done a bit of playing around with some solutions to this and it does require some additional knowledge in parent/child of deferred saves. No solution as yet, but working on it. Current experiments are messy and just don't feel "laravel" for PR.

The problem is that the models have no idea what the relationships actually are. It's proving to be a pain.

@boedy
Copy link

boedy commented Nov 24, 2014

+1 I also want this. Maybe just because this is usually the way I store things when I'm not using a framework like Laravel. I've been googling allot trying to describe the behaviour you're describing, but I mostly come up empty handed. Some posts (including one of mine) that I think are kind of related are:

http://stackoverflow.com/questions/26225034/hasmany-relationship-attach-model-without-saving-directly
http://stackoverflow.com/questions/23951222/eloquent-relations-attach-but-dont-save-to-has-many

The way I currently solve it is by overriding the save method and doing all the persistence logic there.

@kirkbushell
Copy link
Contributor Author

That's certainly one way to do it, but having it managed in an autonomous way with the current system is extremely hard work. The main problem is that the relationships a model has simply aren't known to the model, until they're loaded.

One way is to look at all methods defined on the model, and figure out which ones are relationships (by actually executing them). That's a dangerous approach, so I think we can rule that one out.

Another is to define the relationships as part of an array on the model, as at least some sort of cache that we can query to then check related models that have been created. This is extra work however on the part of the developer that I'm not particularly interested in, but it's certainly doable.

The last solution is to change how relationships work, but I can't see this as feasible given how engrained the current Eloquent API is. #2 certainly looks like the easiest solution, with Eloquent not supported deferred saves for aggregates unless some sort of relationship registry is defined on the model. It would be something like:

$related = [
    'hasMany' => ['User'],
    'belongsTo' => ['Post', 'Category']
];

Or something of that nature at least. It's not particularly ugly, but again it's an extra step that is required on the part of the developer. in short, I don't think this will ever make it into the core without a huge change to how Eloquent relationships are defined.

@Edgpaez
Copy link
Contributor

Edgpaez commented Nov 24, 2014

I find it rather misleading that calling the save method on a Userwould also save all of its related comments. I think calling a saveWithRelated (for lack of a better name) would be a little bit more verbose but much clearer. What do you think?

This is my idea, the code would look something like this:

$user = new User(['email' => $email]);
$post = new Post(['title' => 'Some new post by a user']);

$user->posts()->associate($post);
$user->saveWithRelated();
  1. The hasMany,hasOne, etc methods in Eloquent\Model register the relation in an array inside the Model instance before returning it.
  2. The associate method in the relation wouldn't save the records but 'attach' them somehow (similar to what associate method does in BelongsTo).
  3. In the saveWithRelated method in Eloquent\Model you would iterate the array you filled in step 1 and send all the registered relations something like: $relation->saveAssociatedModels()
  4. The saveAssociatedModels method in the relation would save all the 'unsaved' models

What do you guys think about this?

@kirkbushell
Copy link
Contributor Author

@Edgpaez this is the normal behaviour once a user is saved, in some related cases (such as belongsTo). It's actually more consistent that this be the behaviour for all relationships, including deferred saves. It should be noted however, that it wouldn't save all related ones - only ones that need to be saved. In a way I guess it'd be a helper method - but it's also rather normal for an ActiveRecord implementation to have such a feature.

Your proposed solution is pretty much how I've been going about it so far, but again - I don't think it would make it into core and probably something that would have to remain as part of a separate package. Maybe if we can get more support for this idea it could be baked into core as a separate feature, in which case - saveWithRelated may be the preferred solution.

@boedy
Copy link

boedy commented Nov 25, 2014

I'm not an experienced Laravel developer, but what about something in the lines of this:

Create a helper function in a model base class like this:

use Illuminate\Database\Eloquent\Model;

class BaseModel extends Model
{
    public function save(array $options = array()) {
        //Save parent
        parent::save($options);

        if ( ! in_array('withRelations', $options)) return;

        //loop relationships
        foreach ($this->getRelations() as $relation => $collection) {
            if ( ! sizeof($collection)) continue;

            foreach ($collection as $item) {
                if ( ! is_callable(array($this, $relation))) continue;

                //save child
                if (is_callable(array($this->$relation, 'save'))) {
                    $this->$relation->save();
                } else {
                    $this->$relation()->save($item);
                }
            }
        }
    }
} 

This could then be use as follows in the rest of your application:

$user = new User(['email' => $email]);
$post = new Post(['title' => 'Some new post by a user']);

$user->posts->add($post);
$user->save();

I haven't tested this thoroughly, but what are your thoughts?

@kirkbushell
Copy link
Contributor Author

getRelations only works on relationships that are already loaded - aka, the array/object has been populated.

@boedy
Copy link

boedy commented Nov 25, 2014

Yeah so this would work then right? It works for me. I've updated it a bit so that it works with belongsTo. I must admit it seems hacky, because it overrides the save method. But something like this would work:

$user = new User(['email' => $email]);
$post = new Post(['title' => 'Some new post by a user']);
$author = new Author(['name' => 'John Doe']);

$post->author()->associate($author);
$user->posts->add($post);
$user->save();

@kirkbushell
Copy link
Contributor Author

It's no good if it only works for belongsTo.

@boedy
Copy link

boedy commented Nov 25, 2014

It works with hasMany and belongsTo as you described in your first post.

@kirkbushell
Copy link
Contributor Author

I haven't been able to get hasMany working unless the relationships are loaded. belongsTo is easy because in the model you'er working with, the field is actually on that model.

@jonathanpmartins
Copy link

Like to help but I only have the weekends!

@boedy
Copy link

boedy commented Nov 25, 2014

@kirkbushell I'm not following you. What's the difference between our examples? It also works for belongsToMany relations.

    $playlist = new \models\Playlist();
    $playlist->setName('My test playlist');

    $track1 = new \models\Track(['title' => 'laravel track1']);
    $track2 = new \models\Track(['title' => 'laravel track2']);

    $playlist->tracks->add($track1);
    $playlist->tracks->add($track2);

    $playlist->save(['withRelations']);

Have you tried my code?

@kirkbushell
Copy link
Contributor Author

I'm not saying the exalts are different, I'm saying said code won't work for all relationships. My tests only had it working for belongsTo relationships.

@kirkbushell
Copy link
Contributor Author

Looking for a syntactically clean and elegant solution. Overwriting save, as you pointed out, isn't the answer.

@boedy
Copy link

boedy commented Nov 25, 2014

I'll stick with this for now until someone comes up with a better solution. Please care to share if you do :). You could add optional parameters to the save method if you do choose to go that route (save(['withRelations']).

@jarektkaczyk
Copy link
Contributor

This one is very tricky, especially if you want it recursive and don't want to (re)write hell of a functionality. That's because Eloquent requires parent model to be saved before saving children models.

Like with push, this won't work (as expected):

$user = new User;
$post = new Post;
$post->user()->associate($user);
$post->push();

@mattzuba
Copy link

Here's how I solved this...

http://laravel.io/bin/vBn8v

As far as constructing the item, looks like so:

<?php

    $uploader = new \models\track\Uploader();
    $uploader->user_id = 29766160;
    $uploader->name = 'Flume';
    $uploader->username = 'flume';
    $uploader->avatar = 'test';

    $source = new \models\track\sources\SoundCloud();
    $source->source_id = 123456789;
    $source->source_type = 'tracks';

    //belongsTo
    $source->uploader = $uploader;
    $track = new \models\Track();
    $track->title = 'test';

    //hasMany
    $track->sources[] = $source;
    $track->push();

@boedy
Copy link

boedy commented Dec 11, 2014

@mattzuba how are hasOne relationships handled? I get an "Unknown column" error when saving the parent.

example:

//hasOne wil result in ( Unknown column 'stats' in 'field list' (SQL: insert into `tracks`.. )
$track->stats = new \models\track\Stats();
$track->push();

@mattzuba
Copy link

@boedy Check out the setAttribute method adjustment in this paste and see if that fixes it for you:

http://laravel.io/bin/BL9jz

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

No branches or pull requests

7 participants