silverstripe / silverstripe-admin

Silverstripe Admin Component
BSD 3-Clause "New" or "Revised" License
25 stars 92 forks source link

Injector makes invalid instantiation request #1804

Open NightJar opened 1 month ago

NightJar commented 1 month ago

Resulting in

ArgumentCountError: Too few arguments to function SilverStripe\Control\HTTPRequest::__construct(), 0 passed in [...]vendor/silverstripe/framework/src/Core/Injector/InjectionCreator.php on line 35 and at least 2 expected

https://github.com/silverstripe/silverstripe-admin/blob/55ad9610ff1095b576168a238654d672cb401576/code/SudoModeController.php#L48


Thanks @GuySartorelli for pointing out (via user's slack) that it's looking for a pre-registered request. Seems like this is a new code execution path in CMS 5, as the error doesn't occur when running tests against CMS 4.

The context of the error is attempting to run a PHPUnit test with a normal "just enough" setup (i.e. it's not production traffic):

        $admin = new ModelAdmin();
        $request = $admin->getRequest();
        $request->setSession(new Session([]));
        $admin->doInit();

Interestingly the offending line is not getRequest() as one might think, but rather doInit()

The execution path is via getCombinedClientConfig() which falls throught to the sudo mode controller that for whatever reason uses Injector::get rather than $this->getRequest(). Using getRequest would cause no issue, as per the default request set (but not registered with Injector) in RequestHandler::__construct():

    public function __construct()
    {
        $this->brokenOnConstruct = false;

        $this->setRequest(new NullHTTPRequest());  // because of this

        parent::__construct();
    }
GuySartorelli commented 1 month ago

Thanks for raising this!

You could work around this by injecting the request: Injector::inst()->registerService(HTTPRequest::class, $request);

Given this is only happening in a unit test and there's an obvious workaround I can't promise the issue will be prioritised any time in the near future.

NightJar commented 1 month ago

Yes, I was just about to add this in my own comment. To clarify in the same example from the OP (slightly expanded to include namespace usage changes):

+use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Control\Session;
+use SilverStripe\Core\Injector\Injector;
        $admin = new ModelAdmin();
        $request = $admin->getRequest();
+       Injector::inst()->registerService($request, HTTPRequest::class);
        $request->setSession(new Session([]));
        $admin->doInit();