dwightwatson / validating

Automatically validating Eloquent models for Laravel
MIT License
968 stars 76 forks source link

saveOrFail added to Illuminate\Database\Eloquent\Model. #146

Closed shawndalton closed 8 years ago

shawndalton commented 8 years ago

laravel/framework (5.1.x-dev ac2d4d4) Breaks Watson\Validating\ValidatingTrait :

Declaration of Watson\Validating\ValidatingTrait::saveOrFail() should be compatible with Illuminate\Database\Eloquent\Model::saveOrFail(array $options = Array)

dwightwatson commented 8 years ago

Thanks for spotting this.

I don't think there is much point fighting the Laravel API here, I'm leaning towards having our saveOrFail method match their signature and then call it under the hood.

I think we'll do a validation check first, throw the validation exception if the model is invalid, otherwise pass the call onto Laravel's saveOrFail(). Users of this method should be expecting an Exception on failure, though they're probably prepared only for a ValidationException. Not really sure of a better way to handle this, but I suppose this solution is better than breaking apps when the next release of Laravel is tagged.

I'll get this update out shortly.

Edit: Actually, not going to be as simple as that... this new code will be dependent on the new saveOrFail method in the next release. I'll push this code into dev, but I won't be able to tag it until a new Laravel has been tagged and I can lock that as a dependency.

dwightwatson commented 8 years ago

This fix is on dev-master now. I'm not sure how the best approach to make this only available to people using at least Laravel 5.1.27, or to make it cross-compatible.

lostincode commented 8 years ago

I think cross compatibility would save the headache of new github issues. Tagging a release works for me as well. Thanks for getting on this so quick. :+1:

ValdasK commented 8 years ago

This change causes infinite loop, as saveOrFail method calls itself over and over again.

($this->getModel() returns same class with already overwritten saveOrFail method, why did you think that it can work?)

dwightwatson commented 8 years ago

Whoops - clearly a mistake. I must've been in a mistaken mindset that the trait was somehow separated from the model instead of overriding it. I'm not going to be able to address this issue today but happy to accept a PR that resolves it.

ValdasK commented 8 years ago

I guess all that you have to do is to change $this->getModel()->saveOrFail() into parent::saveOrFail()

dwightwatson commented 8 years ago

That's what I was thinking, but I'm not sure (from memory) if you can call the parent method from a trait. Will have to test it.

ValdasK commented 8 years ago

I wasn't sure too, that's why I have tried it, and it works:

<?php
trait test {
  public function a() {
    parent::a();
  }
}

class aClass {

  public function a() {
    echo 'aClass';
  }

}

class bClass extends aClass {
  use test;

  public function __construct() {
    $this->a();
  }
}

new bClass();

Output: aClass

dwightwatson commented 8 years ago

I've pushed an update to master that should resolve this, I'll tag a release in a bit.

Ignore the failing tests, I tried updating PHPUnit but the older versions of PHP weren't happy with that.