dwightwatson / validating

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

Make setters default to empty array, fix missing attribute exception #185

Closed GregPeden closed 6 years ago

GregPeden commented 6 years ago

This does two related things:

So, that error is thrown because Laravel has overwritten the "isset()" method on Eloquent/Model. This is done because otherwise it will not properly handle attributes and relationships which are present on the model using magic methods. This method implementation fails to account just good 'ol class properties added to the model in the manner which this package uses them.

Wrapping the call in a try/catch and then discarding a LogicException as equivalent to "isset(...) === false" resolves this issue. It is then not necessary to define $rules or the other validation properties on the model.

dwightwatson commented 6 years ago

Sorry, I'm not comfortable putting a hack in place to get around how Eloquent works internally. Especially when you're logging in the code that it's tested on a certain date against a certain version - it just adds more fragility. I don't want to go and have to test this manually against every version of Laravel just to ensure it still works.

I think if you're adding this trait to a base model it's fair to expect that you define rules on every model or once on the base model. I think this is solving a problem that doesn't really need to be solved. I appreciate the time you've taken to put this together and I apologise that I don't feel it's a good fit.

GregPeden commented 6 years ago

Well, I ask you to consider that the way Laravel has overwritten isset() is itself a hack mess, there are lots of comments on the project about it but it hasn't changed. The LogicException that your package gets is precisely because the isset() implementation in Laravel is broken and has been for awhile, so it accidentally looks the property up as a Laravel relationship because it is not a database model attribute. Also, I believe once you drop support for Laravel 5.4 and earlier, in which case you can use PHP 7.0 syntax, you can use the null coalescer and that solves the problem as well. That's really why I left the version comment.

But, your call.

The reason it's relevant is because all of the validation properties will need to be overwritten on the BaseModel if they are going to be used in a patterned matter, as I am. I will indeed be just adding them all as blanks to my "base model" in lieu of this solution.

dwightwatson commented 6 years ago

That's fine but that's a discussion that needs to occur over on the Laravel side of things. I'm not bothered by it, and I feel it's better for an Eloquent-specific library to embrace Eloquent as-is rather than institute hacks around whatever quirks it may have. If Eloquent changes I'd be more than happy to change with it but I don't want to work around it. Using the null coalesce operator as a solution is definitely something that can be considered down the line.