[WIP] Extract `Object` and `Component` into traits #12929

rob006 · 2016-11-06T11:58:59Z

Q	A
Is bugfix?	no
New feature?	yes
Breaks BC?	no
Tests pass?	yes
Fixed issues	#9748

This is only proof of concept, it still need adjust naming and fix descriptions.

Closes #9748

samdark · 2016-11-06T12:09:19Z

I don't think Object::className() is useful since we're raising PHP requirements and there's going to be native ::class support.

samdark · 2016-11-06T12:12:22Z

Makes sense to me overall. Do you think it worth also separating events and behaviors?

rob006 · 2016-11-06T12:27:12Z

This could be merged into 2.0.x IMO. And if we want to prepare to drop Object and Component and propose an alternative to deprecated Object and Component it should provide Object::className() implementation too.

Do you think it worth also separating events and behaviors?

Yes, but I'm not sure if there is a clean way to do this.

samdark · 2016-11-06T12:38:38Z

@yiisoft/core-developers need your opinions.

dynasource

this proof of concept looks good. I approve the split in traits. I agree with @samdark that we should also split the Events & Behaviors. Those two look less entangled as it first seems. Isnt it possible to create BehaviorTrait.php and a EventTrait in which the EventTrait includes BehaviorTrait.

With respect to className(), that one should stay in Component.php with a @deprecated mark. In 2.1 it will be removed.

dynasource · 2016-11-07T21:50:35Z

framework/base/ComponentTrait.php

+    }
+
+    /**
+     * Returns a list of behaviors that this component should behave as.


I see duplication with the @docs of the ComponentInterface. The docs in the traits can be replaced with @inheritdoc tags.

How IDEs would react to that?

Using @inheritdoc in trait is incorrect, since traits doesn't handle any inheritance. It even break yii2-apidoc as far as I remember.

Well, most important is not to have duplicated @docs hanging around. Its hard to keep these synchron.

Of course we can choose not to include the tag. Then an approach like https://www.drupal.org/node/2206175 is favorable. So we should have a method doc starting with a 'Implements method...' line and a @see tag to this method.

This gives you completely useless IDE hints:

interface FooInterface { /** * Interface description. */ public function bar(); } trait FooTrait { /** * Implements FooInterface::bar(). */ public function bar() { echo 'bar'; } } class Foo implements FooInterface { use FooTrait; }

As I said:

and a @see tag to this method.

It doesn't change anything - you still doesn't have any real documentation, only references.

Yeah. References are not enough...

Is there a way to re-structure it the way not to duplicate docs and having IDE hints at the same time at all? I don't see a way to do it.

clicking on the reference can be made quite obvious

rob006 · 2016-11-07T23:39:49Z

Isnt it possible to create BehaviorTrait.php and a EventTrait in which the EventTrait includes BehaviorTrait.

Doesn't should be opposite? BehaviorTrait includes EventTrait?

With respect to className(), that one should stay in Component.php with a @deprecated mark. In 2.1 it will be removed.

I that case we can't say "Don't use deprecated Object and Component, use new traits" because they're missing one of main features of Object.

dynasource · 2016-11-08T08:20:09Z

Doesn't should be opposite? BehaviorTrait includes EventTrait?

The event trait needs php->ensureBehaviors() https://github.com/yiisoft/yii2/pull/12929/files#diff-9c14d263e64e7854e2ef80847ef021b5R397, not the other way around

I that case we can't say "Don't use deprecated Object and Component, use new traits" because they're missing one of main features of Object.

Is that a problem? People can still be advised to use the new traits? And if they want to use the current className(), they still can. Its required for merging in 2.0.x/

rob006 · 2016-11-08T08:29:43Z

The event trait needs php->ensureBehaviors()

Behaviors uses Events: https://github.com/yiisoft/yii2/blob/master/framework/base/Behavior.php .

Is that a problem?

If we are deprecating Object::className() without any alternative, this is a problem.

dynasource · 2016-11-08T08:46:30Z

Behaviors uses Events: https://github.com/yiisoft/yii2/blob/master/framework/base/Behavior.php .

Another entanglement. Lets fix this in another PR and keep this one clean.

If we are deprecating Object::className() without any alternative, this is a problem.

it is deprecated in 2.0.x but it still can be used
it will be removed in 2.1 and developer have to use php ::class (as discussed in other topic)

samdark · 2016-11-08T11:06:32Z

