Various instances of new keyword where injection should be used.

[GuySartorelli]

Throughout the various silverstripe modules there are many many many instances of the new keyword being used to instantiate classes that use the Injectable trait. The ::create() syntax should be used instead except for very specific cases.
Note that I am excluding tests from this - it makes sense for tests to use new to be sure they are testing the exact class they are meant to test and so that any failures in Injector don't cause all tests to fail.

Note that @wernerkrauss made a tool that can automatically make these changes for us

[dhensby]

Historically there had been an informal understanding that ::create() syntax was only used where there was deemed to be "demand" for it, because of the inherent overhead to performing the DI instantiation.

#10265 may have addressed some of that performance concern, but there will still be some overhead.

This strategy is somewhat vindicated by the fact that, if there are lots of instances of not using the ::create() syntax, we don't get complaints about it.

Basically, we should just be cautious about this, but not refuse to move to the ::create() syntax

[michalkleiner]

If a class has the Injectable trait, it should use ::create in core wherever it needs to be created, otherwise you might get complaints from the other end that the class is not replaced everywhere if replaced via Injector, so in general, I support this with the exception of the very few very specific cases.

[dhensby]

I do agree with the principle of "if it's Injectable then it should be instantiated using ::create()" (otherwise why on earth does it have the trait) but I also think you prove my point:

otherwise you might get complaints from the other end that the class is not replaced everywhere if replaced via Injector

We aren't getting those complaints, so it's not a pressing issue and we should be giving thought to performance in the framework wherever we can. If there's no pressing need to make performance degrading changes, should we be making them?

This feels like a principles vs pragmatics problem

[GuySartorelli]

My concern is that people may be complaining about it, just not to us. I've certainly run into a few situations where a class I expected to be injected wasn't being injected where I wanted it to, and have had to work around it.

You're right that we do need to be conscious of the performance implications though. It would be good to quantify that so we can have a better idea of what that overhead actually is.

[michalkleiner]

@GuySartorelli create a PR with your changes and then we can compare that on kitchen sink vs the current state on things like 10000 dev/builds, standard page load, 100-elements-on-a-page page load etc. Without the PR we can't really measure much.

[maxime-rainville]

I think we should aim to do something like that in CMS 5. Just did a quick search on the core modules (excluding tests) and it came back with 1924 instances of new. Not all of those should be ::create calls but majority of what I saw could be.

@guy Would you be able to do some dumb benchmark that creates a lot of Injectable classes with ::create and with new. Then we can measure how much of a hit we are getting between the two. Presuming, we don't see a big performance hit we could look at proceeding with this.

Historically there had been an informal understanding that ::create() syntax was only used where there was deemed to be "demand" for it, because of the inherent overhead to performing the DI instantiation.

Whatever "best practice" we land on, we probably should look at documenting it somewhere ... if only to avoid those recurring argument.

[GuySartorelli]

I used this code to get a rough benchmark, swapping between the two methods. This was adapted from a benchmark @emteknetnz did in #10265

<?php

use CWP\CWP\PageTypes\BasePageController;
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Forms\GridField\GridField;
use SilverStripe\ORM\Connect\MySQLDatabase;
use SilverStripe\Versioned\Versioned;

class PageController extends BasePageController
{
    protected static $classes = [
        GridField::class => [
            ''
        ],
        Versioned::class => [],
        MySQLDatabase::class => [],
    ];

    protected static $numInstances = 25000;

    protected function init()
    {
        parent::init();

        $injector = Injector::inst();
        $time_pre = microtime(true);
        $newInstances = $this->useNewKeyword();
        // $newInstances = $this->useCreate($injector);
        $time_post = microtime(true);
        $exec_time = $time_post - $time_pre;
        echo sprintf("This took %.4f seconds", $exec_time);
        return ['Instances' => $newInstances];
    }

    private function useNewKeyword()
    {
        $instances = [];
        foreach (self::$classes as $class => $args) {
            for ($i = 0; $i < self::$numInstances; $i++) {
                $instances[] = new $class(...$args);
            }
        }
        return $instances;
    }

    private function useCreate(Injector $injector)
    {
        $instances = [];
        foreach (self::$classes as $class => $args) {
            for ($i = 0; $i < self::$numInstances; $i++) {
                if (method_exists($class, 'create')) {
                    $instances[] = $class::create(...$args);
                } else {
                    $instances[] = $injector->createWithArgs($class, $args);
                }
            }
        }
        return $instances;
    }
}

It will spin up 25000 instances of each of the three classes, for a total of 75000 objects instantiated with each of the methods.

The results I got from this show injection being about 15.6% (0.2105 seconds) slower than the new keyword with this setup.

method	time
new	1.3505
create()	1.5610

However, note that both Versioned and MySQLDatabase have some config which tell the Injector to do a little more work. If we remove those and just do GridField, the numbers are closer, with injection being 5.5% (0.0741 seconds) slower:

method	time
new	1.3524
create()	1.4265

[GuySartorelli]

It's also possible that the method_exists($class, 'create') check being done 75000 times accounts for some of the extra time the injection code takes to run in this example.

[GuySartorelli]

NOTE: The benchmark I did was before #10265 which boasted a ~17% performance improvement for dependency injection itself. I think there's no longer any reason not to do this.