[5.4.19] Breaking change in Container::build

[sisve]

• Laravel Version: 5.4.19 (and all later)
• PHP Version: 7.1.1

Description:

The commit ebe568c causes the provided tests to fail in 5.4.19. The test works in 5.4.18.

Steps To Reproduce:

class ZomgTest extends TestCase {
    use CreatesApplication;

    public function testZomg() {
        $this->app->build(ZomgService::class);
        $this->assertTrue(true); // make phpunit happy
    }

}

class ZomgService {
    public function __construct(ZomgRepository $userRepository) {
    }
}

class ZomgRepository {
}

Result:

ErrorException: array_key_exists() expects parameter 2 to be array, boolean given

~/vendor/laravel/framework/src/Illuminate/Container/Container.php:794
~/vendor/laravel/framework/src/Illuminate/Container/Container.php:769
~/vendor/laravel/framework/src/Illuminate/Container/Container.php:746
~/tests/ZomgTest.php:14

[themsaid]

Please explain more why you think that commit broke your code

[sisve]

Because manually reverting that change (moving the line back into the original place) makes my test work again, both on 5.4.19 and 5.4.22 (latest).

[laurencei]

5.4.19 solves the memory leak introduced in 5.4.16 - so its not a matter of simply reverting it. As seen here: #18812

The 5.4.15 to 5.4.16 commit where the container changed was here: 5d9b363

If you run that same test on Laravel 5.4.15 - what result do you get?

[laurencei]

@sisve - if you replace your resolve() with the resolve() from the git below - that should fix your problem?

[antonkomarev]

This change broke jsvalidation package:

• array_key_exists() expects parameter 2 to be array, boolean given (View: C:\wamp\www\aot\resources\views\pages\subjectcategory\add_subject_category.blade.php) proengsoft/laravel-jsvalidation#237
• [L5.4] seems like last two days you and laravel both released new package and by that some error was occured, so kindly check it out. proengsoft/laravel-jsvalidation#236

[antonkomarev]

@laurencei I've tested gist you've provided. It didn't fixed issue in jsvalidation package.

[antonkomarev]

This happens because $this->with is empty array and end($this->with) returns false in hasParameterOverride method. Issue is hidden here.

Not working:

if (isset($this->instances[$abstract]) && ! $needsContextualBuild) {
    array_pop($this->with);
    return $this->instances[$abstract];
}

Working:

if (isset($this->instances[$abstract]) && ! $needsContextualBuild) {
    return $this->instances[$abstract];
}

[laurencei]

@a-kokarev really? The very first line of the gist I listed above sets the array - so I don't see how it could be empty? Did you copy and paste the entire function?

[antonkomarev]

@laurencei Yes, I've copied entire function.

[sisve]

@laurencei, @a-komarev Have you tried the test I provided in the issue?

I've give you exact code I can use to reproduce the issue, and haven't seen any response regarding if it's a faulty reproduction, if no one else has the issue and it's something in my end, etc. At the moment I'm asked to explain why I some code broke something, but I believe that I've already proven that something broke (and I also believe I am very sure what caused it).

If my test can reproduce the issue, then it can probably also verify the fix. That means we can get this solved quickly instead of waiting around for others to confirm that the guessed fix in a separate branch somewhere did or did not fix the issue.

So, can anyone confirm that this indeed a breaking change, or is the test that I think can reproduce it wrong?

[laurencei]

@a-komarev - does the package work with 5.4.16? Can you please test and advise? @sisve - I plan on testing your code etc when I get home in a few hours. I'll report back then with more info.

[antonkomarev]

@laurencei Yes, tested it right now. It's working on 5.4.16

[laurencei]

Ok - so I've done some testing. The changes in 5.4.19 do result in the above, but I think it might actually stem from 5.4.16 and were just hidden. I'll keep working on it and give some results back shortly...

[laurencei]

Ok - I've done a PR and Taylor has accepted it.

That should hopefully have fixed the issues in this thread?

Please confirm if you still run into any issues...