Show image size limit in Frio as "usagemessage" for photo_upload #12267

MarekBenjamin · 2022-11-25T18:48:42Z

As a first step on no image size limit shown on upload site issue , the configured limit is now printed in the already existing but previously empty usageinfo in the frio template for photos_upload.tp

The value of maximagesize in bytes is converted to MB before printing.

Please comment this request regarding code quality and practice.

MarekBenjamin · 2022-11-25T18:50:41Z

forget to run run_xgettext.sh, fixing it.

mod/photos.php

Co-authored-by: Hypolite Petovan <hypolite@mrpetovan.com>

…filesize' is dominant for the actual image upload limit and print out the lower (relevant) one.

MrPetovan · 2022-11-26T00:02:26Z

mod/photos.php

+		// init output var in case no number value could be retrieved.
+		$maximagesize_Mbytes = 0;
+		$sizelimitedby = '';
+		{


What is this curly bracket block for?

Two reasons:

Got the feeling this might be a little messy to be placed inline in photos_contens.php and should be moved to a function, e.g. to be reused in the Admin Site.

But until I am not sure where to put it best as a Function, I used the local block for hiding the wars to the rest of the function to emphasize that they are just used for the conversion and comparison.

But the curly brackets can be removed at will, no problem at all :-)

MrPetovan · 2022-11-26T00:07:48Z

mod/photos.php

+			// Get the relevant size limits for uploads. Abbreviated var names: MaxImageSize -> mis; upload_max_filesize -> umf
+			$mis_bytes = DI::config()->get('system', 'maximagesize');
+			// get_cfg_var('upload_max_filesize') outputs a string in the shorthand notation, need to convert it to bytes
+			$umf_string = get_cfg_var('upload_max_filesize');


Please use ini_get() instead, it will return the computed value as an integer value or an integer string you can use directly.

At least not on my setup

gettype(ini_get('upload_max_filesize')): String
ini_get('upload_max_filesize'): 20M

Gasp!

Never mind then, here's a conversion function you should use (notice the 1024 factor instead of the 1000 you've been using): https://stackoverflow.com/a/6846537

You can declare it in the src/Util/Strings.php file with the signature public static function getBytesFromShorthand(string $val): int and then you can refer to it in mod/photos.php as Strings::getBytesFromShorthand().

Thanks, I will do so shortly.

Already thought about the binary / si prefix issue myself, which is also related to the issue: Whenever I encounter a si prefix (kB, MB, ...) I assume it is decimal prefix 10**x .
When it is about binary prefixes, shouldn't we use KiB, MiB, Gib?

But for the moment: Which interpretation of k, M, G is used in this project: 10**x (si / deicmal) or 2**x (binary). I never be sure in the k, M, G cases, only when binary prexies are used it is celar for me that they relate to 2**x :-)

The PHP manual you linked to earlier mentions the shorthands are meant to be binary prefixes.

…tation to int

mod/photos.php

src/Util/Strings.php

Co-authored-by: Hypolite Petovan <hypolite@mrpetovan.com>

… cluclate 2**20 for just deviding the bytes by one constant divisor.

mod/photos.php

… calculate 2**20 for just dividing the bytes by one constant divisor.

# Conflicts: # src/Util/Strings.php # view/lang/C/messages.po

MarekBenjamin · 2022-11-30T01:24:03Z

While wrapping Strings::getBytesFromShorthand() at the remaining locations, I found that the case $maximagesize = 0 is not handled as INFINITY but as literally 0, e.g. in Photo.php:982

if ($filesize > $maximagesize) {
				Logger::notice('Image size is too big', ['size' => $filesize, 'max' => $maximagesize]);
				return null;
			}

Should we not convert a $maximagesize = 0 to $maximagesize = INF for such cases?

…) with Strings::getBytesFromShorthand()

MrPetovan · 2022-11-30T01:33:15Z

I've never used infinity, but feel free to do so if tests are successful.

MarekBenjamin · 2022-11-30T01:40:14Z

Already started it, should not be a problem as long as we have INF only in the numerators.

MarekBenjamin · 2022-11-30T02:08:06Z

For me, at the moment anything is done besides a general discussion about how we handle limit of -1 and 0 in general, but I created a new issue for that. This PR now should behave as defined with 0 meaning actually no limited and any size is accepted where ever the image size is evaluated.

MarekBenjamin · 2022-11-30T03:05:45Z

There still seams to be a problem with formatBytes(int $bytes, int $precision = 2) in Strings.php:221 when no limited == INF is set. So no pull at the moment.

MarekBenjamin · 2022-11-30T03:16:38Z

