Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Max length of the 'Allowed file extensions' field is too small - triggers validation error when too many extensions #4085

Closed
ghost opened this issue Sep 25, 2019 · 8 comments · Fixed by backdrop/backdrop#2899

Comments

@ghost
Copy link

ghost commented Sep 25, 2019

Description of the bug
The 'Allowed file extensions' field (a setting on the 'File' field type (not sure if its present elsewhere too)) has an issue where you can enter file extensions separated by a space or a comma, but when you save and reload the form, they're displayed with a space and a comma.

This causes issues with the max length of the field, which may be fine when you separate extensions with just a space, but when the form's reloaded and commas are added too, if you try to re-save the form it says you've exceeded the max length of the field.

Steps To Reproduce
To reproduce the behavior:

  1. Create a File field and edit it.
  2. Enter pdf doc docx dot dotx odt ott odm txt rtf csv xls xlsx xlt ods ots ppt pptx pps pot odp otp zip tar gz tgz rar bz2 mpg mpeg mov avi wmv flv mp3 wma m4a mp4 m4p aac wav ogg mid jpg jpeg png gif tif tiff ico bmp in the 'Allowed file extensions' field.
  3. Save.
  4. Re-edit the field.
  5. Note the new value of 'Allowed file extensions' as pdf, doc, docx, dot, dotx, odt, ott, odm, txt, rtf, csv, xls, xlsx, xlt, ods, ots, ppt, pptx, pps, pot, odp, otp, zip, tar, gz, tgz, rar, bz2, mpg, mpeg, mov, avi, wmv, flv, mp3, wma, m4a, mp4, m4p, aac, wav, ogg, mid, jpg, jpeg, png, gif, tif, tiff, ico, bmp.
  6. Re-save.

Actual behavior
The following error is displayed:

Allowed file extensions cannot be longer than 256 characters but is currently 259 characters long.

And the form isn't saved.

Expected behavior
The 'Allowed file extensions' field should have a value of what was originally submitted (e.g. pdf doc docx dot dotx odt ott odm txt rtf csv xls xlsx xlt ods ots ppt pptx pps pot odp otp zip tar gz tgz rar bz2 mpg mpeg mov avi wmv flv mp3 wma m4a mp4 m4p aac wav ogg mid jpg jpeg png gif tif tiff ico bmp) and therefore no error should be displayed and the form should save successfully.

Additional information
Backdrop CMS version: 1.13.3

@ghost
Copy link
Author

ghost commented Sep 25, 2019

Well that was easy: backdrop/backdrop#2899

@ghost
Copy link
Author

ghost commented Sep 25, 2019

The PR simply disables the re-writing of the default value. It apparently was meant to help readability (by adding additional commas between each extension), but since the field is saved with spaces separating them anyway, I think it's still readable.

The alternative would be to run the validation function (_file_generic_settings_extensions(), which converts all commas to spaces) before validating the #maxlength, however since #maxlength gets validated before custom validation functions, this would require a bit of re-writing and bypassing Backdrop's default methodology.

So I figure my PR is simplest and easiest.

@klonos
Copy link
Member

klonos commented Sep 29, 2019

Thanks @BWPanda 👍 ...I would have preferred the comma-separated list, if the character limit issue was not a problem with saving the form; but the bug is more serious than the "aesthetics".

Ideally, we can consider increasing the character limit of the field, but that can be a follow-up 😉

@quicksketch
Copy link
Member

I have a slight preference for leaving the comma in-place. It seems like the problem of running out of characters could easily happen just if you had enough extensions (whether there were commas or not). I think we should just increase the #maxlength property to to 512 (up from 256).

@klonos klonos changed the title Inconsistency with 'Allowed file extensions' field Increase the max length of the 'Allowed file extensions' field Oct 3, 2019
@klonos
Copy link
Member

klonos commented Oct 3, 2019

Ideally, we can consider increasing the character limit of the field, but that can be a follow-up 😉

Now that @quicksketch has seconded that, I feel more confident that this should be the way to go 🙂 ... @BWPanda, care to update the PR accordingly?

@klonos klonos changed the title Increase the max length of the 'Allowed file extensions' field Max length of the 'Allowed file extensions' field is too small - triggers validation error when too many extensions Oct 3, 2019
@ghost
Copy link
Author

ghost commented Oct 4, 2019

Sure, updated the PR.

@klonos
Copy link
Member

klonos commented Oct 4, 2019

Thanks @BWPanda ...back to RTBC 👍

@klonos klonos moved this from High priority to RTBC in Bugs Oct 6, 2019
Bugs automation moved this from RTBC to Fixed/Closed Oct 6, 2019
@quicksketch
Copy link
Member

Merged backdrop/backdrop#2899 into 1.x and 1.14.x. Thanks @BWPanda and @klonos!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Bugs
  
Fixed/Closed
Development

Successfully merging a pull request may close this issue.

2 participants