[Fixes #7300] Add ability to set custom ComponentRegistry to Controller object #7301
Conversation
| $this->_Controller = $Controller; | ||
| $this->eventManager($Controller->eventManager()); | ||
| } | ||
| $this->setController($Controller); |
There was a problem hiding this comment.
I would put this inside the same if it had before
|
Related to #7300 |
| @@ -254,6 +255,11 @@ public function __construct(Request $request = null, Response $response = null, | |||
| $modelClass = ($this->plugin ? $this->plugin . '.' : '') . $this->name; | |||
| $this->_setModelClass($modelClass); | |||
|
|
|||
| if ($componentRegistry !== null) { | |||
| $this->_components = $componentRegistry; | |||
| $this->_components->setController($this); | |||
There was a problem hiding this comment.
Could this be done in the components() method when it is used as a setter?
public function components($components = null)
{
if ($components === null && $this->_components === null) {
$components = new ComponentRegistry($this);
}
if ($components !== null) {
$components->setController($this);
$this->_components = $components;
}
return $this->_components;
}There was a problem hiding this comment.
Nice idea. thanks.
|
I do not understand why we need a custom registry. Can you please explain? |
|
@rchavik If you'd like to implement DependencyInjection plugin for Cake3, then you have to replace the default P.S. I know about https://github.com/lorenzo/piping-bag abd https://github.com/rochamarcelo/cake-pimple-di, but the second one for example makes DI container accessible from everywhere, it's not good. Container should be silent for another application components and should not be used directly in your code. // updated due to typos. |
|
@korotovsky btw piping-bag's aim is so that you don't have to sue the container in your code |
|
@lorenzo Yes, but it has another disadvantages for us, AOP for example. |
|
Any other comments on this before I merge? |
| @@ -61,6 +60,18 @@ public function getController() | |||
| } | |||
|
|
|||
| /** | |||
| * Set the controller associated with the collection. | |||
| * | |||
| * @param Controller $controller Controller instance. | |||
There was a problem hiding this comment.
Fullynamespaced class is required.
|
Looks good to me. Making the framework more DI container friendly is nice given that we don't provide a DI container. |
|
👍 |
|
Can we change the last two args as an array? |
| */ | ||
| public function __construct(Request $request = null, Response $response = null, $name = null, $eventManager = null) | ||
| public function __construct(Request $request = null, Response $response = null, $name = null, $eventManager = null, $components = null) | ||
| { |
There was a problem hiding this comment.
Sorry, these two args here, ie: $config = array('eventManager' => null, 'components' => null)
There was a problem hiding this comment.
What benefits it will give to us? As for me - flat dependencies is the best solution. Moreover I afraid it's not a part of this task.
There was a problem hiding this comment.
@markstory well, request and response was there in 2.x, so i figure we can make it the same. I don't mind getting them all into $config though.
@korotovsky to make it uniform like other classes do, eg: Table::__construct
[Fixes #7300] Add ability to set custom ComponentRegistry to Controller object
| * @param \Cake\Controller\Controller $controller Controller instance. | ||
| * @return void | ||
| */ | ||
| public function setController(Controller $controller) |
There was a problem hiding this comment.
setController() is a public method but lacks an explicit test in ComponentRegistryTest.
No description provided.