yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

Traits, behaviors and events #1053

Closed qiangxue closed 10 years ago

qiangxue commented 11 years ago

This is primarily a discussion, which may or may not result in design changes. But if the answer is positive, the change will be significant and we would like to push it into alpha release.

There are several related questions:

  1. Should we replace behaviors with traits? Which features of behaviors do you think are the most important?
  2. Is it possible to implement behaviors in terms of traits? @klimov-paul proposed an event mechanism for behaviors using traits here: https://github.com/yiisoft/yii2/pull/1042 The main advantage for using traits instead of behavior is performance.
  3. Should we support class-level events like suggested in https://github.com/yiisoft/yii2/issues/1025 ? Such kind of events could be useful if we want to use traits to implement behaviors. It does impact performance, however.

To all and @yiisoft/core-developers: thoughts and suggestions?

jjnavsofs0 commented 11 years ago

@qiangxue imo using traits would be an advantage as we already decided for php 5.4. Reasons:

  1. As you mentioned it has performance advantage.
  2. And if traits can be used as behaviors as @klimov-paul has suggested then all advantages of behaviors can be implemented through traits.
  3. Supporting class-level events is very useful and may come handy at times. And if we decide to go with traits for behaviors then performance issue will be it subsidised imo.

my 2 cents ;)

iJackUA commented 11 years ago

Events via traits and class-level events looks nice, as I see the biggest doubt is how to configure used traits in each usage case. Right now I see only a way to configure via object properties, so behavior attachment will be a little bit "distributed in code of your class" instead one config/init array as it is now.

if now we have

class MyController extends Controller
{
    public function behaviors() {
        return [
            'access' => [
                'class' => \yii\web\AccessControl::className(),
                'rules' => [
                    [
                        'actions' => array('login'),
                        'allow'   => TRUE,
                        'roles'   => array('?'),
                    ]
                ],
            ],
        ];
    }

will it be ?

class MyController extends Controller
{
use \yii\web\AccessControl;

public $accessControlOptions = [
                'ruleConfig' = ['class' => 'yii\web\AccessRule'],
                'denyCallback' => [$this, 'myDenyAction'],
                'rules' => [
                    [
                        'actions' => array('login'),
                        'allow'   => TRUE,
                        'roles'   => array('?'),
                    ]
                ],
            ];

or how could it look like ? (some naming convention for options var needed)

cebe commented 11 years ago
  1. Should we replace behaviors with traits? Which features of behaviors do you think are the most important?

The most important thing about a behavior is that it can be attached and removed dynamically. With traits this is not possible so imo traits and behaviors can co-exist and should be used for different purpose. An extension for example should be able to attach a behavior to a component. It would not be able to attach a trait as it would have to change the class code. Btw: traits are just copy and paste of code as said by ircmaxell:

At the moment I see traits as not much more then "compiler assisted copy & paste". ;): @Max: That's exactly what traits were designed to be, so that's completely correct. It makes it more "maintainable", since there's only one definition, but it's basically just c&p... -- ircmaxell on stackoverflow

samdark commented 11 years ago

1042

klimov-paul commented 11 years ago

I suppose both approaches should be available inside the framework. In some cases the behaviors are more suitable, and in others – the traits are.

I suppose these situations can be determined by simple rules:

1) For the classes, which unlikely will be extended by developers, like application itself and application components, the behavior mechanism is more suitable, because it allows to be attached via configuration. If I need to create my own application class to handle ‘beforeRequest’ event, I do not need traits or behaviors – I could do it just inline, overriding method ‘beforeRequest()’. I do not want to create 2 classes to handle the single event: one – the trait handler, and the one – new application class.

2) For the classes, which likely will be extended by developers like Models and Controllers the trait mechanism should be used. Since, I already created my own Active Record class as descendant of the base one, I can apply traits for it as well – it will save a performance without extra headache.

samdark commented 11 years ago

How much performance it will save?

klimov-paul commented 11 years ago

I am not sure about class-based events. I have to admit such feature could be usefull in some particular situations, but overall it seems to be not a good practice for me. It could make the program uneasy to understand and debug.

For example: Imagine you “inherit” a project from your team mate, who fell ill. You are trying to fix some strange problem, which sometimes appears on ActiveRecord ‘Item’ class saving. You have opened ‘Item.php’ searched its code and find nothing uncommon, then you find ‘Use::behaviors()’ method, which attaches 5 behaviors, you have searched these behaviors files and again find nothing… So after several hours you are finally find the class event attachment made inside some bootstrap file.

I do not like the idea the class behavior can be changed from the external code, which can be placed in God-knows-where.

