Framework: "app()->makeWith(..." does not work with "app()->instance(..."

[Eugene-Melbourne]

• Laravel Version: 5.6
• PHP Version: 7.1

Description:

function app()->makeWith(...
does not work with app()->instance(...

It should return prepared instance, but it creates a new object all the time.

It makes Mocking impossible. Unit-testing concept is not working.

[sisve]

The makeWith and make methods return new instances if you provide any parameters. How would Laravel know that the instance you've provided earlier is built using those same parameters?

[Eugene-Melbourne]

1) Make does not support parameters in laravel 5.6

2) Laravel ignores differences between bind and singleton if there is an instance provided. It is working great for mocking. I think it should also ignore parameters if the instance is given. For mocking purposes the provided instance should be different from what is built using some parameters. Laravel does not care about instance that is set by app()->instance(..., it may be an instance of a different type, why should it care about parameters?

[mfn]

I think it should also ignore parameters if the instance is given.

This does not feel right.

Getting some kind of instance from the service container with dynamic parameters is always a bit of hybrid and can pose some challenges, one of them is the problem of mocking.

For tests, you can use a factory class which will then create your instance based on your parameters. Which means, you return a mock for the factory which then is responsible for creating your mocked actual class. Sounds a bit complex, but in practice it's easy:

// Instead of
$instance = app()->makeWith(Concrete::class, ['your' => $args]);

// you can do
$instance = app(ConcreteFactory::class)->yourOwnMake($args);

You then mock ConreteFactory and return via yourOwnMake whatever you want, and it can also be dynamic based on your $args

[sisve]

1. Make does not support parameters in laravel 5.6

Yes, it does.

https://github.com/laravel/framework/blob/56a58e0fa3d845bb992d7c64ac9bb6d0c24b745a/src/Illuminate/Container/Container.php#L592-L602

[Eugene-Melbourne]

@mfn, thank you.

1) Your solution would work.

2) Your solution does not omit the fact that makeWith works different from normal Container behaviour. It is breaking the pattern.

3) I don't think that making a factory only for testing is the best practice. Many people think that you shouldn't do anything only because it is needed for testability.

[Eugene-Melbourne]

@sisve, I'm sorry. You are right. Thank you.

[Eugene-Melbourne]

What is going on? Why the issue is closed when the bug persists?

[devcircus]

I believe Taylor has commented a similar issue before saying that this is the expected behavior. I'll try to track it down.

[basepack]

Is there any news on this one?

[mfn]

@basepack nope, and nothing is planned. If you've a specific issue, please provide more information what your problem is and what you need help with.

[basepack]

Well, in my serviceprovider I now just do:

if ( $this->app->environment( 'testing' ) ) {
    app()->instance(
        ApiService::class,
        \Mockery::mock( ApiService::class )->makePartial()
    );
}

Not ideal, but it works.

[sisve]

That sounds like something that should be part of your test's setUp() method, not the service provider.

[basepack]

Thanks for your input @sisve, but: Yes, and that is exactly what not is working.
If you have a working setUp() method where you mock a service class, I will be gratefull for it.

But keep in mind this scenario:

// AppServiceprovider.php
if ( $this->app->environment( 'testing' ) ) {
    app()->instance(
        ApiService::class,
        \Mockery::mock( ApiService::class )->makePartial()
    );
}

// SomeTest.php

public function it_can_call_a_service_which_uses_api_service(){
    $anotherService = app('AnotherService::class');

   // This service calls ApiService which is _correctly_ mocked at this stage
   // If you swap the ServiceProvider implementation to the setUp() method it will _not_ work
   $anotherService->callApiService(); 
}

I believe it will work if you use setUp() and then test/call ApiService directly, however if you are testing AnotherService which uses the IOC container to inject ApiService. It will not get the mocked version, but the real implementation.
So thats why we need this service provider hackish way, which I do not like.

[mfn]

I'm using mocked services in setUp all the time, no problems.

How does your setUp look exactly?

How does the code, which resolves the ApiService look like (or the constructor of AnotherService)?

[basepack]

Wow, if that does it for you, you will save my life with this one!

Here it comes:

// AnotherService.php
class AnotherService {

    private $apiService;

    public function __construct( ApiService $apiService )
    {
        $this->apiService = $apiService;
    }

    public function initSubscription( )
    {
        $this->apiService->createAccount();
    }
}

// ApiService.php
class ApiService {

    public function createAccount( )
    {
        // Call the API
    }
}


// AnotherServiceTest.php
class AnotherServiceTest extends TestCase
{
    /** @var AnotherService*/
    protected $anotherService;

    /** @var Mockery\Mock */
    protected $mockApiService;

    public function setUp()
    {
        parent::setUp();

        $this->anotherService = app( AnotherService::class );

        app()->instance(
            ApiService::class,
            Mockery::mock( ApiService::class )->makePartial()
        );

        $this->mockApiService = app( ApiService::class );

    }

    public function it_can_call_a_service_which_uses_api_service(){
        // arrange
        // ...

        // expect
        $this->mockApiService
            ->shouldReceive( 'createAccount' )
            ->once();

        // act
        $this->anotherService->initSubscription();
    }
}

[sisve]

My first thought is that you have already resolved and cached an AnotherService somewhere. Is it perhaps a singleton registered from within your service provider? Then it would keep using whatever reference to ApiService it had from the beginning, and not know about any mocked instances introduced in the Container at a later time.

[basepack]

@sisve, no it is never registered in a service provider. I thought this was exactly why this issue was created for. Because this is unexpected behavior.
Maybe I should mention https://github.com/laravel/framework/issues/19450 where this is also described.

The most annoying thing is: I can't run my full testsuite now, because of that ugly serviceprovider I need for the mockery to work:

if ( $this->app->environment( 'testing' ) ) {
    app()->instance(
        ApiService::class,
        \Mockery::mock( ApiService::class )->makePartial()
    );
}

If I run all tests at once, the specific tests of ApiService now fails of course.

[mfn]

The order in your setUp is wrong:

• your first create an instance of AnotherService _which_ uses ApiService so it will immediately create non-mocked instance of ApiService for that service
• only a line later your set your mocked instance

Switch the two lines and it should work.

[basepack]

@mfn, wow thank you very much! Never expected something that obvious! Will you believe I worked a full day on this one..
So in essence @sisve was right, expecting an already cached instance of ApiService when he says:

My first thought is that you have already resolved and cached an AnotherService somewhere.

[Sti3bas]

You can bypass this with offsetSet:

app()->makeWith(Concrete::class, ['foo' => 'bar']);

// in test
app()->offsetSet(Concrete::class, $mock);

[mikepmtl]

In case someone else is looking for this. You can mock it like this in your tests.
Use bind() instead of instance()

   $orders_mock = \Mockery::mock(OrdersRequest::class)->makePartial();
   $orders_mock->shouldReceive('send')->andReturn($order_response);
   $this->app->bind(OrdersRequest::class, function() use ($orders_mock){
       return $orders_mock;
   });

And it will use the Mock.

[mfn]

Use bind() instead of instance()

That's interesting because I don't see anything in your example which would require that; IMHO $this->app->instance(OrdersRequest::class, $orders_mock); should work equally well, no?