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

Make Component like composite-pattern #503

Closed Ragazzo closed 10 years ago

Ragazzo commented 11 years ago

It would be great if we have ability to set components on some basic class extended from Component, so in this way it will be smth. like composite pattern, also @samdark do you remeber this suggestions from russian php forum? Any other thoughts of core-developers? In Yii1 i was missing this functionality, that i can't do smth. like this for example:

class MyComponent extends CComponent
{
}

$a = new MyComponent();
$a->setComponent($b);
$a->setComponents($config); //configure

I think this functionality can be cleared from Module and set to the Component class.

samdark commented 11 years ago

This one is about providing a facility to lazy-inject classes into your own custom component and configure its properties.

Ragazzo commented 11 years ago

What lazy-inject is? O_o I'm only talking about solution where we have ability to set components on other components.

samdark commented 11 years ago

Umm... Why can't you do it currently?

Ragazzo commented 11 years ago

Component class has no methods for this, all this now is in Module class. For example, when i create some component that has not so much business-logic to be called Module, but it must have other components, so in current implementation i need to copy this functionality from Module class every time.

Ragazzo commented 11 years ago

I am talking about this methods in Module class: getComponent, setComponent, hasComponent, getComponents, setComponents and maybe preloadComponents to load components when component is init.

qiangxue commented 11 years ago

Could you show some examples why you need this?

Ragazzo commented 11 years ago

unfortunately i cant show code, but i can describe situation (that i faced with), it smth. like this: i have to write configs in different ways, so i need for example to provide some metadata, so:

class FileSchema extends Schema
{
     public $routes = array(
         'firstRoute' => 'FirstRouteClass', //can be also array like needed for component creation
         'secondRoute' => 'SecondRouteClass',
         ....
     );
}

