Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: behaviors refactoring #2360

Closed
wants to merge 1 commit into from
Closed

Conversation

@lucianobaraglia
Copy link
Contributor

@lucianobaraglia lucianobaraglia commented Feb 7, 2014

See #2269 and https://gist.github.com/lucianobaraglia/8d184cd9b9c7505a0b24

Usage example:

public function behaviors()
{
    return [
        // arbitrary attributes and values
        'attributestamp' => [
            'class' => 'yii\behaviors\AttributeStampBehavior',
            'attributes' => [
                \yii\db\ActiveRecord::EVENT_BEFORE_INSERT => [
                    'expires_at' => function() {
                        return date('Y-m-d H:i:s', strtotime("+30 days"));
                    },
                    'created_by_name' => isset(\Yii::$app->user->identity) ? \Yii::$app->user->identity->username : 'n/a',
                ],
            ],
        ],
        // default implementation, sets `created_at` and `updated_at` with `date('Y-md H:i:s')`
        // also, `format` could be configured here
        'timestamp' => [
            'class' => 'yii\behaviors\TimestampBehavior',
            'format' => 'Y-m-d',
        ],
        // this will fill `created_by` and `author_id` with the behaviors default value on insert
        // and `updated_by` on update
        'blameable' => [
            'class' => 'yii\behaviors\BlameableBehavior',
            'attributes' => [
                \yii\db\ActiveRecord::EVENT_BEFORE_INSERT => [
                    'created_by',
                    'author_id',
                ],
                \yii\db\ActiveRecord::EVENT_BEFORE_UPDATE => [
                    'updated_by',
                ],
            ],              
        ],
    ];
}
$model = $this->owner;
$attributes = $this->attributes[$event->name];

