important framework issue

Yii 1.1 General Discussion
1

Please see issue posted here.

Reposting below for convenience - please read before voting, and feel free to clarify your vote and discuss here.

[quote name=‘mindplay’]CModel::setAttributes() silently ignores access violations - that is,

massive assignment of attributes which are not safe, and should not be

permitted under the current scenario, are simply ignored.

This, combined with the fact that safeAttributes() is no longer supported

in 1.1, caused us all manner of headaches while upgrading to 1.1, resulting

in countless silent malfunctions in all manner of models - very hard to

track and debug. The only way to debug these errors, is to manually go

through every single model and rebuild portions as needed.

An attempt to write an attribute that isn’t safe is the current scenario

should result in an exception, because it is essentially an access

violation.

Imagine if PHP would silently fail when you try to set a property that is

declared protected or private - debugging would be nearly impossible.

Submitting form inputs that cannot be validated, or have been explicitly

marked as unsafe, must result in an exception - otherwise, there is no way

to reliably debug forms and models.

Even with CUnsafeValidator, an access violation will be allowed to pass by

in silence. If I have specifically marked an attribute as unsafe in a given

scenario, I think we can assume that an attempt to set the attribute is a

hacking attempt, or at the very least an oversight on the developer’s part.

If somebody really does intend to use unsafe attributes as a means of

access control, perhaps CUnsafeValidator could offer an option to allow

access violations to fail in silence. Personally, I would consider such use

of this feature bad practice, but this should be the only curcumstance

under which an access violation should be acceptable and silently fail.
[/quote]

[quote name=‘qiang’]While raising exceptions may be preferable in some cases, it is not so in some other

cases. The current design basically means: please ignore those values that are not

safe. I am not sure if raising exception is the most desired behavior. I do believe

if we change this behavior, it will cause fire from many people.

One solution for you is to write the base class ActiveRecord and FormModel and

override setAttributes() to raise exception as you want. This should help you more

smoothly upgrade from 1.0 to 1.1.
[/quote]

[quote name=‘mindplay’]I don’t understand what practical reason you could have for ignoring values that are

not safe.

In practice, when (or why) would you expose an attribute, with an enabled input on a

form, during a scenario that does not allow modification of the attribute?

I can’t see any practical use for this feature.

On the other hand, I can see plenty of reasons (as listed above) why this would cause

problems for many developers - not just during upgrades, as stated, but during normal

everyday development.

I believe this behavior should be available, but making this the default behavior is

going to cause lots of silent errors, and really seems like a bad call.
[/quote]

2

I think by default it shouldn’t throw exception as qiang noted here.

3

I completely agree qith qiang, all the time I do massive assignements like this:

$model->attributes=$_GET or $model->attributes=$_POST

Sometimes I submit more things in the $_POST variables, because there is a hidden field in the form that might not be used in a certain scenario.

For me the safe attributes list is like a filter.

I would not want any exceptions to be thrown. What would I do with that exception?

If there are so many troubles upgrading to Yii 1.1, has anybody considered to change the Yii 1.1 behaviour?

In my opinion the safeAttributes property should not have been dropped.

4

The word "safe" to me implies a security feature, not just a general-purpose filter.

Mixing models sounds error-prone to me, I wouldn’t consider that good practice.

I guess not everyone is as strict on those issue as me…

5

@mindplay:

I think you could extend CActiveRecord and CFormModel and override setAttributes() (defined in CModel) there. Should be pretty easy:


            if(isset($attributes[$name]))

                $this->$name=$value;

            else 

                throw new CException(...);
6

I could easily implement this feature and move on with my life.

I’m not campaigning for this issue because I want somebody else to do the work for me, but because I think this framework is the cleanest, most well-engineered framework available for PHP.

I see this issue as standing in the way of reliable and fast development, and I would like to see this framework succeed as the primary framework where I work, and everywhere else.

Yii beginners, in particular, are going to potentially waste a lot of hours learning why some code fails silently, without any kind of explanation.

Experienced developers are going to waste time too - testers often simply flip through the application submitting forms to see if anything generates exceptions; when a single attribute, such as a checkbox, or an empty optional text-input aren’t saved, there’s no way to tell, until somebody, at some point, attempts to use that particular feature on a form and notice that something was not saved.

