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

Memory Leak #584

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

ihar-aleksiaichuk
Copy link

memory limit
reading file

memory limit
reading file
@codeclimate
Copy link

codeclimate bot commented Feb 12, 2019

Code Climate has analyzed commit a034c02 and detected 0 issues on this pull request.

View more on Code Climate.

@@ -136,11 +136,17 @@ protected function initFile()
{
if ($this->maxFileSize > 0 && file_exists($this->filename) && filesize($this->filename) >= $this->maxFileSize) {
// need to reduce the file size
$file = file($this->filename);
$file = array_splice($file, ceil(count($file) * $this->reduceRatio));
$reduceLen= ($this->maxFileSize< (int)ini_get('memory_limit')*1024?$this->maxFileSize:(int)ini_get('memory_limit')*1024)*$this->reduceRatio;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability it might be better to change this from the shorthand if else to the full controll structure:

if ($this->maxFileSize < (int)ini_get('memory_limit') * 1024) {
    $reduceLength = ($this->maxFileSize) * $this->reduceRatio;
} else {
    $reduceLength = ((int)ini_get('memory_limit') * 1024) * $this->reduceRatio;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A variable for (int)ini_get('memory_limit') * 1024 wouldn't also be out of place here

$file = array_splice($file, ceil(count($file) * $this->reduceRatio));
$reduceLen= ($this->maxFileSize< (int)ini_get('memory_limit')*1024?$this->maxFileSize:(int)ini_get('memory_limit')*1024)*$this->reduceRatio;
$size = filesize($this->filename)-$reduceLen;
$log =file_get_contents($this->filename, false, null, $size, $reduceLen );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to be consistent with spacing. For example always surround = with spaces (eg. $log = file_get_contents(...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please omit the whitespace after the last argument (in this case $reduceLen)

foreach ($file as $line) {
@fwrite($this->filehandle, $line);
foreach ($log as $line) {
@fwrite($this->filehandle, $line.PHP_EOL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the PSR-2 standard, please surround the concatenation sign (.) with spaces.

$line . PHP_EOL

$file = file($this->filename);
$file = array_splice($file, ceil(count($file) * $this->reduceRatio));
$reduceLen= ($this->maxFileSize< (int)ini_get('memory_limit')*1024?$this->maxFileSize:(int)ini_get('memory_limit')*1024)*$this->reduceRatio;
$size = filesize($this->filename)-$reduceLen;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please surround mathematical operators (+, -, *, /, %) with spaces

@martijn-tao
Copy link
Contributor

@ihar-aleksiaichuk Could you also add a comment on how to test the added functionality?

@alexeyvy
Copy link
Contributor

You should probably cover this case with a unit test

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.

3 participants