if ($attributes !== null) {

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

Why is this check needed? Probably use the following code:

if (!empty($this->attributes[$event->name])) {
    foreach ($this->attributes[$event->name] as $key => $val) { ...

This comment has been minimized.

@lucianobaraglia

lucianobaraglia Feb 7, 2014
Author Contributor

Right...I just have the habit of give that kind of properties a shorter name...
Will wait for more comments from you to refactor...

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

The check above also ensures attributes has the key $event->name, even though we are almost certain.

* By default, AttributeStampBehavior won't fill anything unless you specify and customize the events, attributes and the data.
*
*/
public $attributes = [];

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

From your design, there are three layers of assigning values to attributes, with one taking precedence over the other in the order they are listed below. They should be clearly explained:

  • value specified via attributes
  • value specified via attributeValue
  • value specified via defaultValue

This comment has been minimized.

@lucianobaraglia

lucianobaraglia Feb 7, 2014
Author Contributor

The idea behind this is that only AttributeStampBehavior allow setting values for each attribute.
In TimestampBehavior and BlameableBehavior the defaultValue is always used instead.
Check for processValue() in each class...

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

Since this is a base class, why making this limitation?

This comment has been minimized.

@lucianobaraglia

lucianobaraglia Feb 7, 2014
Author Contributor

Sorry...I am lost...what limitation?

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

I mean your base class can support attribute value stamping via the three layers described above. Why do you want to hide them?

* If `Closure` is use instead, the return value of the anonymouse function will be used.
*
*/
protected $attributeValue = null;

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

Perhaps attributeValues or simply values, to be consistent with attributes?

This comment has been minimized.

@lucianobaraglia

lucianobaraglia Feb 7, 2014
Author Contributor

Same as above, this is only used in internally in AttributeStampBehavior::stampAttribute().

* Each child class should override this value, and maybe in the `init()` implementation if the vlaue need some process.
*
*/
protected $defaultValue = null;

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

Perhaps defaultValues?

This comment has been minimized.

@lucianobaraglia

lucianobaraglia Feb 7, 2014
Author Contributor

My idea is that only one value is used for all attributes in child classes...
If one needs to give different values, should use AttributeStampBehavior.
Having this property in this class is only an indicator...

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

You're right.

if ($attributes !== null) {
foreach ($attributes as $key => $val) {
if (is_numeric($key)) { // e.g. `['attribute1',]`
$attribute = $val;

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

In this case call processValue($val) directly and assign it to $model->$attribute

if (is_numeric($key)) { // e.g. `['attribute1',]`
$attribute = $val;
} else { // e.g. `['attribute1' => 'val1',]`
$attribute = $key;

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

In this case directly calculate the value of the attribute and assign it to $model->$attribute.

This comment has been minimized.

@lucianobaraglia

lucianobaraglia Feb 7, 2014
Author Contributor

But what if the value needs some clean-up?

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

I don't quite understand this use case. Do you have a concrete example?

This comment has been minimized.

@lucianobaraglia

lucianobaraglia Feb 7, 2014
Author Contributor

Let's say you extend AttributeStampBehavior and make your own validator.
You can receive direct values in any format, for example:

public function behaviors()
{
    return [
        'mybehavior' => [
            'class' => 'app\behaviors\MyBehavior',
            'attributes' => [
                \yii\db\ActiveRecord::EVENT_BEFORE_INSERT => [
                    'some_attribute' => [1,2,3],
                ],
            ],
        ],
    ];
}

and then in your validator class, you can use the processValue() method to for example, serialize the array...

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

It seems we have different understanding of the attribute stamping. In my previous comment, I listed three layers of configuring attribute values.

  • via attributes: this has the highest priority, it configures attribute value for specific event
  • via attributeValue: it configures attribute value only by its name
  • via defaultValue: it configures all attributes listed in attributes and has the lowest priority.

The processValue() method is something not belonging to any of the above layers. It's a postprocessing step. I do not know if this is needed, or what is the best way to support this (you certainly don't want to override this method in most cases).

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

IMO, postprocessing of attribute value doesn't belong to this behavior.

// this need to be done because some child classes won't receive `$val` to process
// that classes only will use `$defaultValue`
$this->attributeValue = $val;
$model->$attribute = $this->processValue();

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

These two lines are not needed.

This comment has been minimized.

@lucianobaraglia

lucianobaraglia Feb 7, 2014
Author Contributor

As I said...some behaviors could need clean the value up...
Don't you think?

* @param mixed $value
* @return mixed the value after processing
*/
protected function processValue()

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

There is no value to be processed. How about calculateAttributeValue() or something similar?

{
parent::init();
$value = isset(\Yii::$app->user->id) ? \Yii::$app->user->id : null;
$this->defaultValue = $value;

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

Need to check if defaultValue is set or not.

This comment has been minimized.

@lucianobaraglia

lucianobaraglia Feb 7, 2014
Author Contributor

defaultValue is setted per class...
You could do something like public $defaultValue = 123; or use the init() method if that value need some processing (checking user, for example)...

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

Yes, I mean without checking, you may overwrite the value set via public $defaultValue =123;

ActiveRecord::EVENT_BEFORE_UPDATE => ['updated_at'],
];

public $format = 'Y-m-d H:i:s';

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

I don't think this property is needed. There are basically two kinds of time columns:

  • a column of datetime or timestamp type
  • a column of integer type recording the UNIX timestamp value

It would be great if we could detect the above two cases and act accordingly.

This comment has been minimized.

@lucianobaraglia

lucianobaraglia Feb 7, 2014
Author Contributor

Yes...I agree if this doesn't bring too magic...

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Feb 7, 2014

Ok, I'm done with the code review. Thanks!

/**
* AttributeStampBehavior is used to fill an ActiveRecord's object attributes with arbitrary data when specified events occurs.
*
* AttributeStampBehavior is mainly used as a base class for other behaviors, like `TimestampBehavior` and `BlameableBehavior`.

This comment has been minimized.

@samdark

samdark Feb 7, 2014
Member

AttributeStampBehavior is mainly → It is mainly

* AttributeStampBehavior is used to fill an ActiveRecord's object attributes with arbitrary data when specified events occurs.
*
* AttributeStampBehavior is mainly used as a base class for other behaviors, like `TimestampBehavior` and `BlameableBehavior`.
* Check `yii\db\BaseActiveRecord` for available events.

This comment has been minimized.

@samdark

samdark Feb 7, 2014
Member

Is it able to work with non-AR models?

This comment has been minimized.

@lucianobaraglia

lucianobaraglia Feb 7, 2014
Author Contributor

In theory it shoud work with anything that implement the extends from Component.
Should be something like AR Behaviors?

This comment has been minimized.

@samdark

samdark Feb 7, 2014
Member

Well, then it's just docs that are limiting its use to AR. I think it may be fixed.

class BlameableBehavior extends AttributeStampBehavior
{

public $attributes = [

This comment has been minimized.

@samdark

samdark Feb 7, 2014
Member

Need

/**
 * @inheritdoc
 */

This comment has been minimized.

@samdark

samdark Feb 7, 2014
Member

Same for other docs that are the same as in base class.

This comment has been minimized.

@lucianobaraglia

lucianobaraglia Feb 7, 2014
Author Contributor

Yes, I din't want to document too much because needed your comments and changes before...

use Yii;
use yii\db\ActiveRecord;

class BlameableBehavior extends AttributeStampBehavior

This comment has been minimized.

@samdark

samdark Feb 7, 2014
Member

Class header missing. Same in all files.

@lucianobaraglia
Copy link
Contributor Author

@lucianobaraglia lucianobaraglia commented Feb 7, 2014

Well...after review I don't think if this could be helpful.
You are free to take some of it or not and make the changes you think...
If this is merged, please remember that original TimestampBehavior was a work of @creocoder...

Let me know if I can help in anything... 😃

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Feb 7, 2014

after review I don't think if this could be helpful.

Do you mean the three layer thing?

@lucianobaraglia
Copy link
Contributor Author

@lucianobaraglia lucianobaraglia commented Feb 7, 2014

after review I don't think if this could be helpful.

Do you mean the three layer thing?

I mean the entire refactor...
Is this bringing some benefits?

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Feb 7, 2014

Sorry if my code review made you feel frustrated. I think the refactoring still makes sense. I will come back to this later when I get time.

@lucianobaraglia
Copy link
Contributor Author

@lucianobaraglia lucianobaraglia commented Feb 7, 2014

@qiangxue never ever, I am not that kind of developer!!! 😃
I appreciate your time!
For me, having my code or ideas taken in a professional framework is still a weird idea! So no problem, I am learning a LOT just having you guys reviewing my code...
But please, at least don't post it in http://thedailywtf.com/Series/CodeSOD.aspx 😐

@samdark
Copy link
Member

@samdark samdark commented Feb 7, 2014

@lucianobaraglia your code isn't bad. We just suggest how to make it better.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Feb 7, 2014

Agree with @samdark.

BTW, personally I don't quite like the three layers thing because it's a bit over designed and may confuse users without very good explanation and understanding.

@lucianobaraglia
Copy link
Contributor Author

@lucianobaraglia lucianobaraglia commented Feb 7, 2014

@samdark I know...thanks!
@qiangxue is the only way I came with that allows the AttributeStampBehavior give that freedom.
The child classes are only pre-configured types of AttributeStampBehavior, that have it's own default value (and don't allow setting anything else).
I guess -one of the- problem is in stampAttribute in child classes, where I unnecesary check for values that won't be used.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Feb 19, 2014

ok, so if we have this behavior, should not we have then behavior for is_deleted attribute for example? would like to.

@lucianobaraglia
Copy link
Contributor Author

@lucianobaraglia lucianobaraglia commented Feb 19, 2014

@Ragazzo with this approach several automatic attributes stamp could be implemented easily extending AttributeStampBehavior...
I think in VersionableBehavior, for example... and other more specifics...like SoftDeleteBehavior, etc...

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Feb 19, 2014

yes, but if so, then maybe they should go to yii2-behaviors extension.

@lucianobaraglia
Copy link
Contributor Author

@lucianobaraglia lucianobaraglia commented Feb 19, 2014

@Ragazzo would be nice!
It would be great if you can read all the discussion and help with the @qiangxue and @samdark ideas about this all (and I will learn from all of you 😃 )!

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Feb 19, 2014

i am fine with their suggestions, however some general behaviors are good, and soft-delete is one of them. the question is that should that be yii2-behaviors extension or not.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Feb 19, 2014

I don't think we need to put them in an extension. We usually create an extension if it has some external dependencies. Otherwise, separating them in an extension only brings trouble to users. The benefit of reducing code size is negligible.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Feb 19, 2014

ok, so what is for soft-delete behavior? it is needed same time as other for updating time and blameable behavior.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Feb 19, 2014

Yes, it's good to have.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Feb 19, 2014

ok, @lucianobaraglia can you add behavior for soft-delete? thanks)

@lucianobaraglia
Copy link
Contributor Author

@lucianobaraglia lucianobaraglia commented Feb 19, 2014

@Ragazzo this PR was just a refactoring proposal that's still far to be implemented beacuse code has to be improved, I think...
BTW, just for reference, the list of behaviors that comes to my mind:

  • AttributeStampBehavior
  • TimestampBehavior
  • BlameableBehavior
  • VersionableBehaior
  • SoftDeleteBehavior

Some other...?

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Feb 19, 2014

first two are not the same? and for what is version...?

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Feb 19, 2014

What about TranslatableBehavior?

@lucianobaraglia
Copy link
Contributor Author

@lucianobaraglia lucianobaraglia commented Feb 19, 2014

AttributeStampBehavior is for custom attributes and data stamp...see example here in the first comment...
And Versionable is just taken from Doctrine Behaviors, and works adding a version number each time a record is changed...

@lucianobaraglia
Copy link
Contributor Author

@lucianobaraglia lucianobaraglia commented Feb 19, 2014

@qiangxue it would be amazing, but I think maybe it's out of the scope of this refactoring idea...

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Feb 19, 2014

Translatable? yes, better finish this PR as @lucianobaraglia said. @lucianobaraglia can you open then separated issue with desired behaviors list?

@lucianobaraglia lucianobaraglia mentioned this pull request Feb 19, 2014
4 of 7 tasks complete
@qiangxue qiangxue closed this in 02d5b20 Feb 20, 2014
@cebe cebe modified the milestones: 2.0 GA, 2.0 Beta Feb 20, 2014
cebe added a commit that referenced this pull request Feb 20, 2014
* 'master' of github.com:yiisoft/yii2:
  Fixes #2360: Added `AttributeBehavior` and `BlameableBehavior`, and renamed `AutoTimestamp` to `TimestampBehavior`
@lucianobaraglia lucianobaraglia deleted the lucianobaraglia:wip-behaviors-refactoring branch Feb 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.