It’s considerably faster if common problems pop up early, in the form of exceptions - so that you can test everything at once, trap and fix all errors in one round, rather than having to go back and bother a developer with individual bug reports over weeks or months as the bugs are slowly discovered. It’s just not an effective way to work.

Judging by the vote so far, the majority thinks that an access violation should not cause an exception by default. Only one developer thinks that access violations should never cause an exception. I personally voted "by default", but if "not by default" is what the majority wants, I think that would be the most sensible approach. It would also be the only truly backwards-compatible solution.

7

I still do not see how an access violation exception would help you to find out if

“a single attribute, such as a checkbox, or an empty optional text-input” isn’t saved.

Maybe I do understand you wrong.

Testing is a whole other issue in my opinion.

If one wants to be sure then unit testing should be implemented, always writing a test first and then writing your code. Skipping through forms (I admit, that I also test this way), is not really the right way :slight_smile:

8

sorry, my question is: safeAttributes as been dropped, now Yii how know which are safe and which not?

9

If you forget to mark an attribute as safe, it can’t be saved - if you submit a form with an unchecked checkbox, or an empty text input, you can’t tell if it’s being saved or not, because the result either way (whether it was saved or not) will be an empty attribute.

With an exception being thrown when you try to assign the attribute, you would catch these problems immediately.

Another example is, if you have an input on your form, which is not supposed to display in a certain scenario, and your model did correctly declare the corresponding attribute as unsafe in that scenario, you would get an exception, reminding you that you were supposed to hide that input in a given scenario.

These are just examples, I’m sure there are plenty of other situations where exceptions would help you.

And yes, there are a few, marginal situations where it will hinder - but as explained, in those cases, if you have a good reason to mix attributes from different models in the same form collection, it’s probably a good idea to clarify in your code that you did this intentionally; having to handle the exception forces you to write an exception-handler in these cases, and this results in self-documenting code…

10

Ok, point taken.

I did not mean to mix attributes from different models.

There are always situations where you have more data in your $_POST or $_GET variable than your model will accept.

For instance a piece of data that is intented for the controller. It would be really annoying to have to remove data from $_POST or $_GET to then be able to massively assign using these arrays.

Anyways I see your point about testing, so it could be an interesting feature to be enabled through configuration.

11

What about having it throw exceptions when DEBUG is true otherwise fail silently. I certainly don’t want exceptions being thrown if someone creates some extra POST data and sends it to my site.

Exceptions during debug would mean that you pick things up quickly during development, but that you don’t have extra issues when you go live.

12

Yes, and you should then group it into arrays, e.g. one element per model - which is what Yii does by default in the first place. When $_POST[‘User’][…] contains only of the attributes for the User model, $_POST[‘SearchForm’] for the SearchForm model, etc.

If your forms are organized and grouped "per entity", you should never run into this problem…

13

Well, I am new to yii and I have to admit upgrading to 1.1.1 has caused many things to break. And to reinforce what the original poster voiced, I have spent hours of time wasted trying to fix things. Some issues are easily fixable others I have no clue what is going on.

As for my opinion… Pretty much every language I’ve worked with will raise some type of error or warning for an access violation. With databases this is even more true. You can’t connect to a database or reference a table or field or whatever for which you don’t have rights to access and expect the request to be ignored. The way yii (ar) works with data has been the most difficult part of learning for me. For example, having a field set as required and not submitting the field should also throw an error, but it doesn’t. It reacts as if the form never posted. The same appears to be true with not including a safe attribute property. When you consider that a majority of all your fields WILL be inputted by the user THEN the default behavior really seems out of wack. I could have a table with 30 fields and only 1 - the primary key - would actually not be safe. And not everybody is going to build forms where they rely on the omission of hidden vars. Seeing that I’m new to yii and certainly not an expert my opinion is not based on the breadth of knowledge that the creators of yii possess. But I still stand by my opinion.

As for a solution… Not sure if this is possible, but since yii uses events would it be possible for the AR to raise an event like accessViolationEvent instead of an exception. This way the developer could use a handler like OnAccessViolationEvent. Personally, I don’t want to start overriding a bunch of what yii already does - most people new to the framework aren’t going to, either. I’m willing to learn to do things the expected way. Good documentation and examples will go along way to helping all us newbs out. But I still don’t think ignoring “massively” assigned vars - be them safe or required - is the best solution either.