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

Add a form API element for image that includes image type and size validators natively. #5946

Open
jenlampton opened this issue Jan 19, 2023 · 5 comments

Comments

@jenlampton
Copy link
Member

Description of the need

This came from reading several other issues about various problems with image uploads, and realizing that we do not have a dedicated form API element for images. We currently have only file and file_managed.

Images usually need different validation criteria than generic files, including

  • image type validation
  • image dimension validation

In most places in core, these are added manually to every file upload field used only for images. A new image form API element would make it easier for core developers change all image API elements at once. For example, adding a file type validation that used a site-wide set of allowed image types:

There are also other image-only upload locations in core, such as the site logo and favicon (and there may be others).

Proposed solution

Add a new image form api element, that is identical to file but with additional validators.

Alternatives that have been considered

Helper functions that could be used to determine a default set of allowed file types, based on the operating system and modules.

Draft of feature description for Press Release (1 paragraph at most)

Backdrop now includes a new form API element specifically for images, that will natively support all image types supported by your system.

@avpaderno
Copy link
Member

Should be there just an image form element, or also an image_managed field?

@klonos
Copy link
Member

klonos commented Jan 23, 2023

Should be there just an image form element, or also an image_managed field?

I'm actually questioning the actual need for 'file' and 'managed_file' TBH, since I find it confusing from a DX standpoint to have 2 different form elements for seemingly the same thing 🤔
image

I think that '#type' => 'managed_file', was introduced in order to handle the new, reusable managed media entities, while the "legacy" '#type' => 'file', was kept around for backwards compatibility and to handle files that should not be reused elsewhere in the CMS (like user profile images, uploaded module .zip files in the installer, or localization/translation files etc.).

Perhaps radical, but should we consider merging the 2 types, and then handle things via '#upload_location' (and do the same here with image files)? This is the only real difference between them, right? (I mean other then the "supplementary" properties like '#progress_indicator', '#progress_message', which could really just be omitted for the "plain" unmanaged files).

Similarly, instead of introducing new Form API elements for images here (because one could then argue that we need one for documents, and another for audio, and another for video), should we add a new property in the existing 'managed_file' and 'file' types? Something like '#file_type', which could be an array of any of the machine names of existing core or custom-created file types, and which by default would cause the file element to inherit any extensions configured for these types?

Just thinking out-loud re possible options.

@avpaderno
Copy link
Member

The reason for having an image form element is that the accepted image types are the ones recognized by the enabled image toolkit. One of the validation handlers needs to ask to the image toolkit if the uploaded image is one the toolkit can handle.

We do not have a sound toolkit to which pass a sound file; that is the reason for which we do not need a form element that is different from the file form element, for sounds.

@klonos
Copy link
Member

klonos commented Jan 23, 2023

Right @kiamlaluno 👍🏼

Where I was getting at is that if we had either '#type' => 'audio', (and/or '#type' => 'managed_audio', if we need to match what we currently have with file/managed_file) or better yet a new '#file_types' => array('audio'), property for the existing '#type' => 'file', then people wouldn't need to explicitly specify which extensions (or mime types) are allowed, nor add any custom validations - default ones could instead be pulled from the audio file type configured for the site (in admin/structure/file-types/manage/audio). That way, if someone would leave the defaults in their form element declaration, that element would respect what is configured for the site. So if a new audio type/extension is added to the audio file type by the site admin via the UI, then all audio form elements across the site would automatically pick that. If the site admin forbids an extension by removing it, then all audio form elements would automatically respect that. ...developers would still be able to override the defaults if needed, but for most cases they would just leave the defaults and there'd be less work for them to do. If a '#type' => 'file', specifies multiple file types ('#file_types' => array('audio', 'video'),) then all of these would be supported - if the element omits specifying ``'#file_types'`, then we allow everything unrestricted and there is no validation whatsoever.

I understand what you are saying with re to images, and the fact that they need special handling for their previews by whatever toolkit is available makes it more complex. But we can always allow the image to be uploaded as a "plain" file (link to download the file instead of trying to render it or its thumbnail), even if the toolkit doesn't support the specific type. Right?

And finally, I am not disagreeing with the proposal to add special handling for images in the FAPI - I'm considering different approaches than the one proposed here. So the suggestion is to basically add support for something like this:

  $form['image'] = array(
    '#title' => t('Upload an image'),
    '#type' => 'image',
    ...
  );

...and what I am saying is that we could do this instead:

  $form['image'] = array(
    '#title' => t('Upload an image'),
    '#type' => 'file',
    '#file_types' => array('image'),
    ...
  );

Why? Because if we add a new FAPI element for images, then we may need to add for other types in the future. If we add a way to be able to specify the type of the file we intent to handle (via a property in the existing file/managed_file elements), then I find it more convenient because we are not adding yet another similar element to the API, just a way to handle some special cases.

I hope this all makes sense, and please do note that this is just an idea - I have't considered everything.

@avpaderno
Copy link
Member

avpaderno commented Jan 24, 2023

But we can always allow the image to be uploaded as a "plain" file (link to download the file instead of trying to render it or its thumbnail), even if the toolkit doesn't support the specific type. Right?

If I upload an image for my user account, I expect it to be shown, not to get a link to download it, which is not helpful, since I have the image already in my computer and users visiting my user profile do not want to download a file, but see my "virtual face."
The same is true for a logo or a hero image.

I understand that we are exposing an idea, but we should not worry about audio files, which are not used from Backdrop core for any purpose, differently from images which are used from some core forms and shown in some places.

The actual issue is that users see contradictory error messages when they upload images. If the toolkit supports the image type, they could get an error about the used file extension; if they select a file with an accepted file extension (for example .doc), they could get an error about the image type (which is possibly not an image type, if the file has a .doc extension).
That happens because the form element used to upload an image is the file one, to which validation handlers are added. An image form element would not have a validation handler that checks for wrong file extensions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants