Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Container DRY #12829

Closed
BernhardPosselt opened this issue Dec 13, 2014 · 4 comments
Closed

Container DRY #12829

BernhardPosselt opened this issue Dec 13, 2014 · 4 comments
Milestone

Comments

@BernhardPosselt
Copy link
Contributor

To make it easier for newcomers and more DRY for app authors it would be nice if we could let the container figure out the dependencies by itself :)

$c->registerService('PageController', $callback);
$c->query('PageController');

This will still work and will help us handle the edge cases such as register callbacks for interfaces and parameters.

Now let's enhance the query() method to do the following if no callback/parameter is registered for a key:

  • Does a class called PageController exist? No
  • Does a class called \OCA\MyApp\Controller\PageController exist? Yes (This is the default. The first lookup for PageController is done in the routing system to keep backwards compability)
  • What parameters does it take in its constructor? $AppName, \OCP\IRequest $request, \OCA\MyApp\Service $service
  • Is there a type for $AppName? No
    • Is there a service/parameter registered for AppName? Yes -> use that
  • Is there a type for $request? Yes \OCP\IRequest
    • Is it an interface? Yes -> damn we cant instantiate it, fail. Now this needs to be registered with the traditional callback style like $c->registerService('\OCP\IRequest', $callback). Ideally we've got support for all those interfaces in the base container
  • Is there a type for $service? Yes \OCA\MyApp\Service
    • Is it an interface? No -> great we can instantiate it. Use reflection to find its parameters etc
  • All parameters for PageController are assembled so we can construct PageController and return it

Ok great. This allows in 99% of all cases to get rid of appinfo/application.php. When do you still need it?

  • You are using custom interfaces in your app as type parameters (unlikely)
  • You are injecting strings/ints/arrays/callbacks into your constructor (kinda likely, but you can easily create a pure POPO classes that hold these parameters as public attributes, like \OCA\News\Config->SomeString)

What do we gain by this? A smoother learning curve and more DRY. By default you dont have to deal with anything, you just use the correct types in your controllers and other classes and everything works out of the box. Should you still need an exception, you can add the appinfo/application.php and you need to know exactly as much as now. Downsides? You have to supply type hints (you should do that anyways) and it might be obscure why __construct($AppName) works but __construct($appName) doesnt.

PS: What about routing?

Well in appinfo/routes.php we've got the following atm:

$application = new Application();
$application->registerRoutes($this, ['routes' => [
   ['name' => 'page#index', 'url' => '/', 'verb' => 'GET'],
   ['name' => 'page#do_echo', 'url' => '/echo', 'verb' => 'POST'],
]]);

What if we could this instead?:

return  ['routes' => [
    ['name' => 'page#index', 'url' => '/', 'verb' => 'GET'],
    ['name' => 'page#do_echo', 'url' => '/echo', 'verb' => 'POST'],
]];

The router could check if an array is being returned. If an array is being returned, you can instantiate a container with the current app name (it is the first constructor parameter! https://github.com/owncloud/core/blob/master/lib/public/appframework/app.php#L43) and call the registerRoutes method in it with the returned array.

What about cron? Just create a new Application with your appname and you're done :)

@georgehrke @DeepDiver1975 @butonic @PVince81 @MorrisJobke @nickvergessen @jbtbnl

@BernhardPosselt
Copy link
Contributor Author

Both PRs open :)

@BernhardPosselt
Copy link
Contributor Author

I've morged it into one PR since it is easier for testing. Please review

@MorrisJobke
Copy link
Contributor

Sounds nice :)

BernhardPosselt pushed a commit that referenced this issue Dec 23, 2014
* resolves dependencies by type hint or variable name
* simpler route.php
* implementation of #12829

Generates and injects parameters automatically. You can now build full classes like

    $c->query('MyClassName')

without having to register it as a service. The resolved object's instance will be saved by using registerService. If a constructor parameter is not type hinted, the parameter name will be taken.

Therefore the following two implementations are identical:

    class Class1 { function __construct(MyClassName $class)
    class Class1 { function __construct($MyClassName)

This makes it possible to also inject primitive values such as strings, arrays etc.

In addition if the query could not be resolved, a `QueryException` is now thrown

Routes can now be returned as an array from `routes.php` and an `appinfo/application.php` is optional

Old commit messages:

make it possible to return the routes instead of having to intialize the application
try to get the controller by convention
add first implementation of automatic resolve
add another test just to be sure
store the resolved object
more tests
add phpdoc to public app.php method
use the same variable for the public app.php method
deprecate old methods and add services for public interfaces
deprecated getServer method
disallow private api injection for apps other than core or settings (settings should be an app goddamnit :D)
register userid because its such an often used variable
fix indention and leading slash
use test namespace
add deprecation reasons, remove private api usage checks and remove deprecation from getServer()
add additional public interfaces
add public interface for rootfolder
fix syntax error
remove deprecation from methods where no alternative is there yet
remove deprecated from method which has no alternative
add timezone public service for #12881
add another deprecation hint
move deprecation into separate branch
remove dead comment
first try to get the namespace from the info.xml, if it does not exist, just uppercase the first letter
also trim the namespace name
add an interface for timefactory
move timefactory to public and add icontrollermethodreflector
keep core interface
fix copyright date in headers
@MorrisJobke MorrisJobke added this to the 8.0-current milestone Dec 23, 2014
@MorrisJobke
Copy link
Contributor

#12830 is merged

@lock lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants