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.8] Pivot models provided by using() should use model methods for write operations #27571

Merged
merged 8 commits into from Feb 26, 2019

Conversation

@ralphschindler
Copy link
Contributor

commented Feb 18, 2019

Overview

The driving use case behind this PR is to allow Pivot tables/classes/models to be able to trigger model events, which are currently not possible since InteractsWithPivotTable does not go through the Eloquent model.

Changes and tests have been provided.

Details

This PR adds additional support to InteractsWithPivotTable (used by Relations\BelongsToMany|MorphToMany) such that write operations to a pivot table (specifically: attach, detach, updateExistingPivot) will use the defined $this->belongsToMany()->using(MyPivot::class) when performing these operations. By using the Pivot (through AsPivot trait methods) as an eloquent model, you gain the ability to utilize the eloquent lifecycle events when Pivot's are saved/deleted.

(Sidenote: I believe there are other use cases for supporting Pivot tables at true Models, I feel like there is a use case for treating Pivot's as first class Models inside Nova.)

Example

In a situation where there exist a Category, Post, and a Pivots\CategoryPost inside an application:

namespace App\Models {
  class Category extends Model
  {
    public function posts()
    {
      return $this->belongsToMany(Post::class);
    }
  }

  class Post extends Model
  {
    public function categories()
    {
      return $this->belongsToMany(Category::class)->using(Pivots\CategoryPost::class);
    }
  }
}

namespace App\Models\Pivots {
  class CategoryPost extends Pivot
  {
    public static function boot()
    {
      parent::boot();

      static::saving(function (Model $model) {
        echo "In [{$model->getMorphClass()}|{$model->getQueueableId()}] - SAVING\n";
      });

      static::saved(function (Model $model) {
        echo "In [{$model->getMorphClass()}|{$model->getQueueableId()}] - SAVED\n";
      });

      static::deleting(function (Model $model) {
        echo "IN [{$model->getMorphClass()}|{$model->getQueueableId()}] - DELETING\n";
      });

      static::deleted(function (Model $model) {
        echo "IN [{$model->getMorphClass()}|{$model->getQueueableId()}] - DELETED\n";
      });
    }

    public function setOrderAttribute($value)
    {
      $this->attributes['order'] = ((int) $value) * 10;
    }
  }
}

A sync method might look like, and its output:

$category1 = Models\Category::create(['name' => 'One']);
$category2 = Models\Category::create(['name' => 'Two']);
$category3 = Models\Category::create(['name' => 'Three']);
$post = new Models\Post(['name' => 'My Post']);
$post->save();
$post->categories()->sync([$category1->id => ['order' => 1], $category2->id => ['order' => 2]]);
$post->categories()->sync([$category3->id => ['order' => 5], $category1->id => ['order' => 9]]);

// In [App\Models\Pivots\CategoryPost|post_id:1:category_id:1] - SAVING
// In [App\Models\Pivots\CategoryPost|post_id:1:category_id:1] - SAVED
// In [App\Models\Pivots\CategoryPost|post_id:1:category_id:2] - SAVING
// In [App\Models\Pivots\CategoryPost|post_id:1:category_id:2] - SAVED
// IN [App\Models\Pivots\CategoryPost|post_id:1:category_id:2] - DELETING
// IN [App\Models\Pivots\CategoryPost|post_id:1:category_id:2] - DELETED
// In [App\Models\Pivots\CategoryPost|post_id:1:category_id:3] - SAVING
// In [App\Models\Pivots\CategoryPost|post_id:1:category_id:3] - SAVED
// In [App\Models\Pivots\CategoryPost|post_id:1:category_id:1] - SAVING
// In [App\Models\Pivots\CategoryPost|post_id:1:category_id:1] - SAVED

Inspiration & Research

https://github.com/fico7489/laravel-pivot
https://gitlab.com/altek/eventually/
https://github.com/spatie/laravel-activitylog
Laravel Issue: #8452
https://medium.com/@codebyjeff/custom-pivot-table-models-or-choosing-the-right-technique-in-laravel-fe435ce4e27e
https://github.com/signifly/laravel-pivot-events
https://stackoverflow.com/questions/46358388/laravel-5-5-events-not-fired-on-pivot

@ralphschindler ralphschindler force-pushed the ralphschindler:5.8-pivot-models-when-using branch from 0fa7a3f to f981962 Feb 18, 2019
@ralphschindler ralphschindler force-pushed the ralphschindler:5.8-pivot-models-when-using branch from f981962 to 5d8c183 Feb 18, 2019
@ralphschindler

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

Seems like the clock is ticking on 5.8 😄 , is there anything more I can do to chauffeur this along? I think many might find this PR useful for similar situations to ours: needing a comprehensive way to audit changes (ie: audit trails, audit logging) to data that flows through Eloquent (including changes to pivot/junction tables). Thanks again!

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Thinking through this. Is there any implication to setting that incrementing property to false on Pivot models when NOT using a custom pivot class?

My initial thinking is "no" since we were never really using the custom models when performing any insert / update / delete operations on Pivots. If the user iterated over their pivot models to update some values and called save I don't think the incrementing matters at that point since ID is already assigned.

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

It seems the main breaking change with this PR is if people are using a custom pivot model and were also using an incrementing primary key on that model (I believe Laravel Nova itself does this actually - nevermind, it doesn't), they would need to update their custom pivot models to set incrementing to true.

Are there any other breaking changes here?

@ralphschindler

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

It is entirely possible that change can be backed out... I could write a test to capture the scenario of "someone extended a pivot, is using() it, and it has primary key / autoinc instead of a compound primary key" (which is what Laravel generally wants to you have, pivots without id's).

Separately, Pivot or AsPivot could have a flag (instead of checking using()) that would make the InteractsWithPivotTable use the Pivot::save()/Pivot::delete(), that would make it completely backwards compatible and fully opt-in.

(I read your comment in laravel/ideas that basically said Eloquent was going to move towards using Pivot (Eloquent save/delete) methods anyway, instead of crafting the SQL to run the write operations with). laravel/ideas#953 (comment)

That would mean someone who wanted the Pivot to use its own save/delete might write code that looks like this:


namespace App\Models\Pivots;

use Illuminate\Database\Eloquent\Relations\Pivot;

class FooBar extends Pivot
{
    public $usePivotSaveDelete = true;
}

Then perhaps this becomes the default in 6.0?

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

But, is that the only breaking change on this PR that you are aware of?

@taylorotwell taylorotwell merged commit 0ba72ab into laravel:5.8 Feb 26, 2019
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taylorotwell

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Eh, there is another breaking change that may make me back this out for now... it's basically impossible to do a batch detach now when using a custom pivot model unless I'm mistaken. In a 5.8 application I have, I was doing this:

image

My tests began failing after this PR and it's because obviously I can't chain query conditions on a detach anymore when using a custom pivot model.

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Eh, disregard previous code example... but I do wonder if there is a problem here when using wherePivot and wherePivotIn when performing a detach operation with a custom pivot class.

@erikgaal

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@taylorotwell

Eh, disregard previous code example... but I do wonder if there is a problem here when using wherePivot and wherePivotIn when performing a detach operation with a custom pivot class.

Just observed this exact behaviour in our app. Our app can detach Role from User based on some pivot data. A user can have multiple roles within different contexts. However, since this change, the following did not work anymore as it would detach all related models instead of those matching the wherePivot's.

// This would ignore the context_id
$user->roles()->wherePivot('context_id', 'some-id')
    ->detach('some-role-id');

// This is the workaround
$user->roles()->wherePivot('context_id', 'some-id')
    ->wherePivot('role_id', 'some-role-id')
    ->detach();
@driesvints

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

@erikgaal please open an issue if this PR caused a bug.

@staudenmeir

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

@erikgaal detach() has been fixed in #27997.

@lk77

This comment has been minimized.

Copy link

commented May 3, 2019

Hello,

it seems that since this PR, sync is taking into account the soft delete on pivot models.
When deleting pivot records, they are soft deleted now and not hard deleted.

But the soft delete is not taken into account when reading data from pivot models, they still appear,
which means that relations are undeletable now, if using SoftDeletes Trait. For me it's inconsistent, if it's taking into account soft-delete when deleting, it should also take it into account when reading.

Thanks.

@driesvints

This comment has been minimized.

Copy link
Member

commented May 3, 2019

@lk77 please open up a new issue with a detailed explanation what you're experiencing. Fill out the issue template as well please.

@staudenmeir

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

There are so many issues with this PR, I think we should consider reverting or moving it to 5.9.

@taylorotwell

This comment has been minimized.

Copy link
Member

commented May 3, 2019

@staudenmeir what does "so many issues" mean? Specifically how many do you think. Can you list them so we can determine if should fix them or revert? /cc @ralphschindler

@staudenmeir

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Fixed:

Not fixed:

Breaking changes:

@taylorotwell

This comment has been minimized.

Copy link
Member

commented May 5, 2019

@ralphschindler are you able to help us with any of these?

@taylorotwell

This comment has been minimized.

Copy link
Member

commented May 5, 2019

@themsaid has fixed #28150 now. We will be looking at the others this week.

@themsaid

This comment has been minimized.

Copy link
Member

commented May 6, 2019

I don't believe #28407 is a bug. Also #28025 isn't actually a bug, not sure why you shared it as a bug @staudenmeir.

@AdrianKuriata

This comment has been minimized.

Copy link

commented May 17, 2019

@staudenmeir @taylorotwell @themsaid Hello, I tried to detach all elements attached to the model, but with no effect. I have User and Group model connected with belongsToMany relation, in the User model I have groups method:

public function groups()
{
    return $this->belongsToMany(Group::class)->using(GroupUser::class);
}

In the Pivot GroupUser class I have a boot static method for listening to events:

public static function boot()
{
    parent::boot();

    static::deleting(function (Model $model) {
        logger('Detaching group or groups');
    });
}

And code, which I call:

$user->groups()->detach(); // It is not work

$user->groups()->detach(1); // Works ok

$user->groups()->detach([1, 2]); // Works well too

My question is, do you in the short future make it working without adding any parameter to the detach() method? Problem is with deleting event for the Pivot class which is not called when you don't put any parameter to the detach() method.

Sync not working too.

@staudenmeir

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@themsaid I called #28025 an issue, not a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.