so this situation is very common, i need to be able to set this for example from configs, or i need to set routes with some methods, and thats why all this components methods i need, without them i need to write this functionality by myself again :( Also back for example, each route can provide different metadata (same case as above) format (1 class - 1 metadata type), and again i need to write some code by myself for this, because basic CComponent has no methods for this. Some other developers on ru-forume were also voting for this change.

samdark commented 11 years ago

@Ragazzo it's basically what you can do with components for Module but for your own component, right? An ability to have sub-components for your own component the same way Module does.

Ragazzo commented 11 years ago

But, i dont want to extend from Module class, i need a module component to have some sub-components (and also have the ability to set sub-components properties from config). In current implementation i cant do this, i only need to create Module or right all by myself (( Thats why i think this can be changed, also Module class will have better design after this refactoring. I think this is a good way to go.

qiangxue commented 11 years ago

@Ragazzo Your example is not quite the same as the Module case because you don't need lazy loading, and it's very easy to implement what you want: just define a setRoutes() method which will take an array of configuration and create the routes based on the configuration.

Ragazzo commented 11 years ago

Yes, but this is just a same if it can be out-of-the-box in Component class. And in this way, i need to write every time some set* methods, but if it was a basic functionality of Component class i would not need to do that.

qiangxue commented 11 years ago

How could this be done in Component to help you?

Ragazzo commented 11 years ago

Just make a private $_components = array(); and other methods for setting and retrieving components (that i've described above, they exists in Module class) in Component class, it will not hurt the BC because Module class also extending from Component. This will help me to set sub-components properties from config, also this will help me to set other components in already existing components (some DI for example).

Ragazzo commented 11 years ago

I think i can provide a PR later, if you want to see how this will affect existing code.

klimov-paul commented 11 years ago

It would be better to introduce "ComponentFactory" class, which provides the components storage feature. "Component" class will be left as it is. "ComponentFactory" will extend "Component", "Module" will extend "ComponentFactory".

qiangxue commented 11 years ago

No need PR. I understand what you want to achieve, but that's something I want to avoid introducing into a base class like Component unless you have convincing use cases. Your example is different from Module and doesn't need this functionality.

Ragazzo commented 11 years ago

Ok, as you say (

qiangxue commented 11 years ago

Note what Module provides:

  1. configure components
  2. delayed loading of components
  3. magic access via component IDs

Your example doesn't need 2 and 3, and I think most composite components don't need them either.

Ragazzo commented 11 years ago

But i need it, i need to set sub-components from the config. And i need to reconfigure sub-components, but i cant do this in current way. And i cant every time create a module for a main module, just to have an ability to set or configure sub-components.

qiangxue commented 11 years ago

I know you need it. :) Please show a convincing use case for it (your previous one is not a valid one). This will let us understand better the needs and design accordingly. We need to take extra care to change a base class like Component or Object needs extra care because the change will affect the whole class hierarchy.

qiangxue commented 11 years ago

@klimov-paul Yes, adding a separate layer above Component is a possible solution, although the name ComponentFactory doesn't sound right. And I still want to see the necessity of having this layer because I think most components don't need the above feature 2 and 3 (especially 3).

Ragazzo commented 11 years ago

Ok, i will provide my config and components structure today evening or tomorrow. As for factory, i dont think it needed, we already have a factory method that suites fine everybody i think.

klimov-paul commented 11 years ago

Here is closest example I can find:

At the moment I am working on the external service authentication component for Yii 1.1.x. It should provide the ability for the user to authenticate using Google, Facebook, Twitter and so on. I have created and application component (extension of CApplicationComponent) named “ExternalAuth”. For each external auth service (Google, Facebook, Twitter and so on) I have created a separated classes extending CComponent. Each of this a component may have unique set attributes and definitely will have unique configuration. To be able to configure entire “ExternalAuth” component I need to provide it ability to consume configuration in following format:

// Application configuration file
…
‘components’ => array(
    ‘extAuth’ => array(
        ‘class’ => ‘ExternalAuth’,
        ‘services’ => array(
            ‘google’ => array(
                ‘class’ => ‘GoogleOpenId’,
            ),
            ‘facebook’ => array(
                ‘class’ => ‘FacebookOAuth’,
                ‘client_id’ => ‘facebook_client_id’,
                ‘client_secret’ => ‘facebook_client_secret’,
            ),
        ),
    ),
),

In order to achieve this I have to repeat logic from “CModule” inside “ExternalAuth”.

klimov-paul commented 11 years ago

name ComponentFactory doesn't sound right.

Possible names:

Ragazzo commented 11 years ago

well, so @klimov-paul provided even better example, so i will not provide mine, it is also looks like his example. But do we really need separated class for only creating components? The base thing in factory is that it must be overwrited by other classes, so i think factory method will be just enough, like it is made right now, no? I think moving some functionality from Module to Component will be ok in this way, what do u think?

nkostadinov commented 11 years ago

The request here sounds like DI Component. May be a base container class on top of Component will be best and will provide the DI functionality in the fw.

qiangxue commented 11 years ago

Yes, I think we should think more about this. In particular, we should consider if we want to provide DI and how it will play with the rest of the framework and the application code.

maikbrox commented 11 years ago

I think (or hope), that I have a more specific use case.

For our application we are experimenting by keeping the Controllers, Models and Commands really clean and small. With clean and small I mean: as clean as described in this book: http://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882

For that reason, we are making heavy use of modules and components. While modules can have submodules, components can't as far as I can understand. There are 2 problems we are having with that:

When we have components in a certain module, some of that components grow really big. We want to divide that into subcomponents (structure is fictional):

components |- Search |....|- Search.php (extends CApplicationComponent) |....|- Filter |....|....|- SearchFilter.php (extends CApplicationComponent) |....|....|- FilterFactory |....|....|- FilterInterface |....|....|- QueryFilter |....|....|- FacetFilter |....|....|- StatisticsFilter |....|- Query |....|....|- SearchQuery.php (extends CApplicationComponent) |....|....|- QueryFactory |....|....|- QueryInterface |....|....|- MltQuery |....|....|- TermsQuery

According to the 2 points above, it's hard to not do this in a way we don't want (well, let's be clear: not in my imagination).

For now we do for example:

    Yii::app()->search->doThings();
    Yii::app()->searchFilter->doThings();
    Yii::app()->searchQuery->doThings();

which all have to be registered in the config. It is hard to maintain that.

I think for us, the solution is that we can do:

$filterComponent = Yii::app()->getComponent('search')->getComponent('filter');

or

$filterComponent = Yii::app()->search->filter;

where $filterComponent knows about the classes it contains.

creocoder commented 11 years ago

@maikbrox What problem to define method getFilter() in search component, etc?

maikbrox commented 11 years ago

@creocoder That's my less preferred plan B. Explicitly defining the things that you want to call as a function makes it a bit, or maybe much, more rigid I think. It makes standardizing the component classes harder.

But that's just my taste. I like the way modules handle submodules and I think that it's nice if components work the same. I provided my use case, maybe that gives a good reason for implementing it. If not, it's no disaster.

creocoder commented 11 years ago

@maikbrox You can check for example WebUser component it explicitly wait for $identityClass and have getIdentity() method. This way it have: 1) DI, 2) standart call style. I do not think Search component should be soooo abstract as Module. Moreover i'm sure things like:

$filterComponent = ...->getComponent('search')->getComponent('filter')...->getComponent(...)...;

is antipattern and should be used where you really need this. If use it everywhere architecture fast turn in something like endless-ZF-2-like-LSD-powered-cosmic-scale-code-mess.

maikbrox commented 11 years ago

I know about that getIdentity thing. But, well, your explanation looks plausible. Thanks. I'll try the other option.

Ragazzo commented 10 years ago

bump, what about this one? Yii2 is in the beta-alpha status so it is better to decide now this.

Ragazzo commented 10 years ago

bump once again. I was told by some developers that they will very welcome this feature as they will not need manually set-up in init() method all components. So i think its about now to decide should we do this issue (i think we should, there are also many upvotes for it) or just ignore developers and close it :)

gitmastro commented 10 years ago

Hi Guys,

just suggesting one example for complex component


$filterComponent = ...->getComponent('search/filter/FilterFactory');
$filterComponent->function();

may be I am wrong

creocoder commented 10 years ago

Vote to close and forget this antipattern.

Ragazzo commented 10 years ago

This is not antipattern :)

creocoder commented 10 years ago

@Ragazzo You may think as you want, but as you see two you bumps not help to make this issue interesting for anybody :) Bacause peoples doubts about this feature needed. Any real-world examples? As you see i show above how to reach same goals with another (non-antipattern) methods.

