getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.32k stars 168 forks source link

[3.6.0-rc.1 ] minwidth in file blueprint not working #3849

Closed MatthiasWeisz closed 3 years ago

MatthiasWeisz commented 3 years ago

Describe the bug
The minwidth property in file blueprint doesnt lead to upload permission and error message. Instead smaller files can regulary be uploaded.

To Reproduce
Steps to reproduce the behavior:

  1. Check out kirby 3.6
  2. set minwidth property in file blueprint
  3. upload smaller file in panel File is being uploaded

Expected behavior
File upload is permitted

Screenshots

Kirby Version
3.6.0-rc-1

afbora commented 3 years ago

Related commit https://github.com/getkirby/kirby/commit/139d7880f1852b59039efad0ca69d37b9e8853c8#diff-7031b31652730041abe5f4d32a50811dbe847ec7a189b645f7e9b533979fa370 in https://github.com/getkirby/kirby/pull/3285 PR

afbora commented 3 years ago

@distantnative I guess Image::$validations overriden by File::$validations

distantnative commented 3 years ago

But why? Image extends File and should override it, not the other way around. Need to look at this back in the computer.

afbora commented 3 years ago

Yes, you're right. I'll keep digging..

afbora commented 3 years ago

I found the problem commit that Image class replaced with File in #3285 PR

bastianallgeier commented 3 years ago

@distantnative @afbora can we revert that without too many side effects. I think we cannot launch 3.6 with this issue.

afbora commented 3 years ago

I'm not sure but I've an idea! What do you think about checking file type after immediately creating basic file?

// create the basic file and a test upload object
$file = static::factory($props);

if ($file->type() === 'image') {
  $upload = new Image($props['source']);
} else {
  $upload = new BaseFile($props['source']);
}

https://github.com/getkirby/kirby/blob/develop/src/Cms/FileActions.php#L182-L184

distantnative commented 3 years ago

I think something in that direction. Reverting isn't a way with the refactored class structure. We need to use the right class when checking during upload.

bastianallgeier commented 3 years ago

Why was Image replaced at all? It already extends the File class and it doesn't really do any harm in my opinion to use the Image class for all files, or am I missing something?

bastianallgeier commented 3 years ago

Ah, I see. It's probably mainly because of the set of validations, right?

distantnative commented 3 years ago

Because we split Image into an actual File and Image class - before that any file was treated as image which was kinda weird.

bastianallgeier commented 3 years ago

The reason for that decision was mainly based on the fact that some files can be inspected by getimagesize (mostly videos) which is super weird, but would at least mean that you could apply those checks to videos as well.

distantnative commented 3 years ago

Should we then use Image for images and videos (in @afbora's check)?

bastianallgeier commented 3 years ago

Yes, I think that would be the most "compatible" way with 3.5.x. We could then maybe extend it later with new file type classes if there are additional useful checks and methods for videos or documents or whatever.

bastianallgeier commented 3 years ago