@rob006 isn't PHP 5.6 MyClass::class an alternative to Object::className() for most cases?

klimov-paul · 2016-11-08T11:30:25Z

framework/base/MutatorsTrait.php

+ * @author Robert Korulczyk <robert@korulczyk.pl>
+ * @since 2.0.11
+ */
+trait MutatorsTrait


I am unsure about the naming.
Better be VirtualPropertyTrait

klimov-paul · 2016-11-08T11:42:27Z

Alternative approach for #7936

Dispite the change itself can be useful, this PR does not solves issue #7936 anyhow. In case object becomes a PHP reserved word the Yii core code will still crash.

I am sure we have a disscussion for such improvement somewhere - but I can not find it.
Actually related issue is #9748.

klimov-paul · 2016-11-08T11:51:19Z

Related disscussion:
#1837 (comment)

rob006 · 2016-11-08T11:58:17Z

@klimov-paul We can't really fix #7936 in 2.0.x, but extracting Object and Component into traits will make it pretty easy in 2.1. The only reason to leave Component and Object in this PR is BC.

@samdark You can't use MyClass::class in PHP 5.4, so this is not an alternative for Yii 2.0. IMO it is better to create another trait with className() implementation than just leave it in Object.

samdark · 2016-11-08T12:01:04Z

Right. I agree on another trait for className().

klimov-paul · 2016-11-08T12:05:39Z

We can't really fix #7936 in 2.0.x, but extracting Object and Component into traits will make it pretty easy in 2.1

Then issue #7936 should not be placed under 'Fixed issues' section as it produces a delusion.

The only reason to leave Component and Object in this PR is BC.

Disagreed: Component and Object should not be dropped as constant usage of traits and interface implementation is not practical.
Compare:

class Foo extends \yii\base\Object
{
}

with:

