[qiang]

Currently, when writing a model, we often need to override three methods that are related with attribute specifications: rules(), safeAttributes() and attributeLabels(). It might be more convenient and maintainable if we could merge them into one single method.

We may follow Jonah's idea about merging rules() and safeAttributes(): if an attribute is listed in a rule (for the given scenario), it is considered to be safe.

We'd like to hear your opinions and suggestions. In particular, if we choose to merge them, how should the merged method look like?

[ricardograna]

I disagree. I like the methods being splited, for me it's more maintainable (personal opinion)

By Jonah's idea Yii will be forcing using safe attributes and I don't appreciate them very much.

[jonah]

Combining attributeLabels() also?  Interesting..  Personally I decided not to use attributeLabels() anymore as it seems to be more work for me then its worth (although I might find a good use for it here and there, but generally I won't use it really)

but I guess that's irrelevant...

&rickgrana has a point, every once in a while it would force you to write out a safe attribute rule if you use my method (if you want an attribute to be mass-assignable to the model but is not considered "required" - in my experience this is not very often) even if you don't care if your app is safe

[PoL]

Merge rules() and safeAttributes in one methos seems to be good, may be adding a new rule, to the rules() method...
Ej.:

<?php

public function rules()

{

	return array(

		array('someAttribute1','length','max'=>50),

		array('someAttribute2','length','max'=>255),

		array('someAttribute1', 'required'),

		array('someAttribute3,'numerical','integerOnly'=>true),

		array('someAttribute1', 'someAttribute2', 'safe'),

	);

}

If somebody gives me a good example of combine attributeLabels() with rules(), then i'll consider this alternative, but for now, I love attributeLabels().

[Sentinel]

I strongly agree.
Currently I am creating something I call "field definition" - it is a simple class, containing everything a field must know about itself - label, default validators, field name in the database, friendly field name in the model etc. (I am still working on how to add scenarios correctly).

My idea was that fields are actualy repeating through the projects and even in the models - I mean - If in one model have field "name" or "email", it will be pretty same in another model/project. The only thing that strongly differentiates them is the scenarios they are used in...

I am still working on this "microarchitecture", but I finish it I will be able to reuse my field between models and projects, just mentioning the class.

[ricardograna]

Seeing the PoL code, now I agree to have safeAttributes inside rules (actually, it IS a rule), but don't for attributes labels. Sentinel idea is good, too.

But, if you merge the methods, Yii wiil have backward compability for our attributesLabels and safeAttributes methods?

[jonah]

@Sentinel

That sounds just like what I was brainstorming the other day:
http://www.yiiframew...83.html#msg5083

[Sentinel]

Quote

@Sentinel



That sounds just like what I was brainstorming the other day:

http://www.yiiframew...83.html#msg5083

yes, yes!!

I didn't knew that this thread is there!! 

You were right that attributes should be stored in one direcotry and just assigned to the models. That way they can be easilly reused... I call it "attributes repository" and I use a class - factory to create and assign them... (and I have a lot of work on this...

[aztech]

My vote: rules and safe attributes => yes, rules and labels => no.
From my point of view, usually, when I define rule for attribute it is safe.
I do not like mixing labels with rules, they are something different. Second reason is that split into more methods increase code readability. I know (with a one look) what is what.

[notzippy]

I like the idea behind these functions but a more unified approach would be better. For example rules have an array key of "on" to define the scenario where safeAttributes define the scenario as the key then an array of values.

I vote to combine this into a single call. (Perhaps you would want specific labels for a scenario so that should be added to the map also. )

But if this is the case then it would also be nice to have "scenario merging" and "role based scenarios" also.

"scenario merging" : Be able to define a default scenario which is applied to all the other scenario's. This should allow you (for example) to define an attribute label for every other scenario if you dont need to change it.

"role based scenarios" : Be able to define and redefine a scenario that may be modified based on the users role. This should simplify the work of a controller in assigning a specific scenario based on a users role.

So the map would be
"<scenario>:<role>=>array("attributes"=>array(),"rules"=>array(), "safeAttributes"=>array())"

Where "<scenario>" may be blank (meaning applied to all scenarios)
and "<role>" may be * all users, ? guest users , "rolename"

So you could do

return array(

  // Any user

  ":*"=>array("attributes"=>array("name"=>"Label"),

                       "rule"=>array(rules for all users),

                       "safe"=>array(safe for all usrs)),

  "submit:admin"=>array("attributes"=>array("name"=>"Admin Label"),

                       "rules"=>array(rules for admin users),

                       "safeAttributes"=>array(safe for admin usrs)),

)...

I do like the ideas behind an attribute class but i think that would only take you part of the way..

Just some thoughts @3 in the morning..

nz

[olafure]

Vote for rules and safeAttributes merge.  Adding labels too might make it messy ?

If all there are merged, it could look something like this:

function attributes() {

  array(

    'name' =>

      array('rules' => 

        ... bunch of rules ... 

      array('label' => 'Name',

      array('safe') => true'),

... which is OK (a bit like the configuration file), but is it simple enough for people to learn ?

[dalip]

I very much like Sentinel's idea. Creating a class for every field with some base classes for strings (with email, telephone number as children), files, datetimes. These classes would handle the validation etc. The syntax in the model file could be used by some factory method to instantiate those classes. It would be much like http://docs.djangopr...ds/#field-types

It would make form generation a piece of cake.

For the model I'd move the 'safe' attribute to the rules() method (or rename rules() to properties(). The attributeLabels I'm indifferent over.

It's a pity the php folks didn't add the alternate $array=[1=>'a',2=>'b'] syntax, it would have made stuff clearer.

[qiang]

What about performance if we represent each attribute in terms of an object?

[jonah]

I very very much like the idea of attributes as classes to, and I agree with qaing, the only question is performance. The only way to find that out really is to benchmark it.

Say you are retrieving 20 objects from a DB, and each object has 3 attributes as classes (not all the attributes necessarily need to be classes), that instantiating 60 objects in all.

Doesn't this already happen when you have many behaviors attached?  I don't think the performance effect will be much different from the behavior feature.  In fact, as the way I am looking at it, attributes as classes is not all that much different from behaviors (of course there are some key differences though).

Perhaps a model should be benchmarked with 5 or so behaviors attached, and if that's ok, I think attributes as classes should be ok

If this is implanted I highly recommend django's documentation on their implementation should be carefully read first, as they have some very good ideas that should be borrowed.

Of course djando is Python and not PHP, so just because it works for them does not necessarily mean it would work well for a PHP framework

[qiang]

I think it's fine each AR object only has a few attribute objects. What dalip suggests seems to make every attribute an object, which is too much, I think.

[Jack]

having a rule which specifies what attributes are safe and on what scenario i think could be a good idea because it stays simple.

But I would prefer having a separate function for labels so i can return a different array based on language

[olafure]

Each attribute as object is over the top, I'd say.

Could ActiveRecord perhaps be designed in such a way that it would be possible to extend it to have such attributes ? Just thinking out loud.  People could create their own or CFancyRecord would be shipped along with Yii

[jonah]

@olafure

only attributes that are defined as classes would behave as so I believe (thus backwards compatibility most likely).

[dalip]

IIRC the columns as classes can already be established using behaviours and thus would be backwards compatible. The same goes for a lot of stuff since behaviours are very powerful.

[pestaa]

I dislike the idea.

Have a look at the official documentation. It suggests not to add fields like 'permission' to safeAttributes, since that would be make it possible to massively assign its value.

I don't want my site to be hacked via a simple form just by adding a new field permission=admin.

Or, after merging the two (or three) methods, will it be still possible not to allow certain properties to be massively assigned?