-
Notifications
You must be signed in to change notification settings - Fork 11k
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] Fixes memory leak on anonymous migrations #46061
Conversation
@@ -557,7 +568,9 @@ public function getMigrationFiles($paths) | |||
public function requireFiles(array $files) | |||
{ | |||
foreach ($files as $file) { | |||
$this->files->requireOnce($file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static::$requiredMigrations[$file]
??= $this->files->requireOnce($file);
Try this instead of IF
I wonder if php will ever fix the anonymous global function memory leaking 🙃 |
@@ -557,7 +568,9 @@ public function getMigrationFiles($paths) | |||
public function requireFiles(array $files) | |||
{ | |||
foreach ($files as $file) { | |||
$this->files->requireOnce($file); | |||
if (! isset(static::$requiredMigrations[$file])) { | |||
static::$requiredMigrations[$file] = $this->files->requireOnce($file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly, require_once only returns true
if the file was successfully required and does not return the return value of the file. That might be unintended at this stage
Thank you for looking into this so quickly! |
This can be fixed by cloning the cached migration before returning it. I've tested this using your (@dbohn) example repository and the memory seems to remain the same. This would mean that this pull request could actually target the 9.x branch.
|
@rojtjo Excellent idea. Going to update the pull request. |
Replaced by #46073. |
This pull request fixes a memory leak on Laravel test suites that are using anonymous migrations.
// Work in progress.
Fixes #46029.