class Foo implements \yii\base\Configurable, \yii\base\MutatorsInterface
{
    use \yii\base\ConfigurableTrait;
    use \yii\base\MutatorsTrait;
{
}

klimov-paul · 2016-11-08T12:10:53Z

The side effect of this PR is sugnificant increase of entities in class herarchy: additional interfaces and traits. It makes the source code a little bit twisted.

It would be nice to have @qiangxue opinion on this matter - he was the one insisted inheritance tree should be as simple as possible.

Extracting a traits for Object and Component functionality makes sense only for the '3rd party' library, which may use them to integrate some standalone classes into Yii in easy way.
However, in general this initiative was refused as DI supports object creation and configuration via Closure callbacks.

klimov-paul · 2016-11-08T12:12:16Z

One more related disscussion: #12021

dynasource · 2016-11-08T12:39:50Z

I agree with not deprecating Component & Object for clean & easy inheritance (with those traits).

I do not agree with keeping the current situation:

perhabs its twisted because of the trait & interface combination, but such a combination is something to get used to. Btw., its already getting more and more common in practice.
this traits approach makes the software more modular and cleaner organized
this traits approach make the software more logical: currently Component is uselessly overriding methods of Object. This should be done cleaner (I now see in this PR that Component is still overriding Object. I expect this is for BC reasons?)
like you said, the software will be more flexibile to integrate in 3rd party software

samdark · 2016-11-10T14:15:13Z

@klimov-paul no. We won't compose each core class using traits. That's more for user classes which can't inherit from Object but need properties and behaviors at the same time.

klimov-paul · 2016-11-10T14:38:14Z

We won't compose each core class using traits. That's more for user classes which can't inherit from Object but need properties and behaviors at the same time.

Then what do you mean by telling:

Classes are still there because we need backwards compatibility.

?

samdark · 2016-11-10T14:47:37Z

I've meant that that there will be no changes about core classes in 2.0. In 2.1 for some classes it potentially could make sense to remove all properties and either leave constructor only or create setters without defining properties. In many cases behaviors aren't meant to be attached as well. For these cases one may either not use traits at all or use one of them only.

rob006 · 2016-11-12T15:39:44Z

This is more problematic than I expected. Due some limitations in handling traits and interfaces in PHP 5 (sometimes PHP 5 does not take into account used traits when checking interface implementation, see https://3v4l.org/JWXqX ), we have 3 options:

Do no implement BehaviorsInterface in Component (or don't use interfaces at all, like @klimov-paul requested)
https://github.com/rob006/yii2-dev/blob/aba6d8b9e83597565efec4023740eed8e2ad787c/framework/base/Component.php#L101 => https://travis-ci.org/rob006/yii2-dev/builds/175317588
Use some ugly workaround in Component
https://github.com/rob006/yii2-dev/blob/f8ef6ae8c35145f013eb9af03ed8180a78dd84df/framework/base/Component.php#L101 => https://travis-ci.org/rob006/yii2-dev/builds/175317203
Do not extend Component from Object (breaks BC)
https://github.com/rob006/yii2-dev/blob/955019c90bf95b6a0ff9d6d09260ef1afc7661d5/framework/base/Component.php#L101 => https://travis-ci.org/rob006/yii2-dev/builds/175314851

samdark · 2017-07-21T12:01:29Z

I'd like to have it in 2.1.

dynasource · 2017-07-21T12:21:42Z

me too

cebe · 2017-07-22T21:30:45Z

I don't like splitting the base classes of all classes in Yii into multiple traits, this will blow up the minimum overhead added by the framework, as instead of loading two classes (Object and Component) it would also need to include a set of traits and interface.

sergeymakinen · 2017-07-22T22:22:15Z

I don't think that 2 traits and an interface is a measurable issue in terms of performance. And even 10 traits and interfaces. Especially in a PHP 7 era.

cebe · 2017-07-22T22:30:57Z

Its not only performance its more about general complexity of the base. Yii philosophy is to keep things simple and practical. Separating the base class into multiple traits adds complexity in understanding it.
To understand what is going on in Yii you'd have to look into multiple classes, traits and interfaces and put all that together to get the idea of what you are building on, while currently you have two classes and thats it.

What exactly are the practical benefits of such a separation?

sergeymakinen · 2017-07-22T23:58:35Z

I was about a performance. My thoughts about a PR usefulness are still controversial.

I love Yii's KISS. But I find VirtualPropertiesTrait useful. Because it provides things are common without a need to inherit BaseObject. EventsInterface and its trait are also handy when you need to accept an object implementing this thing without inheriting. TBH I would implement these changes and that's it.

On the other hand, as far as I understand the whole thing is about breaking inheritance chains. From this point I think this PR is a solid step forward.

samdark · 2017-07-23T14:16:08Z

Yes, it's about a step towards removing inheritance chain that's way too long.

rob006 · 2017-07-23T15:50:00Z

What exactly are the practical benefits of such a separation?

Extracting this logic into traits will allow to easily use events/behaviors/virtual attributes in third party classes.
Separating events and behaviors may increase performance in some cases - if you need only events, you can skip behaviors overhead by using only EventsTrait.
At least for me splitting Component into smaller traits focused on single feature, make it easier to read and understand.

dynasource · 2017-07-23T20:44:46Z

Perfectly summarized

cebe · 2017-07-24T11:45:00Z

Yes, it's about a step towards removing inheritance chain that's way too long.

in current state its making inheritance chain more complex.

Extracting this logic into traits will allow to easily use events/behaviors/virtual attributes in third party classes.

what kind of third party are you thinking of here? Which package would require yiisoft/yii2 just to use these traits?

Separating events and behaviors may increase performance in some cases - if you need only events, you can skip behaviors overhead by using only EventsTrait.

the overhead is marginal and having to decide every time you create a class does not really safe anything, it takes time to make a decision whether to include which kind of trait, and it may limit usage if you later decide to add some behavior. For extensions that means if an extension excluded behaviors you can not inject your functionality and start a discussion with the author instead of using existing functionality that is always there if needed. Having behaviors available by default makes Yii perfectly extensible in many ways by default. I have experienced this when working on HumHub, which is very flexible when writing extensions for it, behaviors allow you to inject a lot of functionality. I'd keep this in by default, arguing about performance here would result in the question whether you'd want to use a framework at all.

rob006 · 2017-07-24T13:14:07Z

what kind of third party are you thinking of here? Which package would require yiisoft/yii2 just to use these traits?

Any yii2 extension that provides integration with any existing non-yii2 library. Instead just wrapping everything you can extend library class, use these traits and interfaces and you will have yii2-like component.

the overhead is marginal and having to decide every time you create a class does not really safe anything, it takes time to make a decision whether to include which kind of trait, and it may limit usage if you later decide to add some behavior.

You already need to make decision whether to use Component, BaseObject or nothing as base for your class.
Component will still provide behaviors and events, and creating events-only class will be more complicated than just extend Component, so it probably will be used only if someone knows what he is doing and really need it. I don't think that framework should force using behaviors in this way.

rob006 · 2017-08-16T21:23:50Z

I'm closing this PR for now since after #14478 it needs to be redone from scratch. Please continue discussion in #9748 and let me know about your decision - I'm interested in to finish this in new PR if the idea gets a green light.

rob006 added 5 commits November 6, 2016 10:54

Create ConfigurableTrait.

fb95b26

Fix link to yii\base\Object in yii\base\Component description.

cf0b61c

Add yii\base\MutatorsTrait.

7977258

Add yii\base\ComponentTrait.

f65d5f2

Fix link in ComponentInterface phpdoc. [skip ci]

c2b9064

samdark added this to the 2.1.0 milestone Nov 6, 2016

samdark added the status:under discussion label Nov 6, 2016

samdark modified the milestones: 2.0.x, 2.1.0 Nov 6, 2016

samdark approved these changes Nov 6, 2016

View reviewed changes

rob006 mentioned this pull request Nov 6, 2016

Deprecate yii\base\Object #12699

Closed

dynasource suggested changes Nov 7, 2016

View reviewed changes

klimov-paul suggested changes Nov 8, 2016

View reviewed changes

rob006 added 3 commits November 11, 2016 23:46

Rename MutatorsTrait => VirtualPropertiesTrait.

c11ccb7

Extract EventsTrait and BehaviorsTrait.

2978648

Remove BehaviorsInterface from Component.

aba6d8b

rob006 mentioned this pull request Apr 1, 2017

Require PHP 7.1 for Yii 2.1 #11397

Closed

5 tasks

cebe modified the milestones: 2.1.x, 2.0.x Jul 14, 2017

samdark modified the milestones: 2.1.0, 2.1.x Jul 21, 2017

samdark self-assigned this Jul 21, 2017

rob006 closed this Aug 16, 2017

rob006 deleted the replace-object-by-traits branch August 16, 2017 21:24

rob006 mentioned this pull request Aug 16, 2017

Extract events from Component class into trait #9748

Closed

cebe removed this from the 2.1.0 milestone Aug 17, 2017

klimov-paul mentioned this pull request Sep 1, 2017

ensure backwards compatibiliy with migration without $compact #14745

Merged

[WIP] Extract Object and Component into traits #12929

[WIP] Extract Object and Component into traits #12929

Conversation

rob006 commented Nov 6, 2016 • edited Loading

samdark commented Nov 6, 2016

samdark commented Nov 6, 2016

rob006 commented Nov 6, 2016 • edited Loading

samdark commented Nov 6, 2016

dynasource left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samdark Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rob006 commented Nov 7, 2016

dynasource commented Nov 8, 2016

rob006 commented Nov 8, 2016

dynasource commented Nov 8, 2016 • edited Loading

samdark commented Nov 8, 2016

Choose a reason for hiding this comment

klimov-paul commented Nov 8, 2016

klimov-paul commented Nov 8, 2016

rob006 commented Nov 8, 2016

samdark commented Nov 8, 2016

klimov-paul commented Nov 8, 2016

klimov-paul commented Nov 8, 2016

klimov-paul commented Nov 8, 2016

dynasource commented Nov 8, 2016 • edited Loading

samdark commented Nov 10, 2016

klimov-paul commented Nov 10, 2016

samdark commented Nov 10, 2016

rob006 commented Nov 12, 2016

samdark commented Jul 21, 2017

dynasource commented Jul 21, 2017

cebe commented Jul 22, 2017

sergeymakinen commented Jul 22, 2017

cebe commented Jul 22, 2017

sergeymakinen commented Jul 22, 2017

samdark commented Jul 23, 2017

rob006 commented Jul 23, 2017

dynasource commented Jul 23, 2017

cebe commented Jul 24, 2017

rob006 commented Jul 24, 2017 • edited Loading

rob006 commented Aug 16, 2017

[WIP] Extract `Object` and `Component` into traits #12929

[WIP] Extract `Object` and `Component` into traits #12929

rob006 commented Nov 6, 2016 •

edited

Loading

rob006 commented Nov 6, 2016 •

edited

Loading

dynasource left a comment •

edited

Loading

samdark Nov 8, 2016 •

edited

Loading

dynasource commented Nov 8, 2016 •

edited

Loading

dynasource commented Nov 8, 2016 •

edited

Loading

rob006 commented Jul 24, 2017 •

edited

Loading