Injector class replacement does not work first time

[willmorgan]

Steps to replicate:

• Make a service that doesn't extend any other class. Let's call it, MyCoolService.
• Create a mock class, let's call it, MyCoolService_Disabled
• Update the config to use the disabled service:

    Config::inst()->update('Injector', 'MyCoolService', array(
        'class' => 'MyCoolService_Disabled'
    ));

• Call Injector::inst()->create('MyCoolService');

Expected: The result is a MyCoolService_Disabled object.

Actual result: The result is a MyCoolService object.

[willmorgan]

Update: I've got around this by instead using Injector->registerService and using the $replace parameter, but still...

[tractorcow]

The problem is that the config locator, a service used by the injector class to extract dependencies from the config, will cache any config data. See SilverStripeServiceConfigurationLocator.

What's probably necessary is some kind of reset function similar to DataObject::reset.

[tractorcow]

Solution: use Injector::inst()->load() instead of Config::inst()->update(). The syntax is nearly the same.

[willmorgan]

The problem is that the config locator, a service used by the injector class to extract dependencies from the config, will cache any config data.

Hang on, you're closing this and there's still a clear defect in the framework? :confused:

[tractorcow]

Do you think it's still necessary to make it work the way you had attempted earlier? I had thought that the solution I presented would have solved the issue.

I mean, if you disagree then I don't mind to re-open this, and we could look at a solution that would prevent this issue coming up again, but if it's just a matter of style as to how injector is overridden, then it's not really a defect is it? :)

[tractorcow]

I've had a go at implementing a cursory 'reset' function that could be used if you wished to update the Injector via config.

https://github.com/tractorcow/sapphire/commit/a9416c2f0930d6401403d5bbd4fbc72ce67498b5

Note the warning on Injector::reset. It makes me feel though that this api has so few non-disruptive use cases that it could be difficult to maintain it. Nethertheless, there is an option. :)

The trade off we have decided to make in the current implementation is to be as optimised for performance as possible. There's no way of elegantly picking up config changes without impacting performance. Between creating an object and the config there are several levels of caching, in the Injector itself, the config locator, and within the config itself. The implementation I made above demonstrates what's necessary to cope with this.

[willmorgan]

Thanks for reopening this, @tractorcow.

Do you think it's still necessary to make it work the way you had attempted earlier?

I think it's still necessary, yes. As long as this config based way of replacing classes is possible...

Injector:
    TheirClass:
        class: MyClassImplementation

...you should reliably be able to update the config and have those changes take effect. The somewhat unclear existing way of conditionally setting config in yml files brings about the need to do it in a _config.php file, anyway.

Your commit looks good, though, and the warning on reset makes sense (I was nesting within Config + Injector in a test scenario.) I'm wondering whether it would be more sensible to simply offer the ability to overwrite the cache on a single spec entry, so performance isn't degraded too much.

[tractorcow]

@willmorgan the yaml use case you just showed there should be fine. If it's configured via yaml, then the config will be initialised prior to any instances being loaded, or injector being initialised with the config data.

I think it's only when you try to update config programatically mid-to-late during the execution pipeline (or in the _config.php, at times I guess) that you'd run into issues with cached configs.

yaml conditionals don't really provide a great deal of flexibility, I admit though, but it's hard to manage the tradeoff between flexibility and performance, which config has its own personal struggle with.

In SS 4.0 it's likely we'll rewrite the config system with more of an emphasis on performance.

[tractorcow]

What would really help this problem is enabling some kind of "Partial invalidation". If something becomes outdated, then only invalidating that part of the config is necessary. Not sure how this could be done without some kind of event / messaging system though.

[maxime-rainville]

Just recreated the example in SS4 and it seems to work as expected.

<?php

use SilverStripe\Core\Config\Config;
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Dev\BuildTask;

class TestConfig extends BuildTask
{
    public function run($request)
    {
        Config::inst()->merge(Injector::class, Foo::class, ['class' => Bar::class]);

        // This outputs "Bar"
        echo get_class(Injector::inst()->create(Foo::class));
    }
}

Unfortunately, Silverstripe CMS 3 has entered limited support in June 2018. This means we'll only be fixing critical bugs and security issues for Silverstripe CMS 3 going forward.

You can read the Silverstripe CMS Roadmap for more information on our support commitments.