backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

Image library ignores field extension (limit) settings #6590

Open indigoxela opened 3 months ago

indigoxela commented 3 months ago

Description of the bug

TBH, I'm not sure, whether this is a bug or works-as-designed. But it's a bit unexpected...

Limiting an image field to a specific extension, and also checking "Enable image browser", leads to the odd situation, that one can only upload images with that extension, but add arbitrary images via image browser.

Steps To Reproduce

  1. Go to admin/structure/types/manage/post/fields/field_image
  2. Restrict extensions to only one image extension
  3. Enable the image browser (if not on yet)
  4. Open a node form on /node/add/post
  5. See the extension restriction in the widget help text
  6. Open the image browser, add an image of a different type (previously uploaded)
  7. See the image added without nagging

Actual behavior

The field accepts any image type, actually anything from the browser.

image-field-widget

svg-accepted

Expected behavior

Not sure, but I wouldn't actually expect the field widget to be so permissive with image extensions.

Additional information

argiepiano commented 3 months ago

Thanks for posting this. One would assume that the image browser view would enact some sort filter to limit to specific file extensions.

You probably are 3 steps ahead of me, as usual, @indigoxela :smile: but just in case this may be helpful....

At quick glance, this perhaps may be the way to go:

  1. Add a contextual filter to the image browser view, showing all results if not available
  2. Edit file_managed_file_browser_open() and get the value for the file extensions from $element['#upload_validators']['file_validate_extensions'] if it exists. If not do not pass an argument to the view
  3. Map the extension to the MIME type (is there a function in core that does this?)
  4. Pass that/those extensions (if available) to the view in views_get_view()

EDIT: a potential problem with this approach is that contextual filters are passed as a string, with + as a separator (for OR). MIME types use + in some types (image/svg+xml). Not sure if the contextual filter processor actually handles escaped +.

klonos commented 3 months ago

I agree that this is not expected behavior, and that it should be considered a bug. It appears to even be a way to circumvent any restrictions set for the file field. The image library should be respecting any image type/extension restrictions that the field has, and I would like us to try to do that while the image is being selected - not throw any validation errors after a "wrong" or disallowed image was selected. In other words, try to prevent errors from happening in the first place, rather than telling people that they've done something wrong after the fact.

indigoxela commented 2 months ago

Good to know, that I'm not the only one finding this behavior odd. Thank you both for your quick feedback.

FTR obviously I never ran into trouble with this so far, only realized it when working on a contrib module, that does something similar.

And for possible options for fixing that in core, we could have a look there

Yes, a views argument, but not configurable via views admin UI, as this has to be dynamic based on field instance - something not (easily) available in views.

You probably are 3 steps ahead of me, as usual...

@argiepiano in which universe does that happen? :joy: Isn't it usually the other way round (you're ahead)?

indigoxela commented 2 months ago

A little more explanation for the contrib code, in case someone needs it. At first I also tried to work with regular contextual filters (arguments), but besides other technical difficulties, realized that this relies on GET, which - although there's no path for default views - seemed unreliable and not flexible enough to me. Hence the values are set server-side only. Might be a little overdone, but...

In the module I use a views query tag (set via UI) to mark specific views for alteration in hook_views_query_alter. I'm not 100% sure, if that's the only hook we can use. The order of views hook execution is a bit... tricky. But the pager works fine that way.

It could be, that things are a bit easier in core than in contrib, as we don't have to hook into - we "own" the code. Or maybe not.

If anyone has a smarter idea than setting a view query tag, please share it here.

klonos commented 2 months ago

@indigoxela what is the requirement? Is it "pass dynamic arguments to views, using GET"?

klonos commented 2 months ago
  1. Map the extension to the MIME type (is there a function in core that does this?)

Perhaps file_get_mimetype(), or file_get_mimetype_type(), or file_match_mimetypes() might be our friends here (most likely the first one I think).

indigoxela commented 2 months ago

what is the requirement?

The requirement is to somehow pass filter (query) options to the view dynamically, depending on the context, the dialog is opened from. That's not available in views out-of-the-box, as the view has no "knowledge" of the context it got embedded in.

Embedding happens in file_managed_file_browser_open() - the AJAX callback.

You can compare that to the contrib code in file_library which does it quite differently.

One of the tricky points is, that this view should be interchangeable, so we can't rely on only the view name to extend filters (for image fields the browser_view gets set in image_field_widget_info() - could be overridden in a hook_field_widget_info_alter). But the AJAX file_managed_file_browser_open callback is supposed to be generic - for all managed file items (defined via file_element_info). We can't get too specific (for image fields) in that callback.

The view has to get filtered by file extension, but the view needs to get "informed" of that dynamic filter. One possible solution is, to pass arguments to the embedded view. But we have to be careful with changes in the callback, because of its generic nature. My trick in contrib was to use the tempstore to save the filter options and pass the tempstore key as argument to the view, and in views_query_alter hook read that value and adapt the db query as needed (views is an additional db abstraction layer).

@klonos does that help to understand the difficulties we're facing?

klonos commented 2 months ago

Yes, thank you @indigoxela for taking the time to provide such a detailed description of the problem 🙏🏼 ...certain aspects of this are above my head, so I was trying to see if there's any "entry point" that I could help with.

indigoxela commented 2 months ago

certain aspects of this are above my head, so I was trying to see if there's any "entry point" that I could help with.

Never mind, the problem is quite complex. Not only for you. I'm struggling, too, to get the whole picture. Just got things working elsewhere by reducing complexity (by limiting to one widget type only, with a specific setting. And only act on views with a specific query tag...). Maybe we should reduce complexity here, too, just for the image library, with a custom callback.

And we're not alone, BTW. Looking at how the $title is set in file_managed_file_browser_open()... someone already misinterpreted the purpose of that AJAX callback to be for the image library specifically, which isn't the case.