Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DI: Nette\Http\Request subclass is required instead of interface #90

Closed
iinfo-dev-mk opened this issue Apr 26, 2016 · 18 comments
Closed

DI: Nette\Http\Request subclass is required instead of interface #90

iinfo-dev-mk opened this issue Apr 26, 2016 · 18 comments
Milestone

Comments

@iinfo-dev-mk
Copy link

@iinfo-dev-mk iinfo-dev-mk commented Apr 26, 2016

When we want to you own service for http request, subclass of Nette\Http\Request is required

// contex is instance of Nette\DI\Container
$context->removeService("http.request");
// OwnRequest implements Nette\Http\IRequest
$context->addService("http.request", new OwnRequest());

it throws exception

Nette\InvalidArgumentException: Service 'http.request' must be instance of Nette\Http\Request, OwnRequest given

I think same problem is with all services from Nette\Http package. Only interface can be requested, not class.

Maybe problem is with DI container itself, when removeService is called, I wan't to remove all information about particular service, it seems that in DI container still remains some information about removed service, what isn't desirable.

@fprochazka

This comment has been minimized.

Copy link
Contributor

@fprochazka fprochazka commented Apr 26, 2016

There should be no references to Nette\Http\Request in Nette, since we've successfully replaced it with impl of it's interface and the application worked.

Can you try to find out, what service is requesting the class? You should first check your application, to make sure you're not requiring the class itself, instead of interface and then check the extensions you're using. Also maybe upgrade Nette, since the last references were removed few versions ago.


And also, the request object is a value object, that should in fact be a service. If you wanna work with multiple requests, you should use Kdyby/RequestStack.

@iinfo-dev-mk

This comment has been minimized.

Copy link
Author

@iinfo-dev-mk iinfo-dev-mk commented Apr 26, 2016

@fprochazka I'm using this default extension. After container was created, I tried to replace http request service with code above, and it failed with the exception.
It worked fine with nette 2.2, with nette 2.3 it failed.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Apr 26, 2016

I remember that there was a break BC, when I changed it to interfaces, but I don't remember the details.

@matej21

This comment has been minimized.

Copy link

@matej21 matej21 commented Apr 26, 2016

@dg yes, there would be a BC break if someone requires Http\Request directly and not the interface.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Apr 26, 2016

Exactly.

@iinfo-dev-mk IMHO you can change option class (or type in Nette 3) in your config file to the interface

@iinfo-dev-mk

This comment has been minimized.

Copy link
Author

@iinfo-dev-mk iinfo-dev-mk commented Apr 26, 2016

Require Http\Request directly isn't good practice if DIC offers service name by default. But replace particular service with another one is valid requirement for the DIC.

@dg No, see the issue description, there is no issue with the configuration, I need to use removeService, addService calls. Replacing services this way doesn't work correctly. Hence I also noticed if problem isn't in DI container itself, see my description.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Apr 26, 2016

I mean that you can IMHO change class in config.neon

services:
    http.request:
         class: Nette\Http\IRequest

and then replace it in bootstrap.

DI container behaves correctly.

@iinfo-dev-mk

This comment has been minimized.

Copy link
Author

@iinfo-dev-mk iinfo-dev-mk commented Apr 26, 2016

Again, no, I can't use any config file, there is only PHP code which wants to replace service in already built DIC ($context variable) ''dynamically'' via

$context->removeService("http.request");
$context->addService("http.request", new OwnRequest())

It worked fine with nette 2.2 and fails with the exception in nette 2.3.
There was always possibility change any service dynamically with removeService, addService, which doesn't work now. How can I change http request service dynamically?

Again, problem can be in DIC itself, when I call removeService(serviceName) I expect that DIC removes all information about particular service, but it seems that isn't true.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Apr 26, 2016

So change it via $configurator->addConfig(['services' => ['http.request' => ['class' => 'Nette\Http\IRequest']]]) or via extension.

Dynamic changes in container are limited, because it is statically compiled, so you cannot change one class to another when they are not descendants.

@iinfo-dev-mk

This comment has been minimized.

Copy link
Author

@iinfo-dev-mk iinfo-dev-mk commented Apr 26, 2016

I can't call any $configurator, PHP code knows only about already built DIC, which is passed in.

If DIC can't dynamically replace service with instance of same interface such DIC is useless.
And secondly, current dynamic behaviour is inconsistent, I can replace other services for any instance of same interfaces dynamically without any problem. Finally, it was possible in nette 2.2 and you breaks it silently in 2.3.

