I think it's important to always have the options of declaring the attribute safe/unsafe. Now the default it unsafe unless defined safe.
If the default will be "save if it has a rule", then we'll have to have a way in the rule to make it unsafe.
Personally i wouldn't like it to be safe automatically if it had a rule.
For me it doesn't make sense that an attribute is considered safe if it simply has a rule. For instance sometimes I define rules and do not set a specific scenario for them, simply because if the field is empty the validator won't be used anyways (so I do not need to).
What I was proposing in the first place is for attributes to be considered safe if they are set to be required
@Jonah: could you explain more why it doesn’t make sense that an attribute is considered safe if there is a rule applicable to it for the given scenario?
Here's my argument: given a scenario, if an attribute has a rule to be applied, that means we know it will be validated before being saved to database. Unsafe attributes are usually PK, FK, security-related attributes.
PK: we seldom list it in validation rules unless PK is something to be entered by user. In this case, having a rule does mean it is safe to be massively assigned.
FK: some should be set by user input (e.g. post category), some set in the program (e.g. post author). For the former, we should have a rule to ensure the category is set correctly. For the latter, no rule is needed because it doesn't come from user input. In both cases, our initial rule still works.
security-related attributes: the analysis is similar to FK.
As you can see, in all these cases, we don’t really care if the rule is required or not.
What if a rule does not specify a scenario? Will it apply to all scenarios in that case, as it does now?
If a rule is specified without a scenario does that mean it's attributes is considered safe for all scenarios?
Yes, it will apply to all scenarios, and is a safe attribute for all.
I think the idea is simple and convinient - but would make it easier to accidently create a security hole.
That would mean that you can't set rules for attributes that you deem unsafe.
Which means that you have to trust your co-workers and those 'authorized' people to put correct data in, because it can't be validated (with rules).
For an example: Let's say an attributed called 'securityLevel'. You want to make sure it's integer between 1 and 5. You can't make a rule for it, since you don't want public users to be able to assign in when creating User instance. So you have no way of enforcing data integrity in the database, unless you write your own validation outside the rules().
But if that's the way it will be implemented, can we at least be able to mark the rule 'unsafe' - so we can still make a rule even for those unsafe attributes.
I agree that we may have 'unsafe' rule to make the whole thing complete. It is very rare though that we would need to use 'unsafe' rule in practical applications. For the 'securityLevel' example you gave, it would mean we do not trust the explicit assignment we have for the 'securityLevel' attribute and would use some rules to ensure the data sanity even though the data is set using an explicit assignment.
The only thing that bothers me is that sometimes I currently might define a rule with no scenario even though that rule does not actually apply to all scenarios. For instance I may set an email attribute to use the email validator. This way, the email attribute will always validate as an email address when it is needed to
This way I don't have to assign the email rule to the email attribute for every scenario that uses it
But with this new implementation now it will always be considered 'safe' if I do this technique
So that should be considered
Let me summarize: we would like attributes that can’t be massively assigned.
Otherwise, a distressing security hole would be present.
–
Can I suggest not to implement these validating rules via methods, but public properties. I’ve seen numerous methods returning arrays and string that can be stored in class properties. This shouldn’t be a performance consideration, but semantically, using methods to store data is wrong, I guess. Am I right?
@Qiang: If we'd have a way of marking them unsafe, then I'd be happy with this.
@Jonah: But wouldn't the email attribute be safe anyway?
In which cases wouldn't you want the user's mass assign to include e-mail ?
@Jonah: I can see your point. The fact is: with the current 1.0 implementation, 'email' is safe by default because it is not a PK attribute. Unless you define safeAttributes() that explicitly excludes 'email' from the safe list for the particular scenarios, the new scheme will not be less secure (it is actually more secure in most cases).
@pestaa: the reason to use method to return rules is to allow dynamic customization. For example, you may want to use Yii::t() to customize the error message. This is not possible with public properties.
Yes, I do have safeAttributes() customized to do that. I suppose it will work fine your way, personally I think over all it will require typing a longer rules() list, and you will have to be careful about adding new scenarios so that all the appropriate rules are assigned to it
that was just an example. There could be some cases where you do not want an email attribute massively assigned though
You are totally right. I wasn’t familiar with this limitation.
What about declaring these arrays in the constructor and storing them in public properties anyway? I wanted to stick to semantically correct ways, so here’s my golden path.
In this case, discussion about merging methods (which can be confusing in the end because of endless configuration possibilities) won’t be necessary, since the data is in the properties. (Simplifying their declaration may be still needed, though.)
I have implemented the changes we discussed here:
Remove safeAttributes() and use validation rules to decide whether an attribute is safe or not.
Add 'safe' and 'unsafe' validators.
Additionally, I removed the $scenario parameter from all methods requiring it. Instead, we use $model->scenario to decide what the current scenario is. This makes the APIs much cleaner and more consistent.
We will keep attributeLabels() as is.
Please let me know if you find any problems with these new implementations. The code is in trunk (not 1.0 branch).
Great, Qiang!!
And what about the BC for the actual safeAttributes method? It will be supported?
We are breaking BC for this feature. We will give detailed explanation in the Guide on this BC breaking change.
Qiang, could you post an example code applying this new approach?
Maybe could be possible to develop an script to add the actual safeAttributes to the end of the rules method. This should make this BC-break less critical.
Great improvements, Qiang, thank you for all the efforts!
What’s the difference between applying unsafe validator and not applying any safe/unsafe validators at all? Why not assume that all fields are unsafe unless explicitly marked as safe? Or do they already have a default behavior?
The rules of thumb of this new implementation are:
An attribute is safe if it is associated with a validation rule for the current scenario.
An exception is that if the associated validation rule is 'unsafe', the attribute is unsafe.