Resolve controller via container

[lordrhodos]

This PR extends the DIC feature with support to resolve the controller instances from the container:

class FooController extends AbstractController
{
    /**
     * @var BarService
     */
    private $barService;

    /**
     * FooController constructor.
     *
     * @param BarService $barService
     */
    public function __construct(BarService $barService)
    {
        $this->barService = $barService;
    }

    public function barAction(int $id): Response
    {
        $bar = $this->barService->bar();

        return Response::create("Hello $bar (id: $id)");
    }
}

To be able to support constructor based DI and to avoid that a missing parent::__construct call would break the supported flow of the AbstractController I have decided to remove the final constructor from AbstractControllerand to implement the BaseApp against a new ControllerInterface :

interface ControllerInterface
{
    public function setApp(BaseApp $app);

    public function setRequest(Request $request);

    public function init();

    public function dispatch();
}

A new ViewAwareControllerInterface adds support for the View if needed and is implemented by the AbstractController.

There is some code duplication in the AbstractController::foward method setting the required dependencies via setter injection, so maybe a dedicated ControllerResolver or similar would be better.

One breaking change has been introduced which can be fixed quite simple when upgrading: the visibility of the AbstractController::init method needed to be change to public.

I am open to suggestions if there is a better way to provide this feature and of course to feedback regarding this approach ;-)

[felixsand]

I'm not so sure of this approach. It does expose the internals of the Controller a bit too much (thus increase coupling). Also might this not break backward compatibility (for example in the SWS and SCS)?

I think we should set this as "on hold" for now and focus to get the 1.0.0 with dependency injection out as a first step. What do you say?

[lordrhodos]

I'm not so sure of this approach. It does expose the internals of the Controller a bit too much (thus increase coupling). Also might this not break backward compatibility (for example in the SWS and SCS)?

I am happy for other approaches if we can get propery controllers with DI in the constructors as result. IMO the suggestions in this PR try to minimize the effort to upgrade. One small visibility change which can be fixed very quick and should not be an issue when upgrading.

I understand your concern about exposing the constructor and the init method, but I do not see any other way keeping breaking changes to a minimum.

The new DIC feature should help us move away from the service locator antipattern which in the current implementation is encouraged by the final constructor and no way to support proper DI. IMO al other approaches to realize good DI will be messy. We could try to use setter injection or use the init function, but this will be ugly and I just have not found any good argument why this would be better. If you have another idea I am happy to try it.

The interface addition is an important bug fix. The current implementation instantiates the controller here:

starlit-app/src/Router.php

Line 226 in 780f28b

$actualController = new $controllerClass($this->app, $request);

after fetching the controller class from here

starlit-app/src/Router.php

Lines 236 to 250 in 780f28b

	public function getControllerClass($controller, $module = null)
	{
	$moduleNamespace = null;
	if (!empty($module)) {
	$moduleNamespace = Str::separatorToCamel($module, '-', true);
	}
	$controllerClassName = Str::separatorToCamel($controller, '-', true) . $this->controllerClassSuffix;
	return '\\' . implode('\\', array_filter([
	$moduleNamespace,
	$this->controllerNamespace,
	$controllerClassName
	]));
	}

If I am not mistaken this class can be anything ... and finally we call the dispatch method on it

starlit-app/src/BaseApp.php

Line 197 in 780f28b

$response = $controller->dispatch();

without knowing or checking that this method even exists!

I think we should set this as "on hold" for now and focus to get the 1.0.0 with dependency injection out as a first step. What do you say?

I'd say 0.x releases are allowed to introduce breaking changes even for minor versions as of semver:

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

and I'd say as the migration effort covering the breaking change is very small lets get this into 1.0.0 😛

[lordrhodos]

just saw that the DIC branch adds strict type hinting to the Router::route function so an instance of AbstractController is returned

https://github.com/starweb/starlit-app/pull/4/files#diff-cbd75215699157bafdce077e3deff391R187

This fixes above ☝️ mentioned issue with the dispatchmethod.