Skip to content

Commit

Permalink
Change file size retrieval to use PHP functions as shell exec has nev…
Browse files Browse the repository at this point in the history 
…er worked here because of the douple escapeshellargs
  • Loading branch information
@Sebastian-Roth
Sebastian-Roth committed Nov 4, 2018
1 parent 5b28264 commit 11bcc40
Showing 1 changed file with 2 additions and 6 deletions.
8 changes: 2 additions & 6 deletions packages/web/lib/fog/fogbase.class.php
View file
Expand Up @@ -2302,11 +2302,8 @@ public static function lasterror()
*/
public static function getFilesize($file)
{
$file = escapeshellarg($file);

return trim(
shell_exec("du -b $file | awk '{print $1}'")
);
$size = filesize($file);
return isset($size) ? $size : 0;
}
/**
* Perform enmass wake on lan.
Expand Down Expand Up @@ -2420,7 +2417,6 @@ public static function fastmerge($array1)
*/
public static function getHash($file)
{
$file = escapeshellarg($file);
$filesize = self::getFilesize($file);
$fp = fopen($file, 'r');
if ($fp) {
Expand Down

6 comments on commit 11bcc40

@mastacontrola
Copy link
Member

Choose a reason for hiding this comment

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

Part of the reason we used shell for getting the filesize is due to limit's on 32 bit vs 64 bit numerals.

Using the shell method, we get back a string which can be parsed regardless of the returned number.

@Sebastian-Roth
Copy link
Member Author

Choose a reason for hiding this comment

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

@mastacontrola The major issue with this was actually the douple escapeshellarg call. Filenames would ended up as ''/path/filename.ext'' and ended up hashing even all the bigger files too because $filesize was literally always zero (when called from within the getHash function).

@Quazz
Copy link

@Quazz Quazz commented on 11bcc40 Jun 13, 2019 •
edited

Choose a reason for hiding this comment

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

@Sebastian-Roth

I noticed the FOGImageSize service makes use of this function, which would explain why the image size on server is reported as 4K since that's the directory size itself.

It seems pretty difficult in PHP to accurately get size of a directory content.

This one may work though:

function dir_size($directory) { $size = 0; foreach(new RecursiveIteratorIterator(new RecursiveDirectoryIterator($directory)) as $file) { $size += $file->getSize(); } return $size; }
(obviously would need to be altered to work properly)
from https://stackoverflow.com/questions/478121/how-to-get-directory-size-in-php

Then again, I'm not sure if that would then still work for files only. Perhaps we need to check if it's a file or directory and check based on that info, that's not too hard to do.

@Sebastian-Roth
Copy link
Member Author

Choose a reason for hiding this comment

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

@Quazz Just pushed a commit to hopefully address this: e3b754e

@Quazz
Copy link

@Quazz Quazz commented on 11bcc40 Jul 8, 2019

Choose a reason for hiding this comment

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

@Sebastian-Roth Looks good, it's reporting correct sizes for a good number of images, though some report back 0 for some reason. That said, I don't think that's down to this function at this point since it does work in manual tests!

@mastacontrola
Copy link
Member

Choose a reason for hiding this comment

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

I've updated working-1.6 with a few minor changes.

For example, continuing the loop using the RecursiveIteratorIterator method isDot(). Also, it will default a size based on the path, but if the path is a directory it will do the recursion. Less logic checking = microoptimization, but also easier to follow logic.

Please sign in to comment.