Skip to content

show file size - #26

Open
sluc23 wants to merge 2 commits into
adixon:masterfrom
ixiam:2.x-devShowFileSize
Open

show file size#26
sluc23 wants to merge 2 commits into
adixon:masterfrom
ixiam:2.x-devShowFileSize

Conversation

@sluc23

@sluc23 sluc23 commented Jun 13, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@demeritcowboy

Copy link
Copy Markdown
Collaborator

Looks nice 👍

untitled2

Comment thread CRM/Logviewer/Page/LogViewer.php Outdated
$filesize = filesize($logFileName);
$sz = 'BKMGTP';
$factor = floor((strlen($filesize) - 1) / 3);
$filesize = sprintf("%.2f", $filesize / pow(1024, $factor)) . @$sz[$factor];

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is some pretty cool and scary code to generate the file size in appropriate units! Did you make this up or is it pretty battle tested?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Okay, seems like there are a plethora of examples similar to this out on the web and that this should work most of the time. I suppose since this isn't run-time critical code I should be less concerned about edge cases, but for example - what happens if filesize is empty? Then factor is negative one, and all hell breaks loose? Using sz as a string and then getting an index of it is clever but scary for those of us who remember pointer arithmetic and out-of-bounds disasters. So ... I think this is okay except for a test for length of filesize > 0 and sz as an array instead of string.

@adixon

adixon commented Jun 21, 2024

Copy link
Copy Markdown
Owner

@twomice I'm looking at you and what did I say about scope creep?

@twomice

twomice commented Jun 24, 2024

Copy link
Copy Markdown
Contributor

@adixon Er, well ... [stares at ceiling awkwardly]

@sluc23

sluc23 commented Jun 25, 2024

Copy link
Copy Markdown
Contributor Author

@adixon I've found this code in the web, and I thought it was interesting to apply a different approach on how filesize is calculated, just to have some variety on the classic approach to get the file size. I'm using it already in prod as a patch, and it works fine.

If you have doubts about it, I can rework it in a classic approach. tx!

Co-authored-by: demeritcowboy <demeritcowboy@hotmail.com>
@gngn23

gngn23 commented Apr 1, 2026

Copy link
Copy Markdown

My two cents:

a) I think it's better to add the filesize after the file name at the top of the page (instead of displaying it on the download button).

b) human readable filesize
I would have thought that CiviCRM already has a function to get human readable filesize from bytes.
But I did not find anything...

The pretty cool and scary code in the PR is mentioned in the "User Contributed Notes" in the php.net documentation of filesize() along with many others.

I like one I found on stackoverflow:

/**
 * Converts a long string of bytes into a readable format e.g KB, MB, GB, TB, YB
 * 
 * @param {Int} num The number of bytes.
 */
function readableBytes($bytes) {
    $i = floor(log($bytes) / log(1024));

    $sizes = array('B', 'KB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB', 'YB');

    return sprintf('%.02F', $bytes / pow(1024, $i)) * 1 . ' ' . $sizes[$i];
}

I think we should use a function for this.

@gngn23

gngn23 commented Apr 2, 2026

Copy link
Copy Markdown

Digging a little deeper into PHP's filesize() I noted

Because PHP's integer type is signed and many platforms use 32bit integers, some filesystem functions may return unexpected results for files which are larger than 2GB.
https://www.php.net/manual/en/function.filesize.php

But I could offer a PR like proposed above - should I?

@adixon

adixon commented Apr 2, 2026

Copy link
Copy Markdown
Owner

PR welcome!

@gngn23 gngn23 mentioned this pull request Apr 2, 2026
@gngn23

gngn23 commented Apr 2, 2026

Copy link
Copy Markdown

I created a new PR #40

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.

5 participants