Fix clobbering of the upload size validation

[nfauchelle]

Issue #10058

When the validation is set here like this, it overrides validation which has already been setup with a simple '*' rule for the size based on PHP.

If you've defined in the sites yml config something like

SilverStripe\Assets\Upload_Validator:
  default_max_file_size:
    '[image]': '2m'
    '*' : '1m'

then it will not be respected.

If you review SilverStripe\Assets\Upload_Validator and check the getAllowedMaxFileSize method, you'll see the sizing will be populated (if it hasn't been done before).

You can see it fail by;

• Setup a new SilverStripe site.
• Set your PHP to allow max post / max upload size of 10mb.
• Add the above config to your sites yml file and flush.
• In the CMS you'll be able to upload a 5MB file, when you shouldn't.

[nfauchelle]

Fixes #10059 #10058 :)
@michalkleiner PR'd.

[michalkleiner]

It's a PR for #10058, 10059 is the PR itself.

I don't think we can remove that block just like that, it removes setting the max size completely from the validator. It will need adjusting the logic, but not removing it.

[nfauchelle]

@michalkleiner It gets set when the validation is first actually called. If nothing has been defined in the config it falls back to original behaviour.

https://github.com/silverstripe/silverstripe-assets/blob/1/src/Upload_Validator.php#L117

Edit;
You could alter the method and check if the sizing has been set before it then tries to set the code. But the act of getting the current will set that size (based on that method above) so~

[michalkleiner]

Hmm, interesting the tests are passing, so it's either not being covered by tests properly or the code really wasn't needed.

[michalkleiner]

Adding a test specifically for this config scenario would be great, that way we'd be 100% sure.

[nfauchelle]

@michalkleiner tests added. The second test fails without my fix.

[nfauchelle]

@michalkleiner All good?

[michalkleiner]

Looks ok to me, though I can't approve it yet, so we need to wait for another core committer to give it a tick.
@maxime-rainville or @emteknetnz pretty please?

[emteknetnz]

Shouldn't it be something like this in order to fix the inverse problem? i.e. your config value is greater than the ini values?

$maxConfig = <get a config value if specified>
$min = $maxConfig ? min($maxUpload, $maxPost, $maxConfig) : min($maxUpload, $maxPost);
$this->getValidator()->setAllowedMaxFileSize($min);

If your config sets a max file size of say 5000 MB, which is above what's defined in php ini, then wouldn't php will block you from uploading it?

[nfauchelle]

@emteknetnz
I think you're looking at the wrong part of what this is about.

Currently if you do '[image]': '2m' then it gets ignored and will use the php max default since the validation doesn't bother to look at the config.

You'll see my commits removes that code which means it flows through to the code that does check the config and lets it work properly.

[emteknetnz]

This doesn't seem correct, I just tried testing this PR

I set my config to:

SilverStripe\Assets\Upload_Validator:
  default_max_file_size:
    '[image]': '500k'
    '*' : '1m'

And uploading a 700kb image, validation message was correct saying Filesize is too large, maximum 500 KB allowed

However when I changed my php.ini to upload_max_filesize = 50K (from 50M) and restarted the webserver, when I attempted to upload an 85kb image, I was shown the wrong error message Filesize is too large, maximum 500 KB allowed, I would expect it to be Filesize is too large, maximum 50 KB allowed

[nfauchelle]

@emteknetnz
If you roll back my change you'll find the first item - where the config settings got used in your test wont work anymore.

The bug your running into looks like an existing bug in core which is now being exposed in https://github.com/silverstripe/silverstripe-assets/blob/1/src/Upload_Validator.php#L117 - as we can see this existing code isn't checked if the config value will exceed PHP defaults.

Would you like me to fix that as part of this PR, or keep the PR specific?

[emteknetnz]

Seems like it should be fixed at the same time if you're able to do that. Seems logical that the validator should set the max size to be the minimum of (config, upload_max_filesize, post_max_size)

[nfauchelle]

@emteknetnz The fix I am going to make will be in the https://github.com/silverstripe/silverstripe-assets package, since that is where the Upload_Validator is.
Which branch you want the PR for that fix to go against?

[michalkleiner]

@nfauchelle 1.9 please and thanks

[nfauchelle]

PR added at silverstripe/silverstripe-assets#469

[nfauchelle]

So this PR now fixes two issues. The first being my original upload size validation not working.
My most recent commit fixes an issue in the Convert class dealing with strings with no numbers/empty which was throwing errors, which seperate PR in the assets package picked up on.

I believe we're all good to go here.

[emteknetnz]

I've left some comments on the corresponding assets pr - silverstripe/silverstripe-assets#469 (review)

[GuySartorelli]

I've rebased and retargetted to 2 so that this can be included in a future minor release.

Also bumped the assets constraint as per silverstripe/silverstripe-assets#469 (review) but otherwise left as-is.

changes were made

[emteknetnz]

Have tested this and associated PRs locally, they work good