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

[10.x] Do not use app() Foundation helper on ViewServiceProvider #51522

Merged
merged 1 commit into from
May 21, 2024
Merged

[10.x] Do not use app() Foundation helper on ViewServiceProvider #51522

merged 1 commit into from
May 21, 2024

Conversation

rodrigopedra
Copy link
Contributor

PR #51450 makes use of the app() helper to solve the view engine resolvers leaking memory issue.

The app() is provided by the Foundation component, while the fix was made into the View component.

Currently, the Foundation component is not a dependency of the View component.

This PR

  • Replaces the app() calls with Container::getInstance() calls where appropriate

Note that the View component already requires the Container component, and that the app() helper code basically delegates to Container::getInstance():

function app($abstract = null, array $parameters = [])
{
if (is_null($abstract)) {
return Container::getInstance();
}
return Container::getInstance()->make($abstract, $parameters);
}

No tests were added.

@rodrigopedra rodrigopedra changed the base branch from 11.x to 10.x May 21, 2024 11:35
@taylorotwell taylorotwell merged commit 4b60ec3 into laravel:10.x May 21, 2024
3 checks passed
@rodrigopedra rodrigopedra deleted the 10.x branch May 21, 2024 20:49
@rmn-eloha-jouny
Copy link

rmn-eloha-jouny commented May 27, 2024

Hello @taylorotwell and @rodrigopedra !

This commit is not included into the version v10.48.11 of the standalone package https://github.com/illuminate/view/commits/v10.48.11 and this is breaking some of my dependencies.

Is this possible to release a new version with it please ?

@driesvints
Copy link
Member

@rmn-eloha-jouny heya. I found the root cause of this but we're still thinking on how to address this. Rest assured that this change will be added to tomorrow's release.

@rmn-eloha-jouny
Copy link

@driesvints Thanks for the quick response!

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.

4 participants