getkirby-v2 / panel

This is the deprecated admin panel for Kirby v2.
http://getkirby.com
Other
134 stars 70 forks source link

Field is not updated in non-default language if translate: false #781

Closed texnixe closed 8 years ago

texnixe commented 8 years ago

See this forum topic. Maybe it would be best if the value of non-translatable field was not stored at all but the value taken from the default language.

bungle commented 8 years ago

I think that correct way to do it is to store non-translatable only on default language. Please keep in mind that this should apply to structured fields as well. If structure is marked as non-translatable, it would only be stored on the default.

bastianallgeier commented 8 years ago

I totally agree. I just wonder what happens to content, which is already translated in the text files and marked as untranslatable in the panel. Should the panel then erase the already translated content?

texnixe commented 8 years ago

Hm, untranslatable IMO only makes sense if the non-default language can't have any other value no matter what is currently in the content file. The content file then was either edited outside of the panel or before the untranslatable option was added to the blueprint. I'm in favor of erasing translated content in such fields.

bastianallgeier commented 8 years ago

I tend to agree. But this is a typical case where I don't want to miss anything and introduce a rather destructive behaviour.

bungle commented 8 years ago

Can it be left as "non-destructive" the behavior only changes so that untranslatable fields are never read or written on alternate languages, but if there is already a translation, that is left alone, but not used until translatable is set back to true or the attribute is removed.

texnixe commented 8 years ago

That would tie Kirby to the Panel/blueprints, though, and Kirby would have to read each blueprint before outputting anything, which would not really make sense and slow things down? What could be good alternatives to either erasing the translated content or checking the blueprints?

bungle commented 8 years ago

Doesn't kirby need to read blueprints anyway to get default values in case when there is no value in actual content file? Or is this another issue? Or are the blueprints only for the panel?

Could blueprints be optionally cached on file, apc, memcached, redis. And optionally cache could only check statof the file, or stat could be disabled to only flush the cache on PHP process reload (either apache restart or php-fpm restart)?

bungle commented 8 years ago

And when cache is turned on for content, could Kirby store default values and no-translatable content values on cache (for all languages) so that it only needs to read one cache entry?

bungle commented 8 years ago

As a side note, I have to say that so many things in Kirby have been very well though out. It kinda feels so natural, and even the tiniest details seem to be just right. It's beautifully designed, cgrats!

texnixe commented 8 years ago

Or are the blueprints only for the panel?

Yes, the blueprints are only for the Panel, all settings only apply there. If you just enter anything into a text file manually, no validation is applied.

lukasbestle commented 8 years ago

I agree to erase translated data if the option is set. It's an option after all. :)

distantnative commented 8 years ago

I would agree to erase the data. Only other solution I can think of: If data exists in a translate: false field, show it as (in some form highlighted) read-only field with an X or something to remove the pre-existing translation. No way to edit or input new translations but keeping the data as long as the user does not hit the X to remove it. Clicking on the X will reveal the content from the default language. But as I said, I'm in favour of simply removing it.

lukasbestle commented 8 years ago

That's a great compromise. Only disadvantage I can think of: It would require every field to support the feature. So maybe just deleting the information is simpler. We need to document this for the translate option as a warning though.

rasteiner commented 8 years ago

What about just not saving the data in non default languages, instead of actively erasing them? Erasing them isn't necessary, because they would never exist if the panel wouldn't write them. This way we still have the possibility to "force" a translated content by other means (like editing manually the content file). I think the only problem here is that the panel writes the unwanted translations even if the fields aren't enabled, basically carving them in stone (the panel writes them once, and then you can't edit them anymore because the fields are disabled). If the panel wouldn't save the "disabled" fields, there wouldn't be any problem.

lukasbestle commented 8 years ago

The issue here only appears if you enable the option in the blueprint after creating a page. Once set, the Panel will do exactly that for new pages. Deleting is necessary for existing pages only.

rasteiner commented 8 years ago

Oh, my fault. Actually this mechanism doesn't seem to work for plugins (at least not the "color picker" and the "selector" plugins). They always save the data, even when they should not. This made me believe that it didn't work at all. But of course this would be another issue. (Probably an issue for the affected plugins, I guess).

sorry.

bungle commented 8 years ago

No, the problem appears on new pages as well. E.g.

  1. Create a new page where there is field: translate: false
  2. Save default language page
  3. Save alternate language page

Now it is stored on alternate language as well and cannot be edited. If on this new behavior it is never written on alternate language then it fixes the issue for new pages. That means though that you need to "open" those screwed up pages with old behavior manually if saving doesn't erase on new behavior.

rasteiner commented 8 years ago

I guess it depends on the field type, doing what you describe on a text type works correctly (the translate: false fields effectively aren't saved).

But other "types" (like most plugins and maybe even some core field types that don't extend the InputField base class) don't work correctly.

I think the problem is that the mechanism is based on the frontend not submitting the values instead of the backend not accepting them.

Basically the only thing that prevents the data from being saved is that we set a "disabled" attribute on the input element in the form. This way the browser (or the JS serializer - don't know) skips those fields when submitting, therefore they aren't saved. But if some plugin developers forget to check for the "disabled()" property the whole thing falls apart.

The irony here is that most of the time it's really those fields that don't work that we want to be translate: false, like the color picker or the file selector: you don't need to translate "that color" or "that product shot"...

texnixe commented 8 years ago

The default behavior of Kirby is to get the content of the default language if a field is empty in the non-default language. In this situation, not saving any content for the non-default languages if the field is set to translate: false makes sense.

However, if you filter your content by language ($page->content('en')->title()), the non-translated field would then have no output at all.

After all, maybe not storing any content for the non-default languages is not such a good idea after all, and updating all non-default languages after the default language has been updated would make more sense? Maybe this is an edge case, but it's certainly worth keeping in mind, I think.

bungle commented 8 years ago

If this is only a panel thing, then:

on default language save: 1) check if there is translate false and if yes, update all the language files with that value (not just the default)

on alternate language save: 1) check if field has translate false then check if default language has a value and use that for alternate language (update just the alternate language file)

And the problem is not with some plugins, it is a problem with all the kirby fields. And right now, makes translate: false useless.

distantnative commented 8 years ago

We just fixed this on the develop branch. Will be included in the next release.