contributte / console

:boom: Best minimal console (symfony/console) to Nette Framework (@nette)
https://contributte.org/packages/contributte/console.html
MIT License
91 stars 32 forks source link

Fix setting url to http.request #43

Closed jfilla closed 8 months ago

jfilla commented 4 years ago

Service http.request can be any class implementing interface IRequest. If you have custom IRequest implementation and console.url config is set, DI container cache looks like this:

public function createServiceHttp__request(): Flowgate\FunctionalTests\RequestMock
{
    return new Nette\Http\Request(new Nette\Http\UrlScript('http://localhost'));
}

, which causes type error. It should look like this

public function createServiceHttp__request(): Flowgate\FunctionalTests\RequestMock
{
    return new Flowgate\FunctionalTests\RequestMock;
}
mabar commented 4 years ago

I believe this works in your case in tests, but it breaks behavior of ConsoleExtension for anyone who overrides Request for standard usage. Imho what should be done here is to override RequestFactory by ConsoleExtension instead so you are still able to override Request

jfilla commented 4 years ago

I tried what i think is standard usage (is it?). Creating class

<?php

namespace App;

use Nette\Http\Request;
use Nette\Http\UrlScript;

class CustomRequest extends Request
{

    public function __construct()
    {
        parent::__construct(new UrlScript('http://whatever'));
    }

}

registering it as http.request

services:
    http.request: App\CustomRequest

in cli command i tried to call

$this->container->getByType(IRequest::class);

which fails with error

TypeError: Return value of Container_b2518d818a::createServiceHttp__request() must be an instance of App\CustomRequest, instance of Nette\Http\Request returned

because of this inside DI container

public function createServiceHttp__request(): App\CustomRequest
{
    return new Nette\Http\Request(new Nette\Http\UrlScript('http://localhost'));
}

I think with console.url enabled is imposible to override http.request anyway. I will think about it and figure out better solution.. and fix tests of course.

jfilla commented 4 years ago

What about now? I created console requestFactory, which is used only when default requestFactory is used. Exception is thown on custom requestFactory implementation and console.url set. Console.url in combination with custom requestFactory is in my opinion useless, url should be set by custom requestFactory itself.

mabar commented 4 years ago

Looks way better. I will give it some final touches later today before release. Thanks

f3l1x commented 4 years ago

Hey @jfilla, I want to assure you we'll handle this issue. We didn't have a time yet.

f3l1x commented 8 months ago

Thank you @jfilla ❤️