If I want attach same handler for all my Active Record classes, I could just create my own base Active Record class, which will attach it, and then extend my models from this class.

samdark commented 11 years ago

Yes, generally it's a bit cryptic to debug such code. Still for some products like CMS it can be extremely useful.

klimov-paul commented 11 years ago

How much performance it will save?

Hard to tell, we need to compose performance tests to find out.

It is obvious that trait approach will save performance avoiding magic method calls for the properties and methods he adds to the owner. Also #1042 approach will save memory usage, because it does not need to store callbacks for event handlers for each object, which has a BehaviorTrait. For a hundred Active Record instances with several behaviors attached, this could be a tremendous saving of memory.

rawtaz commented 11 years ago

At this point I don't think traits should replace the behavior feature in Yii2.

I think that traits has more to do with code composition, and is great for that. But it isn't the same thing as behaviors, which is more about reusable components/parts of the application rather than traits. They're in different domains/levels of the architecture. I think @cebe summed it up pretty nicely.

Behaviors can take configuration, and we can add multiple "instances" of one and the same behavior with different configuration if needed (e.g. when a behavior isn't written so that it supports handling of multiple so one has to add it multiple times instead), I'm not sure one could do that with traits? It's also hinted in the need for UniqueSuffix for method names in PR #1042 that traits are generally of an "apply only once to a class" nature.

As someone else mentioned, the configuration of/for a trait would be spread out into the class rather than being nicely contained in the behavior's config. Or we'd need to have some name-conventional container for the config such as $accessControlOptions in what @iJackUA wrote. I just get a feeling that traits need to be somewhat shoehorned into the app in a way they're not really designed for, if we try to replace behaviors with them.

There has also been a discussion about being able to check, at runtime, which behaviors have been added to a class. Can this be done with traits, can one check if trait X is added/used, and can one enumerate traits? That discussion need actual use cases though.

qiangxue commented 11 years ago

Class-level events allows us to intercept code execution without the need to get the actual object first. For example, I may use it to add some special logging for every save operation of AR.

The most important thing about a behavior is that it can be attached and removed dynamically.

@cebe: Is there any use case for dynamically attaching/detaching behaviors?

I do not like the idea the class behavior can be changed from the external code, which can be placed in God-knows-where.

@klimov-paul In general event-driven code has similar problem. So this is not specific to class-based events.

How much performance it will save?

@samdark Behaviors are heavy in both time and space. This is especially obvious when they are used in classes such as AR. I don't have actual performance comparison between traits and behaviors, though.

As someone else mentioned, the configuration of/for a trait would be spread out into the class rather than being nicely contained in the behavior's config

@resurtm Both traits and behaviors actually "contaminate" the master class with their properties.

Can this be done with traits, can one check if trait X is added/used, and can one enumerate traits? That discussion need actual use cases though.

See this: http://php.net/manual/en/function.class-uses.php

mgrechanik commented 11 years ago

Hello everyone.

Class-level events allows us to intercept code execution without the need to get the actual object first

Class-level events sound good. But then there is the next question - how to detach this event handler from particular object? In the current model of behaviours we might be able to attach such Class-level handlers to an object via yii\base\Component::construct (we don't have this method yet) Checking which to assign with is a(), right? And detaching would be no problem in the current event architecture.

Is there any use case for dynamically attaching/detaching behaviors?

It is a very important feature. It looks like some analog of drupal hooks. And we know how powerful drupal is and these hooks is the hearth of it's architecture. We need to have some way to attach/detach events on the fly.

For example: I have some tree menu of categories in my database. I create some class M1 which preserves some metadata and this tree which it loads from DB to a multidimensional array (in some basic format) property. Using behaviours, I might be able to do a lot of different work with this tree:

But with static behaviours I see the reason of what you are proposing. For example for Access control filters. Though, as was mentioned by iJackUA, I argee there is still a question about how to configure this trait behavior. By convention about what propepties to use in class? But there might be a lot of different traits.. The Behaviour being an object of any type looks more advanced here again

But if there is a possibility of using trait behaviour in some case, why not? What if to the current implementation of yii\base\Component::trigger just add some additional check for "trait event methods" (using get_class_methods, as was proposed)? And so it would be like this: if traits are enough with some class, it uses them. In other case it uses advantages of behavior objects.

mgrechanik commented 11 years ago

to attach such Class-level handlers to an object via yii\base\Component::_construct (we don't have this method yet)

I will correct myself about this my suggestion because we start attaching behaviors in yii\base\Component::ensureBehaviors of course.

And another thing about Class-level events is this: If we define to attach some behaviors to class A, might we also want to have a way not to attach them to class A2 that is subclass of A?

qiangxue commented 10 years ago

Here is an implementation of AutoTimestamp using trait: https://gist.github.com/qiangxue/c130a1fd0880e3f12158

In general, the procedure to define and use a trait-based behavior is as follows:

Compared to our current way of implementing behavior, the main difference is that the above approach doesn't support dynamic attaching/detaching behaviors. However, so far I have yet to see a practical use case of this.

It seems to me the biggest drawback of trait is potential conflict of method/property names between the owner class and the traits. To solve this problem in a systematic way, perhaps a common prefix/suffix should be used for every trait (as a naming convention).

samdark commented 10 years ago

You mean one should prefix/postfix all variables and functions in a trait with the name of behavior?

rawtaz commented 10 years ago

I think due to the nature of traits this makes for way uglier code. It is no longer a pluggable piece of behavior, it's a trait. It's not the same thing. And the prefixing that is needed, that's just way ugly. I guess what I'm saying is that I don't like it, and I prefer clean code in general.

cebe commented 10 years ago

@cebe: Is there any use case for dynamically attaching/detaching behaviors?

When building a modular system we need a way to attach behaviors via config dynamically. When I install a module I want it to be able to modify application behavior to its needs without having to touch the code of an application component (when I disable module, app behavior should change back to normal without me having to touch the code). When I write in config

'response' => [
    'as myBehavior' => [/* ... */],
]

I can do this by using the original yii component class and extend it without the need for my own extended version of the class that adds a line of trait usage. If I have more than one module that wants to hook into a components behavior I would not be able to do it with traits because I have one extended component in Module1 and another in Module2. No way to add them both to application.

traits are nice for code duplication and may be used in places where we do not necessarily need behavior and benefit from performance gain. So imo both concepts can co-exist because they are not the same, they have something in common but use cases are very different.

AutoTimeStamp fits well in a trait while other behaviors may not.

qiangxue commented 10 years ago

@samdark, @resurtm: if both trait and class declare the same property but with different default values, PHP will report error. So adopting a prefix/suffix protocol is probably needed if we want traits to be truly self-contained. This is so far the biggest drawback of trait, as far as I can tell. This is not specific to trait-based behaviors.

@cebe When you dynamically attach/detach a behavior, what is the main use of the behavior? Is it only to attach event handlers?

klimov-paul commented 10 years ago

I don’t like @qiangxue’s implementation of the timestamp behavior via trait. I see following drawbacks in it: – It still uses callbacks to store event handlers, while it is not necessary for the trait: there is no reason why object should store self reference. This is just a waste of memory. Do you remember issue from Yii1: https://github.com/yiisoft/yii/pull/1426 ? – Such implementation requires manual invocation of ‘initXyz()’ method, which should be added by hand. If I would use trait based behavior, I would prefer its attachment to be simple mention it in the ‘use’ statement without any additional code. Although if we are planning to invoke ‘initXyz()’ automatically on ‘init()’ method via ‘magic’ code, this means this method is invoked always, even if trait does not handle any event.

May I ask: what exactly is wrong with #1042? It has been designed in the way: unless event is actually triggered, there would not be any additional calculations or memory reservations.

It seems to me the biggest drawback of trait is potential conflict of method/property names between the owner class and the traits. To solve this problem in a systematic way, perhaps a common prefix/suffix should be used for every trait

Do not forget about ‘instead of’ statement. If trait declares property or method, which should be overridden by its owner you can resolve the conflict using ‘instead of’ statement, allowing container class to declare this property on its own.

Indeed there is a problem of potential naming collisions, but it always been present around behavior feature.

When I have created behaviors for Yii1 I always kept in mind name collision problem. I do not see nothing ugly naming field ‘timestampAttributes’ instead of just ‘attributes’.

Here is some example to think about: Assume, I am creating a behavior for the Active Record, which should apply Nested Set model for it. I would like to add the ability to query by the tree axis like ‘parent’, ‘descendant’, ‘child’ and so on. I would like to add these axes as the ‘scopes’ methods. So, what should I do? Using traits I can do this easily: I just need to declare these methods inside the trait just like it was the Active Record itself. So while querying my Nested AR I would not loose extra time on calling for the magic methods. Now: what I need to configure? – 3 field names: ‘left_index’, ‘right_index’, ‘level’. Their values unlikely will be changed in the class, which will actually use my Nested Set trait. Also it is unlikely the same AR class will need more then one Nested Set behavior.

So imo both concepts can co-exist because they are not the same, they have something in common but use cases are very different.

Oh, they have MUCH MORE in common then you think.

Any trait can be easily transformed into behavior! Assume I have created TimestampBehaviorTrait which is trait. Now what if want it to be not a trait, but common behavior? – I do not need to copy-paste it. I can create new behavior using already existing trait:

trait TimestampBehaviorTrait
{
    public $timestampAttributes = []
    …
}
class TimestampBehavior extends Behavior
{
    use TimestampBehaviorTrait;
}

P.S. It seems you have forgotten why the behavior mechanism appeared in the first place. It has been created to replace the lack of multiple inheritance feature in PHP. Of course the behavior mechanism has it own benefits, that is why in the University I was told “multiple inheritance” is bad practice and can be avoided using composition. Now you have this opportunity, still it is hard for you to accept it. You should also remind some issues related to behaviors, which have risen at Yii1: https://github.com/yiisoft/yii/issues/598, https://github.com/yiisoft/yii/pull/1803 and others.

bwoester commented 10 years ago

I used behaviors to dynamically attach interfaces to classes I can't control (provided by yii or extensions). The behavior implements the interface and gets attached to the class at runtime. In this use case, the behavior basically acts as an adapter, forwarding calls to appropriate methods of the owner.

As an example, think about a REST extension. It provides IRestResource and IRestCollection interfaces. IRestResource defines CRUD operations, IRestCollection defines paging, adding and retrieving IRestResource instances. The extension only works on those interfaces, not on any specific implementation. A user of the extension can configure which class to use for each named resource.

The most straight forward way to implement a resource would be to use CActiveRecord, so the extension comes with an adapter behavior implementing the IRestResource interface which forwards CRUD operations to the AR instance.

So when invoked to work on a given resource, the extension first checks if the configured class for the named resource implements IRestResource. If it does, the class can be used as is. If the class itself does not implement IRestResource, the extension searches for a registered adapter. This way, the extension is able to work on all classes derived from CActiveRecord by attaching the behavior to them that implements the required interface.

qiangxue commented 10 years ago

@klimov-paul The proposed timestamp behavior implementation is mainly a proof of concept thing. It is provided to trigger more in-depth discussion like you just did.

Do you remember issue from Yii1: yiisoft/yii#1426 ?

This issue is about base behavior class attaching event handlers that are not really implemented by child classes. The situation here is different. Also, since PHP 5.3, the circular reference memory issue is resolved: http://www.php.net/manual/en/features.gc.collecting-cycles.php So the memory isn't a big concern, even though the proposed approach does use more memory than yours. But again, this is just another proposal.

Such implementation requires manual invocation of ‘initXyz()’ method

I introduced initXyz() mainly because I want behaviors to have a life cycle that would allow them to do some initialization work. Yes, it has the drawback that the method will always be called whenever the owner class is instantiated. This is acutally like what Yii 1.1 is doing with behaviors.

what exactly is wrong with #1042?

It is an alternative solution too. It has the performance advantage you have described. Its drawback is that it is a bit automagical, and it only addresses the event handling aspect of behaviors.

Indeed there is a problem of potential naming collisions, but it always been present around behavior feature.

The issue of name conflict in traits is much more severe than that in behaviors, which almost makes traits not truly usable. For example, if trait A and B both define private property $a but with different default values, you will not be able to use both A and B in the same class because PHP will report an error. The deeper issue is that A and B are likely to interfere with each other without actually knowing that. Please correct me if I understand traits incorrectly.

With the above discussion and investigation, I'm actually leaning towards keeping the current behavior implementation.

klimov-paul commented 10 years ago

With the above discussion and investigation, I'm actually leaning towards keeping the current behavior implementation.

At the beginning of this discussion I have already said:

I suppose both approaches should be available inside the framework.

I suppose idea of keeping the original behavior mechanism is out of questioning – it would be kept.

Still the trait behavior solution should be provided on one way or another. We have already seen it is possible. This means it is already done. Even if we drop the idea of supporting behavior via trait and will insist developers should not do it – it would not help. The trait based behavior will be just the first extension community will create. It does not matter if anyone like it or not – it can’t be undone. All we can do at the moment is providing the clean (or more or less clean) core based solution for it.

To create trait based behavior solution, first of all, we need to think fresh: you should reconsider the whole problem and design from the beginning – not just hold on the current implementation. We should reconsider the event handling: how it should be done in case we do not need extra object to contain the mixing.

I have already shown you that any trait can be easily converted into a normal behavior. So if anyone have doubts should particular mixing been implemented via trait or behavior, he can simply support both without extra efforts. But there is no way the behavior can be composed into a trait.

qiangxue commented 10 years ago

Still the trait behavior solution should be provided on one way or another. We have already seen it is possible. ... But there is no way the behavior can be composed into a trait.

You get contradicting conclusions. :)

To create trait based behavior solution, first of all, we need to think fresh

Yes, that's what I wanted to do. I wanted to drop some non-essential features of behavior to make this possible. But unfortunately this is not really true as shown in the above discussion. A behavior is self contained, can be dynamically attached/detached and has its own life cycle. Once you turn a behavior into a trait, you lose one or more such features. Therefore, introducing the so-called "trait-based behavior" would cause more confusion.

The event mechanism you proposed is a way to support part of the behavior features. It is more like introducing a convention of writing event handlers without actually attaching it. I'm not sure if we should support it. It's a bit too automagical to me. Should a user really wants to use events in a trait, there is also the alternative solution I described (without the need of the framework to introduce any new convention.)

klimov-paul commented 10 years ago

To create trait based behavior solution, first of all, we need to think fresh.

Do you remember a task (or a problem), which resolving produced the original behavior mechanism? How it is sound?

qiangxue commented 10 years ago

The original intention of supporting behavior is to mimic the mixin support. There's no specific task/problem for it. Also features such as event attaching were not in the original implementation.

klimov-paul commented 10 years ago

The original intention of supporting behavior is to mimic the mixin support. There's no specific task/problem for it. Also features such as event attaching were not in the original implementation.

Traits in PHP is a mixin support. If now you will face the same task, how you would resolve it?

qiangxue commented 10 years ago

I know. I personally don't care about dynamic attaching/detaching feature. But the potential naming conflict of trait (even private names) makes me think it should only be used in limited places.

klimov-paul commented 10 years ago

I would let the end developer to decide what to do inside his project.

klimov-paul commented 10 years ago

The trait is an alternative, but not the replacement, for the origin behavior, and it is not a default feature. The end developer should choose whether to use trait or not.

At #1042 I composed the solution inside a trait, but not inside the ‘base/Component’ itself. It is a message: “if you want to use traits as behaviors, you should do it by your hand on your own risk – if you want this apply this trait to your class and go, but be ready for the consequences”.

iJackUA commented 10 years ago

@klimov-paul do you propose to make first all behavios that allows it as Trairs. But still leave a support of current behaviors. And if developer will need (to add additional configuration option or dynamic attach/detach etc.) he will just convert Trait into "Class extends Behavior" and will move on with it ? It could be mind-blowing thing for novices :) In any case "global" configuration of trait that will be mixed with own class params will create a big mess imo and will not add clearance to the code.

klimov-paul commented 10 years ago

@iJackUA, once again: – trait based behavior should not be default feature – it should be an option. – the end developer should choose, if he want to use them or not. – behavior traits will raise anyway, we simply can’t stop this. All we can do to create some canonical solution, compose the instructions and, of course, warnings about it. – if you start to create new behavior (like Active Record Nested Set) from the trait, you can compose it into descendant of ‘base/Behavior’ easily, thus supporting both approach, allowing the end developer to use the one he likes.

There is no perfect solution – everything has its drawbacks.

lucianobaraglia commented 10 years ago

@klimov-paul could both options live happily together? If so, I just would suggest to rename BehaviorTrait to TraitBehavior.

In the end, this could be not a community extension but an official one.

klimov-paul commented 10 years ago

That is not for me to decide

qiangxue commented 10 years ago

@lucianobaraglia What do you expect TraitBehavior to provide? If you only need mixin features, a trait is the thing to go. If you need other features such as dynamic attaching/detaching, configuration, event handling, you should use behaviors.

lucianobaraglia commented 10 years ago

@qiangxue I am more than happy with the current implementation of behaviors Being said that, it seems a lot of people likes the trait stuff ( I have NO experience using traits ).

cebe commented 10 years ago

@cebe When you dynamically attach/detach a behavior, what is the main use of the behavior? Is it only to attach event handlers?

In most cases it is event handlers but in case of AR for example it is very often used to add methods that help manipulate the record in some way. Can't remember that I had a behavior that did not use events though.

When behavior is only event handlers it is still better to have behavior instead of just attaching the event handlers as they are a kind of handler package that will be attached and detached all together so you can not forget one and get inconsistent behavior.

samdark commented 10 years ago

In 1.1 it was adding scopes but it now should be done with a trait.

qiangxue commented 10 years ago

Conclusion: we will leave the current behavior implementation as is. We may consider adding some documentation to compare traits and behaviors and giving guidance on when to use which.

I'm closing this issue. The support for class-level event is done: https://github.com/yiisoft/yii2/commit/8b00693a0a4e13ea07623f7a96f57d68bffbe416