Fixed it. But there is this method Strings::formatBytes($maximagesize)) in Strings:221 which, in theory, never should be called with a maximagesize values of INF but I expect it to crash when it called with INF for the value.

Will suggest a handling for INF in the next commit.

…rings

…sion

MarekBenjamin · 2022-11-30T03:40:16Z

Should all be fixed, upload now works for profile pics. Main Upload still broken due to another issue not addressed here

MrPetovan

I'm not convinced by the use of INF, we were already checking whether maximagesize had a value or not, which is the same condition as replacing its value by INF.

MrPetovan · 2022-11-30T03:55:20Z

mod/photos.php

+
+	if ($maximagesize == 0) {
+		$maximagesize = INF;
+	}

 	if ($maximagesize && ($filesize > $maximagesize)) {


This condition was already effectively using 0 to disable the check, not sure using INF here adds anything.

That is the point, if we pass maximagesize == 0 to line 663, the processing will be discarded and since fileesize will always > 0. And this is contradicting the semantic that maximagesize == 0 should effect "no limit"

Ahh sorry, now I get it (as you already guessed I am not too familiar with PHP ;-) ) since the AND condition is already false because PHP trades "0" as false. I get it. You are right, then it is not necessary.

There are two variants of this if statement in line 663, the other one is !isempty($maxfilesize). I will adjust it by removing isempty() .

Thanks, it bothered me too but I didn't want to pile on you.

MrPetovan

One last change!

src/Module/Media/Photo/Upload.php

Co-authored-by: Hypolite Petovan <hypolite@mrpetovan.com>

MrPetovan

Thank you for your work!

Show image size limit in Frio as "usagemessage" for photo_upload

d402fe3

updated messages.po

c11e2b0

MrPetovan requested changes Nov 25, 2022

View reviewed changes

mod/photos.php Outdated Show resolved Hide resolved

mod/photos.php Outdated Show resolved Hide resolved

MarekBenjamin and others added 3 commits November 25, 2022 21:53

Update mod/photos.php

9dd1cf8

Co-authored-by: Hypolite Petovan <hypolite@mrpetovan.com>

Added check if upload_max_filesize (php.ini) or friendica option 'max…

bb67f6e

…filesize' is dominant for the actual image upload limit and print out the lower (relevant) one.

message.po once more

74bab60

MarekBenjamin requested a review from MrPetovan November 25, 2022 22:05

changed var name

c4c1bd2

MrPetovan requested changes Nov 26, 2022

View reviewed changes

MarekBenjamin mentioned this pull request Nov 26, 2022

Show max. image upload size on photo upload page #7807

Closed

MarekBenjamin marked this pull request as draft November 26, 2022 20:02

Marek Bachmann added 4 commits November 26, 2022 21:43

introduced getBytesFromShorthand function to covert from shorthand no…

907deed

…tation to int

fixed a forgotten debug output

f4d49e5

Better formatting

f88722b

whitespaces

6c73077

MrPetovan requested changes Nov 26, 2022

View reviewed changes

mod/photos.php Outdated Show resolved Hide resolved

mod/photos.php Outdated Show resolved Hide resolved

src/Util/Strings.php Outdated Show resolved Hide resolved

src/Util/Strings.php Outdated Show resolved Hide resolved

MrPetovan added this to the 2022.12 milestone Nov 26, 2022

MrPetovan added the Enhancement label Nov 26, 2022

MarekBenjamin and others added 4 commits November 26, 2022 23:12

Update src/Util/Strings.php

ce56cab

Co-authored-by: Hypolite Petovan <hypolite@mrpetovan.com>

Update src/Util/Strings.php

765b50b

Co-authored-by: Hypolite Petovan <hypolite@mrpetovan.com>

Integrated isnumeric check to limit comparison condition

54289b2

using constant value for calculation of MiB output, really no need to…

597f1ba

… cluclate 2**20 for just deviding the bytes by one constant divisor.

MrPetovan requested changes Nov 26, 2022

View reviewed changes

mod/photos.php Outdated Show resolved Hide resolved

Marek Bachmann added 2 commits November 26, 2022 23:44

using constant value for calculation of MiB output, really no need to…

f45b358

… calculate 2**20 for just dividing the bytes by one constant divisor.

Added info about upload_max_filesize to admin page

719c294

MarekBenjamin marked this pull request as ready for review November 27, 2022 00:29

Marek Bachmann added 2 commits November 27, 2022 23:52

Merge branch 'develop' into show_image_upload_limit

a01872a

# Conflicts: # src/Util/Strings.php # view/lang/C/messages.po

messages.po

6ebf0f8

MarekBenjamin requested a review from MrPetovan November 27, 2022 22:58

Marek Bachmann added 2 commits November 30, 2022 02:26

Wrapped all occurrences of DI::config()->get('system', 'maximagesize'…

bbd1c34

…) with Strings::getBytesFromShorthand()

Merge branch 'develop' into show_image_upload_limit

ca7af7a

MarekBenjamin mentioned this pull request Nov 30, 2022

Semantic of $maximagesize = 0 in code is not unlimited but litteraly 0 #12297

Closed

Added handling as infinity if maxfilesize is set to 0

97a3587

MarekBenjamin marked this pull request as ready for review November 30, 2022 02:03

MarekBenjamin marked this pull request as draft November 30, 2022 02:58

Added getBytesFromShorthand at the remaining places.

bb9afc5

Marek Bachmann added 4 commits November 30, 2022 04:34

use the existing function for format byte values to KiB, MiB, GiB

037e181

Check for inf values before try to converts bytes to binary prefix st…

75b01f6

…rings

Correctly output binary prefix values since base 2 is used for conver…

a3d8a59

…sion

message.po

0080832

MarekBenjamin requested a review from MrPetovan November 30, 2022 03:40

MarekBenjamin marked this pull request as ready for review November 30, 2022 03:40

MrPetovan reviewed Nov 30, 2022

View reviewed changes

Removed redundant maximagesize = INF statements

79235b6

MarekBenjamin requested a review from MrPetovan November 30, 2022 16:08

MrPetovan requested changes Nov 30, 2022

View reviewed changes

src/Module/Media/Photo/Upload.php Outdated Show resolved Hide resolved

src/Module/Media/Photo/Upload.php Outdated Show resolved Hide resolved

MarekBenjamin and others added 3 commits November 30, 2022 18:16

Update src/Module/Media/Photo/Upload.php

f9c0d5a

Co-authored-by: Hypolite Petovan <hypolite@mrpetovan.com>

Update src/Module/Media/Photo/Upload.php

e4fd9d4

Co-authored-by: Hypolite Petovan <hypolite@mrpetovan.com>

Merge branch 'friendica:develop' into show_image_upload_limit

8f5d6fd

MrPetovan approved these changes Nov 30, 2022

View reviewed changes

MrPetovan merged commit 35ca496 into friendica:develop Nov 30, 2022

MarekBenjamin deleted the show_image_upload_limit branch November 30, 2022 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show image size limit in Frio as "usagemessage" for photo_upload #12267

Show image size limit in Frio as "usagemessage" for photo_upload #12267

MarekBenjamin commented Nov 25, 2022

MarekBenjamin commented Nov 25, 2022

MrPetovan Nov 26, 2022

MarekBenjamin Nov 26, 2022 •

edited

MarekBenjamin Nov 26, 2022 •

edited

MrPetovan Nov 26, 2022

MarekBenjamin Nov 26, 2022

MrPetovan Nov 26, 2022

MarekBenjamin Nov 26, 2022

MrPetovan Nov 26, 2022

This comment was marked as resolved.

This comment was marked as resolved.

MarekBenjamin commented Nov 30, 2022

MrPetovan commented Nov 30, 2022

MarekBenjamin commented Nov 30, 2022

MarekBenjamin commented Nov 30, 2022

MarekBenjamin commented Nov 30, 2022

MarekBenjamin commented Nov 30, 2022 •

edited

MarekBenjamin commented Nov 30, 2022

MrPetovan left a comment

MrPetovan Nov 30, 2022

MarekBenjamin Nov 30, 2022

MarekBenjamin Nov 30, 2022

MarekBenjamin Nov 30, 2022

MrPetovan Nov 30, 2022

MrPetovan left a comment

MrPetovan left a comment

Show image size limit in Frio as "usagemessage" for photo_upload #12267

Show image size limit in Frio as "usagemessage" for photo_upload #12267

Conversation

MarekBenjamin commented Nov 25, 2022

MarekBenjamin commented Nov 25, 2022

Choose a reason for hiding this comment

MarekBenjamin Nov 26, 2022 • edited

Choose a reason for hiding this comment

MarekBenjamin Nov 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

MarekBenjamin commented Nov 30, 2022

MrPetovan commented Nov 30, 2022

MarekBenjamin commented Nov 30, 2022

MarekBenjamin commented Nov 30, 2022

MarekBenjamin commented Nov 30, 2022

MarekBenjamin commented Nov 30, 2022 • edited

MarekBenjamin commented Nov 30, 2022

MrPetovan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrPetovan left a comment

Choose a reason for hiding this comment

MrPetovan left a comment

Choose a reason for hiding this comment

MarekBenjamin Nov 26, 2022 •

edited

MarekBenjamin Nov 26, 2022 •

edited

MarekBenjamin commented Nov 30, 2022 •

edited