Page MenuHomeIn-Portal Phabricator

INP-1721 Correct math for detecting allowed file size during upload
ClosedPublic

Authored by erik on Oct 25 2018, 7:38 AM.

Details

Summary

changed upload limit calculation logic

Test Plan

Static test code:

echo MAX_UPLOAD_SIZE; exit;

Dynamic test code:

ini_set('memory_limit', '');

Test Plan

  1. Put static test code in the index.php, after $application->Init();
  2. Put dynamic test code in the index.php, before $application->Init();
  3. For below listed test data confirm, that MAX_UPLOAD_SIZE constant value is calculated correctly using this data:
    • 1G, 75000K, '50M' => 52428800
    • 1G, 75000K, '100M' => 76800000
    • 300000, 7500K, '100M' => 300000
    • 300000, 0, '100M' => 300000
    • 300000, 4K, '-1' => 4096
    • 300000, 0, '-1' => 300000

How to read test data:

  • first value goes into upload_max_filesize setting from php.ini file
  • second value goes into post_max_size setting from php.ini file
  • third value is set as second parameter in the dynamic test code
  • value after => is what should be displayed on the screen
  • restarting web server & memcache before starting each test

Diff Detail

Repository
rINP In-Portal
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

erik created this revision.Oct 25 2018, 7:38 AM
alex requested changes to this revision.Oct 25 2018, 8:27 AM
alex added inline comments.
core/kernel/globals.php
1007–1025 ↗(On Diff #869)

Considering that method is called several times (3 at max) during each page load you though refactor it to make it maximally fast (maybe examples exist on the internet).

Several ideas to consider, but feel free to do what you think is best:

  1. doing single preg_match (e.g. /^([\d]+)(k|b|m)$/) and then take actual multiplier (if found from mapping array) or if not found return integer number in bytes
  2. doing switch/case to get actual multiplier
  3. attempt to exit early if no letter is found
  4. and so on
core/kernel/startup.php
118–123 ↗(On Diff #869)

Please wrap all that in self-invoking anonymous function (aka closure).


All these temp variables used to calculate constant value pollute global scope.

120 ↗(On Diff #869)

Please replace $post_max_size !== '0' with (string)$post_max_size !== '0'.


I'm not sure if ini_get is doing any automatic type casting or not.

122 ↗(On Diff #869)

Please replace $memory_limit !== '-1' with (string)$memory_limit !== '-1'.


I'm not sure if ini_get is doing any automatic type casting or not.

This revision now requires changes to proceed.Oct 25 2018, 8:27 AM
alex added a comment.Oct 25 2018, 8:28 AM

Also the test plan doesn't cover a glimpse of what's actually being checked in code. To test what you've done you need to manually change values of corresponding setting via ini_set (to cover each if/else created) before calling new code from startup.php to assert that result is desired number of bytes.

erik edited the test plan for this revision. (Show Details)Oct 25 2018, 12:04 PM
erik updated this revision to Diff 870.Oct 25 2018, 12:09 PM
erik edited edge metadata.

Improvement for calculations result caching

alex edited the test plan for this revision. (Show Details)Oct 26 2018, 3:58 AM
alex edited the test plan for this revision. (Show Details)Oct 26 2018, 4:06 AM
alex accepted this revision.Oct 26 2018, 4:19 AM
This revision is now accepted and ready to land.Oct 26 2018, 4:19 AM
This revision was automatically updated to reflect the committed changes.