Generally PHP is dynamic language, types are complete optional, any object can be passed as argument to any function and fails only in runtime if user want. So there is no reason to limit add, remove or replace services in DIC dynamically.

@matej21

This comment has been minimized.

Copy link

@matej21 matej21 commented Apr 26, 2016

If DIC can't dynamically replace service with instance of same interface such DIC is useless.

It can. Problem is that there is, for a compatibility, concrete class instead of the interface.

Why can't you modify config? If you are using IRequest everywhere, it won't break anything and you will be able to replace the service dynamically.

@iinfo-dev-mk

This comment has been minimized.

Copy link
Author

@iinfo-dev-mk iinfo-dev-mk commented Apr 26, 2016

@matej21 Our code with dynamic modification of DIC is used in special internal testing case from command line and only for that purpose there is necessary to replace default http request with special one, which is created also dynamically according command line arguments.
This is typical use case for dynamically behaviour of PHP and dynamically service change.

I know that dynamic changing of DIC is the edge case, but shouldn't be limited to even interfaces. It's very handy to use some small piece of dynamic PHP code for such special situations, without modification of application itself, we are in PHP not in Java.

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Apr 26, 2016

@iinfo-dev-mk Noone argue, that DIC should allow to replace interface instance by another one. You are right. And DIC works in this way. But Http\Request is an exception, because it is added into DIC as a class, not as an interface. And it cannot be changed because of BC break.

If OwnRequest is used only for tests, you can workaround it.

class OwnRequest2RequestWrapper extends Http\Request
{
    private $own;

    public function __construct(OwnRequest $own)
    {
        $this->own = $own;
    }

    # here overload all public methods to call $this->own methods
}

Such class instance can replace Http\Requst in existing DIC. And one day, you will delete OwnRequest2RequestWrapper class and you will replace IRequest directly by OwnRequst.

It's not nice, it's workaround but IMHO it will work.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Apr 26, 2016

@iinfo-dev-mk

Finally, it was possible in nette 2.2 and you breaks it silently in 2.3.

Yes, it is bad, can you please send pull request to the documentation? https://github.com/nette/web-content/blob/doc-2.3/en/migration-2-3.texy

(Related commit nette/di@116eb12)

@iinfo-dev-mk

This comment has been minimized.

Copy link
Author

@iinfo-dev-mk iinfo-dev-mk commented Apr 26, 2016

@milo Yes I know, after all I eventually finish with inheriting from Http\Request but I don't like it, because it depends on Http\Request changes in the future and I need to investigate Http\Request and OwnRequest relation, it's much more fragile then before.

And still I think dynamic modifications of DIC via removeService, addService can be more relaxed, because it's primary targeted for special cases where such possibility is useful.

@iinfo-dev-mk

This comment has been minimized.

Copy link
Author

@iinfo-dev-mk iinfo-dev-mk commented Apr 26, 2016

@dg documentation - OK, I will try later this week

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Dec 20, 2016

This BC break can be probably solved via narrowed autowiring in DI 2.4 and alias.

@dg dg closed this in a47259b Jan 25, 2017
dg added a commit that referenced this issue Jan 25, 2017
…t & Response (BC break) [Closes #90]
dg added a commit that referenced this issue Jan 25, 2017
…t & Response (BC break) [Closes #90]
dg added a commit that referenced this issue Jan 25, 2017
…t & Response (BC break) [Closes #90]
dg added a commit that referenced this issue Jan 26, 2017
…t & Response (BC break) [Closes #90]
dg added a commit that referenced this issue Jan 30, 2017
…t & Response (BC break) [Closes #90]
dg added a commit that referenced this issue Jan 30, 2017
…t & Response (BC break) [Closes #90]
dg added a commit that referenced this issue Feb 2, 2017
dg added a commit that referenced this issue Mar 5, 2019
…f Request & Response (BC break) [Closes #90]"

This reverts commit 2e43a58.
dg added a commit that referenced this issue Mar 9, 2019
…f Request & Response (BC break) [Closes #90]"

This reverts commit 2e43a58.
@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Mar 9, 2019

Fix reverted, solution is here #90 (comment)

dg added a commit that referenced this issue Mar 11, 2019
…f Request & Response (BC break) [Closes #90]"

This reverts commit 2e43a58.
dg added a commit that referenced this issue Mar 11, 2019
…f Request & Response (BC break) [Closes #90]"

This reverts commit 2e43a58.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.