Skip to content

Instantly share code, notes, and snippets.

@sminnee
Created Feb 22, 2017
Embed
What would you like to do?
App RFC [wip]

Problem

There are a few issues with the current SilverStripe 4 code:

  • Our bootstrap is a large pile of spaghetti. This makes it harder to build other scripts that pull in SilverStripe services.
  • There are a lot of top-level anchors of global state - Injector::inst(), Config::inst(), etc, that need to managed. This is especially clear in the case of test execution
  • We're increasingly using Injector, and combined with fully-qualified interface/class names as the service names, makes sonme

These issues could be solved with an App object

The App object

namespace SilverStripe\Core;

class App
{

    // singleton management
    static function inst();
    static function set_inst(App $inst);

    // Core objects that make up a SilverStripe application
    function getModuleManifest();
    function getConfig();
    function getInjector();

    // Shortcut for $this->getInjector()->get("$service.$namespace");
    function getService($service, $namespace = null);

    // Shortcuts for key services from the injector
    function getDatabase($namespace = null);
    function getCache($namespace = null);
    function getDirector();
}

The App object would be instantiated in bootstrap. As well as getters for the 3 core services (module manifest, config, and injector), it can be a staging ground for other shortcut methods.

DefaultAppFactory

Rather than a lengthy sequential bootstrap, we can refactor the bootstrap code to be a factory. That way, the boostrap could be as simple as something like this:

require_once __DIR__ . '../vendor/autoload.php';

use SilverStripe\Core\DefaultAppFactory;
use SilverStripe\Control\DefaultRequestFactory;

$app = DefaultAppFactory::create();
$request = DefaultRequestFactory::create();
$response = $app->getDirector()->direct($request);
$response->output();

inst()

The current inst() methods scattered in the app can be refactored to defer state management to App:

class Injector {
  function inst() {
    reutrn App::inst()->getInjector();
  }
}

This means that tests can be boostrapped by putting in a new App object.

$this->App

Any object that is instantiated by the injector can be configured to have an $app property populated with the relevant object. This means we can encourage developers to avoid static calls to app/injector/etc altogether!

class SomeClass {
  function someCustomThing {
    $cache = $this->app->getService(SimpleCache::class);
    return $this->app->getDatabase()->query("SELECT * FROM CustomTable")->first();
  }
}
@tractorcow
Copy link

tractorcow commented May 17, 2017

I've done some more research and propose https://gist.github.com/tractorcow/c0656a334c13eac8ae252610d226a1c6 as a more complete interface for this.

Note that I've excluded moving Director into App for the time being (to reduce scope affecting refactor of Director.php)

@flamerohr
Copy link

flamerohr commented May 17, 2017

The $app property opens up other issues like infinite loop while dumping an Object created by Injector. On the otherhand, this would make it easier to switch app context for the Object, which is quite helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment