Do not hard-code the "Select from Image Library" string in file_managed_file_browser_open()

[klonos]

Over in #2718, @argiepiano mentioned this forum post where he documents how to mimic the image browser introduced with the Image Library (see #3297) in order to implement a generic, custom file browser/picker (the specific post describes how to do that for pdf files, but can be adapted for any type of file). He mentions the following:

A minor annoyance: the dialog has the title "Select from Image Library". That title is, unfortunately, hard-coded in file_managed_file_browser_open(). Perhaps I'll create an issue for this. It'd be easy to pass the title within $form_state.

Lets fix that 🙂

[klonos]

There you go: https://github.com/backdrop/backdrop/pull/4732 ...just a couple of lines changed, and now the title in the modal gets the string from the view name. Rename the view, and you get the new name in the title:

[indigoxela]

One big disadvantage of this approach is, that the view name isn't translatable.

[argiepiano]

True, but the views title is. So, instead of doing $view_info->human_name, @klonos, you can do $view_info->get_title() within the t(). And I'd change the name of that variable, since $view_info is misleading. This is a full-fledged view object.

EDIT: actually, regardless of whether you use human_name or title, the t() in the patch will add the string to the translatable strings once Backdrop executes that line. This will allow the site owner to use the string translation UI to translate it afterward. Not ideal...

[klonos]

One big disadvantage of this approach is, that the view name isn't translatable.

That is only a problem if we see 'Image Library' as a feature in core that needs to be translated whenever it is being mentioned in the admin UI. If you consider the simplest use case, then the title on that modal should be saying just 'Select existing image'. The addition of the 'Image Library' bit was done because that is what we called the view that comes OOTB with core, and because that's what we thought appropriate in the original implementation of the feature. If we need to expand the functionality however, then that is very limiting. As it is now there is no other place available to select images from - it is always going to be the image library, so what is even the point of mentioning where the images are being selected from? So if the "this is not translatable" argument is a blocker, then I would suggest that we switch to plain 'Select existing image' instead, which covers both use cases:

• it doesn't force a hard-coded string for which people need additional code in order to override it when it doesn't make sense for their use case
• the entire string can be translated

Having said that, I would like us to add the specific view/display name to the modal title, because I see this issue here as a part of changes that I am suggesting in #2718 and #6512, where the view/display that will be used for the image and file browsers will be configurable. In these cases, including the name of the view/display being used in the title of the modal would be essential in order to provide context of the task being performed. Think "Select from Annual Reports" or "Select from Personal Photos" etc.

As an alternative compromise, we could add logic in the code that checks if the specific view/display that is provided by core is used, and if so then it leaves the entire string as Select from Image Library. If a different view/display is used, then it uses its title in a dynamic sting with a variable.

At this point though, I would like to point out that this touches on translatable configuration (see #704, #3455, #3522, #4382, #4894 etc.). Things like Views labels should be available for translation regardless. Right? ...in which case, could this be sorted by adding the "human_name" config in core/modules/filter/config/views.view.image_library.json to a _config_translatables. Right?

[herbdool]

@klonos you need to also call the config with $config->getTranslated('human_name') but Views makes that more complicated because it has its own object to represent views. Though I do see this crafty approach in the Views module (in views_plugin_display_block.inc):

$description = t('View: @view (@display)', array('@view' => t($this->view->get_human_name()), '@display' => t($this->display->display_title)));

So even though we're not supposed to translate a variable like t($text) Views is already doing it so we can probably do the same here.

[klonos]

@herbdool I also had the same concern another time about translating a variable that way, but I was told that it was OK. I cannot recall the exact PR where that has happened, but I was able for example to find the following other instances where we are doing that (not a compete list):

• 'title' => $t($title), here: https://github.com/backdrop/backdrop/blob/1.x/core/modules/menu/menu.install#L21
• t($dblog->type), here: https://github.com/backdrop/backdrop/blob/1.x/core/modules/dblog/dblog.admin.inc#L63
• $type_options[$machine] = check_plain(t($node_type->name)); here: https://github.com/backdrop/backdrop/blob/1.x/core/modules/dashboard/includes/block.node_types.inc#L138

I think that I was pointed to some documentation in d.o actually, which said that it was OK. It was some time ago, so I cannot recall exactly.

[klonos]

@herbdool looping back to this, I've updated the PR to be using config_get_translated() instead.

Having said that, I don't see any _config_translatables declaration in the various config files for views as it is currently though. Since config_get_translated() falls back to the non-translated string, that will do for now.

I would like to move this issue here forward since the simple, 2-line code change addresses the problem this issue was raised for. Allowing certain things like Views names and descriptions, as well as the same for each display etc. to be translated seems like a broader task and out of scope here. We'd need to consider what other things in each view need to be translatable too (such as the the header, footer, no-results text etc.). So lets leave that as a separate follow-up. Perhaps in #4137(?).

Anyone care to review/test this?

[klonos]

This was discussed during the dev meeting today, 33:22 into the recording. Here: https://youtu.be/CTPd63wSpAg?t=2002

• we also touched on #6512 (which @jenlampton suggested that we should do before I even bring it up 🙂)
• this change here is small, and we should get it in before we move on to the larger plans (#2718, #6512 etc.)
• we also touched on #4137, which we should also address.
• @quicksketch suggested that instead of config_get_translatable() we should be using the method on the view object that gets the translated label. However, the $view variable that is used in file_managed_file_browser_open() is using views_embed_view(), which returns markup - NOT the view object. I'll see what can be done with regards to that.

[indigoxela]

After a short while and a steep learning curve re file_managed_file_process() - which is the base for all managed file form items, not only image - my conclusion is: whatever we change the $title to isn't ideal, as long as it contains hardcoded strings. :wink:

However, maybe function file_managed_file_browser_open() tries to cover too many cases, anyway. Currently, as core doesn't use a view for anything besides image browser, and only provides one image browser view, it's OK to just use one function for all and everything.

As soon as we try to add something to file fields in core, we'd have to override the AJAX callback function, anyway, to overcome the limitations, I guess. At least, that's what I had to do in my contrib project file_library. :wink:

My personal conclusion: the current PR improves things for some cases. But the base problem's still pending.

Find some considerations re this render process handling in this otherwise not directly related issue. You have to scroll down a bit.