laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.17k stars 10.89k forks source link

Issues updating extra columns on polymorphic pivot tables #3693

Closed weotch closed 10 years ago

weotch commented 10 years ago

Given a buildings table and a services table and a pivot table that looks like:

serviceables
- id
- serviceable_type
- serviceable_id
- service_id
- featured          // My extra column
- created_at
- updated_at

Buildings (and other models) can have Services. The Service model has this relationship:

<?php
class Service extends Model {
  public function buildings() { 
    return $this
    ->morphedByMany('Building', 'serviceable')
    ->withTimestamps()
    ->withPivot('featured');
  }
}

I attempt to update the pivot column like so:

<?php
Service::find(7)
  ->buildings()
  ->where('serviceables.serviceable_id', '=', 16)
  ->first()
  ->pivot
  ->update(array('featured' => 0));

I get this exception:

Column not found: 1054 Unknown column 'morphType' in 'field list' (SQL: update serviceables set service_id = 7, serviceable_id = 16, created_at = 2014-02-20 17:16:43, updated_at = 2014-02-20 17:16:43, featured = 0, morphType = serviceable_type where serviceable_type is null and service_id = 7 and serviceable_id = 16)

There are a couple of things I've noticed here that I think are incorrect:

gmishkin commented 10 years ago

I did a couple things to work around this.

Add "protected $morphType;" to MorphPivot, so that when MorphToMany newPivot calls setMorphType on the MorphPivot, the type column name doesn't get added to the attributes of the pivot model (this is where your extra morphType column is coming from).

Then to handle the value for that not being set, I added a line to MorphToMany __construct after $this->morphType is set that calls $this->withPivot($this->morphType). Now that will be there automatically.

I also noticed that all the columns were being updated (empty original). I think fixing that could help, but I think having at least the first change would still be necessary to prevent an attribute called "morphType" from getting into the pivot model (which would of course not be present in the original, so it would look dirty, so it would attempt to be saved incorrectly). Maybe bringing the actual morph class value in with the extra withPivot call wouldn't be needed, because maybe there's not actually a way that would ever change.

P.S. this was happening because I was calling ->pivot->save(). If there's another way you're supposed to update pivot models let me know, maybe that's the issue.

skovacs1 commented 10 years ago

How has this issue has not been addressed in these two months? Perhaps a pull request will bring it to someone's attention.

Adding $morphType as a protected object on the MorphPivot class resolves the erroneous column in the query so at minimum, that makes for a solid pull on all applicable branches.

It would be a strange use case where a morphType would change. This would imply that an object from one table representing one type was being moved to another. In the context of polymorphism, this can reasonably happen when casting inherited objects to their parent classes, etc., but in a database, this is a bit of an oddball indicative of some curious database design. The correct approach would be to insert copied data as a new model into the other table, then change the morphable object id and morphType to maintain the associated data about the relationship and drop the old object. I have trouble coming up with a specific use case for that and it sounds rather convoluted, but maybe I'm missing something there.

The empty original is certainly making for inefficient queries and this could get costly if the pivot data were large. That certainly warrants further looking into.