Framework: [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 https://github.com/laravel/framework/commit/ebe568c440cee4998765de7f23813f8dac560d56 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: https://github.com/laravel/framework/pull/18812

The 5.4.15 to 5.4.16 commit where the container changed was here: https://github.com/laravel/framework/commit/5d9b363448b9aac83b610166141b8bba9f1807f8

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:

• https://github.com/proengsoft/laravel-jsvalidation/issues/237
• https://github.com/proengsoft/laravel-jsvalidation/issues/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...

[antonkomarev]

No issues in the jsvalidation package with your fix! Great job @laurencei !

[sisve]

As mentioned on Slack, https://github.com/laravel/framework/pull/19161 fixed my issues too. Rejoice!