search

backdrop / backdrop-issues

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

Views strings not translatable when loaded from an export #4137

Open jenlampton opened 4 years ago

jenlampton commented 4 years ago

This issue is a spin-off from a views cross-port that likely won't apply to Backdrop, but we may want to discuss.

from @quicksketch in https://github.com/backdrop/backdrop-issues/issues/3720#issuecomment-541466138

I don't think we'll need/want a direct port of that code from Drupal 7.

From @quicksketch in https://github.com/backdrop/backdrop/pull/2654#issuecomment-541464146

I think we may need to think about this one wholistically. The concept of "exported" views doesn't exist in Backdrop; we don't have hook_views_default_views() like D7 did. Instead all views are always stored in config. So the idea of using a different translation system based on whether a view is exported or now doesn't make sense in Backdrop. That may also mean that the view::is_translatable() method needs updating in Backdorp.

PR by @klonos: https://github.com/backdrop/backdrop/pull/2654

indigoxela commented 3 years ago

The problem with translation of imported views seems (partly) solved since 1.16.3.

One remaining thing is that the strings in views (title, header text...) only get picked up when editing/updating the view at least once after import.

klonos commented 3 years ago

Comment by @quicksketch in the PR (from Oct 2019):

I think we may need to think about this one holistically. The concept of "exported" views doesn't exist in Backdrop; we don't have hook_views_default_views() like D7 did. Instead all views are always stored in config. So the idea of using a different translation system based on whether a view is exported or not doesn't make sense in Backdrop. That may also mean that the view::is_translatable() method needs updating in Backdrop.

klonos commented 3 years ago

Based on what @quicksketch said, and looking at the patch in https://www.drupal.org/files/issues/views-1248716-5-exported-view-translation.patch should we then ALWAYS assume that all views are localized/localizable?

Also, what should happen to the views_localize_all variable from Views 7.x? Should that be left out, or still converted to config? ...and if converted, then should we always set it to TRUE?

indigoxela commented 3 years ago

@klonos I'm not sure if I get your comments right...

should we then ALWAYS assume that all views are localized/localizable?

They are and should be (otherwise it would be a regression). See issue #4382 for details. Whether strings in views are translatable or not is based on:

  1. The locale module being enabled
  2. The advanced setting "Translation method" on admin/structure/views/settings/advanced (config views_localization_plugin)

Also, what should happen to the views_localize_all variable from Views 7.x

This variable doesn't exist in Backdrop and it would be useless anyway. I belief, the PR of this issue is outdated and should be closed.

The only thing that might need some work is - as I already mentioned - that strings from views currently don't get picked up automatically when views get imported. But I'd suggest to open a new issue for that. Sort of a follow-up for #4382.

klonos commented 3 months ago

I'd like to loop back to this. As I mentioned in #6511, there is no _config_translatables declaration in the various config files for views as it is currently, so things like Views names and descriptions aren't translatable.

... 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.)

klonos commented 3 months ago

This was discussed earlier today, during the dev meeting (as part of the discussion around other issues (like #6511 and #6512). It was 40:13 into the recording. Here: https://youtu.be/CTPd63wSpAg?t=2413

klonos commented 3 months ago

... the changes from 7.x might be "a workaround for a Drupal 7 era problem" ...

...so with #4382 we currently have this in Backdrop, under admin/structure/views/settings/advanced:

image

Whereas Views 7.x has this:

image

The checkbox and the respective functionality is what the backport is meant to bring across.

klonos commented 3 months ago

OK, so my understanding of the situation is this:

The above is fine. However, we need to investigate if the following applies to us when converting 7.x views, and whether that would affect things in any way after such views have been converted:

Views being loaded from an exported feature are always of type "default". As soon as they are being saved and stored to "views_view" and "views_display" they will have type "overridden". However, the Views module treats "default" views as not translatable in views/includes/view.inc, is_translatable(). The result is that after a fresh install of a feature with a view the admin string translations will not appear. Only after saving the view manually (override it) they will.

If that does not apply to us, then the only thing that we might need to consider is to do a update_variable_del('views_localize_all'); in an update hook for cleanup purposes ...but even that might not be required (?) ...FWIW, currently the sting views_localize_all does not exist in our codebase at all.

As for the follow-up issue at https://www.drupal.org/node/2564947, we also need to see if anything affects us:

When deploying Views with Features and translating those Views with i18nviews default string (next, field-label) are not translatable.

klonos commented 3 months ago

I've created #6608 specifically for the addition of _config_translatables to views config files. So if we decide that nothing in this issue here applied to us, we should then close the PR and this issue and be done with it.