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

[5.4] Fix container Build() #19161

Merged
merged 1 commit into from
May 11, 2017
Merged

[5.4] Fix container Build() #19161

merged 1 commit into from
May 11, 2017

Conversation

laurencei
Copy link
Contributor

@laurencei laurencei commented May 11, 2017

So this PR attempts to fix a bug that I think was introduced in 5.4.16 (5d9b363), but only became visible in 5.4.19 (ebe568c)

It was identified in this issue: #19122

Basically if you run make() or makeWith() - they will both call resolve() which will correctly set the parameter stack (using $this->with).

Depending on the path - resolve() may call build() - so the build() function uses the parameter stack as part of the process - which is all correct.

But the problem is you can also call build() directly (it is a public method). In this situation the parameter stack is never set (because it is not used) - but the function assumes it was. When the check on the parameter stack occurs, it can return false if the array was empty.

In 5.4.16 -> 5.4.18 this was not noticed, because the parameter stack would always have arrays (incorrectly) left over from previous use (that was the memory leak fixed in 5.4.19) - and thus would never return false. It would actually even possibly return an incorrect value from the previous build.

But in 5.4.19 we stopped the eronous/incorrect population of the parameter stack when it was not used, and now you can sometimes have a situation where build() checks an empty parameter stack.

Further - the reason all the test pass for 5.4.16 onwards is that there does not seem to be any build() tests in any of the framework unit tests? There are tests for make() etc that can call build() - but none for build() itself which is a public method - which is how this slipped through.

I've added a couple of tests that would fail without this change, to help prevent regression.

@taylorotwell taylorotwell merged commit 0b934bc into laravel:5.4 May 11, 2017
@laurencei laurencei deleted the fix_container branch May 11, 2017 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants