dwightwatson / validating

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

Create static getters #186

Closed GregPeden closed 7 years ago

GregPeden commented 7 years ago

This adds static call methods for the attribute names and messages.

It also fixes one of the unit tests which was calling the rules() static method instead of the getRules() non-static method on an instanced class object. This is really a semantic change unless the rules() method is changed for other purposes.

dwightwatson commented 7 years ago

Wow, I had no idea Eloquent would call protected methods if you called them statically. What do you think about just implementing them as static methods though? I think it makes it clearer to anyone that reads the code how they're intended to be used. In addition I don't suppose I could trouble you for some test cases for these?

GregPeden commented 7 years ago

I tried that and it is glitching out, I was unable to figure out why. Otherwise I agree that makes sense.

So you're thinking:

public static function rules()
{
    return (new static)->getRules();
}

In my testing I have been getting a code 500 error centered around that "(new static)" code structure, though only in case where the property $rules is not defined on the model (per my other PR which you decided to not adopt). That's not a big deal for rules except:

As is often the case with Laravel, I am sure something mystical and secret is happening in the background related to how loose Laravel is with static declarations and the like, so I can't trace it. The odd thing is, the __callStatic() magic method is just creating a new instance of the class and then running the method on it, just as you have proposed, only it occurs automatically instead of explicitly. Otherwise I don't get why my use generates a 500 error.

GregPeden commented 7 years ago

As mentioned before, this would be more straightforward and better optimized for performance if the validation properties on the model were defined as static, then they could just be looked up on the static class without any instancing tricks, and it would save some compute resources by not having to instance the class. However, I understand that this is not in the cards.

dwightwatson commented 7 years ago

How bizarre, not sure why that would be the case. Unfortunately I no longer have any current projects that use this library so it's not going to be easy for me to dive down that rabbit hole. If this works let's go with it.