Ragazzo commented 10 years ago

You may think as you want, but as you see two you bumps not help to make this issue interesting for anybody :) Bacause peoples doubts about this feature needed. Any real-world examples? As you see i show above how to reach same goals with another (non-antipattern) methods.

read above comments, its all about implementing this. Nobody reacts just because they know that if someone from Yii-core will not want this feature then all other developers just need to shut the fuck up cause its not core-developers case. I would rather suggest to listen to developers opinions rather then produce shit code, that is already enough for Yii2 too.

creocoder commented 10 years ago

If this feature will be implemented than all Yii 2 components should be rewriten to use this. This feature can be interesting from component hierarchy point of view. But component hierarchy lead to complexity. This is why i call this feature antipattern. This will not help make things easier, this will make things complex. Components should be plain. Hierarchy like component 1 => component 2 => component 3 is a mess ) While component 1 have property with ID for component 2 and component 2 have ID for component 3 allow leave components on same 1 level.

Ragazzo commented 10 years ago

If this feature will be implemented than all Yii 2 components should be rewriten to use this

just move 3 methods from Module to Component

This is why i call this feature antipattern. This will not help make things easier, this will make things complex. Components should be plain. Hierarchy like component 1 => component 2 => component 3 is a mess ) While component 1 have property with ID for component 2 and component 2 have ID for component 3 allow leave components on same 1 level.

Nope, its just about how developers see this. Ones making overusing scenarios and introducing new fields where it is not needed, some making complex sub-components, that also should be avoided. But when i configure Component object and make available in it getting other components (1 nested level as you see), i cant do this. I just need to extend from Module that is not even about it.

creocoder commented 10 years ago

i cant do this

Do you know how Yii 2 components solve this trouble? It use:

public $componentID;

You agree? You can check all Yii 2 components. You suggest other way to do things. Not Yii way. By suggesting that you try to change Yii way from one to another. That cause Yii 2 components should also change its approach. So this topic is not just about new harmless feature. This topic about something BIGGER. Something very very dangerous.

Ragazzo commented 10 years ago

You agree? You can check all Yii 2 components. You suggest other way to do things. Not Yii way. By suggesting that you try to change Yii way from one to another. That cause Yii 2 components should also change its approach. So this topic is not just about new harmless feature. This topic about something BIGGER. Something very very dangerous.

what? i need to declare internal components of some object as external and available to all application? I guess this will not go anywhere, so lets just save our time ;) There are some other issues in Yii2 on what we can work :)

creocoder commented 10 years ago

@Ragazzo Also several month ago you get an question to you

Could you show some examples why you need this?

Today you have an question:

Any real-world examples?

Show any convincing example where this feature needed as water and there is no other (simplier) approaches to reach this goal. You can show such example? I very much doubt. ;-)

Ragazzo commented 10 years ago

Show any convincing example where this feature needed as water and there is no other (simplier) approaches to reach this goal. You can show such example? I very much doubt. ;-)

so examples above are not enough? Nuff said)) because it will be like yeah, you got one more example, but still not convinced because there are only two opinions: mine and wrong :D

creocoder commented 10 years ago

so examples above are not enough? but still not convinced because there are only two opinions: mine and wrong

You are not right at this point. The reason why it not convienced is because there is another approaches how to reach same goals :)

Ragazzo commented 10 years ago

Ah, i guess while someone will not submit PR it will not be resolved/rejected. :) So i dont think we need to discuss it, lets wait for PR, will try to do in next few days :)

creocoder commented 10 years ago

@Ragazzo Yes, this is great idea. I mislooked @klimov-paul examples. Seems it make sence... hmm... a little :D

samdark commented 10 years ago

That's how it can be done currently:

class Rocket extends Component
{
  public $engine = '\app\rocket\lightspeed\Engine';

  public function fly()
  {
    if (!$this->engine instanceof Component) {
       $this->engine = Yii::createObject($this->engine);
    }
    return $this->engine->work();
  }
}
$rocket = new Rocket();
$rocket->engine = [
  'class' => 'app\rocket\gasoline\Engine',
  'fuel' => 'Gas',
];
if (!$rocket->fly()) {
  echo 'Well, that was pretty old engine...';
}

Proposal makes it:

class Rocket extends Component
{
  public $engine = '\app\rocket\lightspeed\Engine';

  public function fly()
  {
    return $this->engine->work();
  }
}
$rocket = new Rocket();
$rocket->engine = [
  'class' => 'app\rocket\gasoline\Engine',
  'fuel' => 'Gas',
];
if (!$rocket->fly()) {
  echo 'Well, that was pretty old engine...';
}

Is it that big difference?

Mirocow commented 10 years ago

Yes I want to have it