kirkbushell / eloquence

A drop-in library for certain database functionality in Laravel, that allows for extra features that may never make it into the main project.
MIT License
543 stars 58 forks source link

Error thrown in model when trying to implement a custom pivot model relationship #1

Closed bryantAXS closed 10 years ago

bryantAXS commented 10 years ago

I've created a custom pivot model with the following code, and am getting an error:

Edit :: Sorry this isn't the custom pivot model, this is a normal model, but contains the newPivot method necessary to use a custom pivot model, which is related to this model. I'm adjusting the title too.

ErrorException: Declaration of LandUseType::newPivot() should be compatible with Illuminate\Database\Eloquent\Model::newPivot(Illuminate\Database\Eloquent\Model $parent, array $attributes, $table, $exists)
class LandUseType extends \Eloquent {

  protected $table = "land_use_types";

  protected $guarded = [
    "id"
  ];

  protected $fillable = [
    "unit_name",
    "ite_trip_gen",
    "sf_per_unit",
    "sp_per_unit",
    "site_acreage_multiplier",
    "gallons_per_unit_per_year",
    "tons_per_unit_per_year",
    "icon",
    "short_description",
    "long_description"
  ];

  public function sites()
  {
    return $this->belongsToMany("Site", "site_land_use_types");
  }

  public function newPivot(Eloquent $parent, array $attributes, $table, $exists) {

    if($parent instanceof Site){

      return new SiteLandUseType($parent, $attributes, $table, $exists);

    }

    return parent::newPivot($parent, $attributes, $table, $exists);

  }

}

Any thoughts on this?

I have added the service provider, adjusted my aliases, and ran a dump-autoload

kirkbushell commented 10 years ago

Hi @bryantAXS, thanks for the issue report.

This appears to be a PHP issue - the signature that you've created for your own newPivot method is actually different to the Eloquent signature, which has resulted in the error. If you match the signature, you should be good to go. Specifically, you're not using the right class for the first argument, it should be:

Illuminate\Database\Eloquent\Model

Btw, apologies for taking so long - for some reason I didn't see any notification for this =\

bryantAXS commented 10 years ago

Hey Kirk, thanks for the reply! I made these updates and they all appear to be working.

Quick secondary question though: How does this effect the specification of the rules, attributes, guarded, and fillable properties assigned on a model? Should they use the snake case I'm using within my DB schema, or the snakeCasing I'm enforcing in the model?

Thanks!

kirkbushell commented 10 years ago

You should still be using snake_case when defining your database schema, aka your migrations. The only time you really need to use snake_case when using this class within your application, however - is if you're looking to access the $attributes or $original arrays directly. Eloquence doesn't touch the original casing, it leaves it alone.

Does that answer your question?

bryantAXS commented 10 years ago

I believe so. So for the guarded, fillable, and rules properties, those will use the snake case still. And essentially the only time I would still need snake case is for the DB migrations... Correct?

On Wed, May 14, 2014 at 12:32 PM, Kirk Bushell notifications@github.comwrote:

You should still be using snake_case when defining your database schema, aka your migrations. The only time you really need to use snake_case when using this class within your application, however - is if you're looking to access the $attributes or $original arrays directly. Eloquence doesn't touch the original casing, it leaves it alone.

Does that answer your question?

— Reply to this email directly or view it on GitHubhttps://github.com/kirkbushell/eloquence/issues/1#issuecomment-43119292 .

Bryant Hughes bryant@thegoodlab.com www.thegoodlab.com @thegoodlab http://www.twitter.com/thegoodlab

kirkbushell commented 10 years ago

Yup, that's right. Tbh I haven't actually tested guarded/hidden.etc. attributes, so let me know if you encounter any issues :)

bryantAXS commented 10 years ago

Sounds good!

On Wed, May 14, 2014 at 12:46 PM, Kirk Bushell notifications@github.comwrote:

Yup, that's right. Tbh I haven't actually tested guarded/hidden.etc. attributes, so let me know if you encounter any issues :)

— Reply to this email directly or view it on GitHubhttps://github.com/kirkbushell/eloquence/issues/1#issuecomment-43121085 .

Bryant Hughes bryant@thegoodlab.com www.thegoodlab.com @thegoodlab http://www.twitter.com/thegoodlab