Configuration of Import behaviour may be unclear / incorect [#1938824]

Problem/Motivation

As part of reviewing the schema file for the local.settings.yml I looked at the actual functionality and found it not clear and consistent.

This is how I see things (and it is possible that I am just not getting it)

yml configuration
The yml configuration uses two Boolean variables
overwrite_customized & overwrite_not_customized

Settings form
The settings form at admin/config/regional/translate/settings (locale.pages.in)
converts those two settings into three permutations
LOCALE_TRANSLATION_OVERWRITE_NONE => t("Don't overwrite existing translations."),
LOCALE_TRANSLATION_OVERWRITE_NON_CUSTOMIZED => t('Only overwrite imported translations, customized translations are kept.'),
LOCALE_TRANSLATION_OVERWRITE_ALL => t('Overwrite existing translations.'),

This looks incorrect to me see my comment for the issue Create configuration schemas for locale module

The following code

<?php
  if ($config->get('translation.overwrite_not_customized') == FALSE) {
    $default = LOCALE_TRANSLATION_OVERWRITE_NONE;
  }
  elseif ($config->get('translation.overwrite_customized') == TRUE) {
    $default = LOCALE_TRANSLATION_OVERWRITE_ALL;
  }
  else {
    $default = LOCALE_TRANSLATION_OVERWRITE_NON_CUSTOMIZED;
  }
?>

suggests that overwrite_customized will have no impact if overwrite_not_customized is set to FALSE.
My understanding is that:
overwrite_not_customized == FALSE & overwrite_customized == TRUE
Is the same as:
don't override translations that come from community source (d.o) and do overwrite customize translations.

Configuration execution code
When searching for the code that actually uses the yml configuration I came across a function _locale_translation_default_update_options() that converts them into an array 'overwrite_options' that end up eventually in Gettext.php after being called from locale_translate_batch_import()
This is where I extracted my understanding of the significance of this settings.

Proposed resolution

Review the settings form mapping of yml configuration logic

Disclaimer: I am new to most of this code and ran into this issue by accident so apologies if I am missing the point.

Comments

Comment #0.0

guy_schneerson CreditAttribution: guy_schneerson commented 10 March 2013 at 17:13

Issue summary:

View changes

Updated issue summary.

Comment #1

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 21 March 2013 at 09:36

Status:	Active	» Closed (works as designed)
Issue tags:	-Configuration system, -Configuration schema	+sprint

I don't think this has anything to do with config schema or system, so removed those. This is how the settings are stored (found via http://drupalcode.org/project/drupal.git/commitdiff/936cbb9144a3f1b0ac18...):

  switch ($values['overwrite']) {
    case LOCALE_TRANSLATION_OVERWRITE_ALL:
      $config
        ->set('translation.overwrite_customized', TRUE)
        ->set('translation.overwrite_not_customized', TRUE);
      break;
    case LOCALE_TRANSLATION_OVERWRITE_NON_CUSTOMIZED:
      $config
        ->set('translation.overwrite_customized', FALSE)
        ->set('translation.overwrite_not_customized', TRUE);
      break;
    case LOCALE_TRANSLATION_OVERWRITE_NONE:
      $config
        ->set('translation.overwrite_customized', FALSE)
        ->set('translation.overwrite_not_customized', FALSE);
      break;
  }

What happens here is that translation.overwrite_not_customized will only be false if NONE was picked and then translation.overwrite_customized separates between ALL and NON_CUSTOMIZED. So these are 3 layers on top of each other. It is either none, of only the non-customized is TRUE or both of them are TRUE.

This is indeed slightly different from the manual import form where you can pick if you want to import new stuff, import overwriting existing non-custom stuff or import overwriting existing custom stuff, and these three are checkboxes, so you can arbitrarily combine them. However for the automated update setting, the same UI is not used (there is even no option to opt out from new strings being added, that is assumed you want). I think there would be value in syncing the two user experiences (and therefore use of settings), but I see no bug here whatsoever.

Comment #2

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 21 March 2013 at 09:36

Issue tags:

-sprint

Don't know how sprint ended up there :)

Comment #3

guy_schneerson CreditAttribution: guy_schneerson commented 21 March 2013 at 19:12

@Gábor Hojtsy thanks for your explanation

I sill think that the stored configurations settings can have a case not supported by the UI but supported by the executing code:

  'translation.overwrite_customized' = TRUE 
  'translation.overwrite_not_customized' = FALSE

If this is set by code or imported into the site, overwrite_customized can be reset to FALSE if someone visits the configuration screen and saves without changing any settings.

If we are saying that all configuration settings should only be set using the provided configuration screens than this is not an issue.

Even if my understanding is correct, this is a vary minor issue and I am sure you got bigger concerns at the moment.

Comment #4

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 22 March 2013 at 08:19

Yeah I agree it is unfortunate the saved config can become inconsistent if manually edited, but so is anything else. You can manually update a string value to be a list and that will break the site too. People manually editing config files will need to be aware of how the values are used. I agree it would be better to store this value as one flag not too, but still then you could manually edit it to a value not recognised by the code and could have even worse effects. It is not really possible to solve the kind of problems that can arise from manual editing I think.

Comment #5

guy_schneerson CreditAttribution: guy_schneerson commented 22 March 2013 at 13:13

My understanding was that adding Schema files was not only for translation purpose but to also make configuration data more semantic, so if the cmi system doesn't already do this, it should eventually use the schema to validate all data i.e. loading and saving, this should protect data from being directly edited.

Comment #6

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 22 March 2013 at 13:17

That is not @heyrocker's position. The schema system was added as an optional add-on layer and is currently not planned to be required for any normal config operation. Core only starts to use it in #1905152: Integrate config schema with locale, so shipped configuration is translated and I don't know of other plans for core to use it. See the goals on the original issue at #1648930: Introduce configuration schema and use for translation.

Comment #7

guy_schneerson CreditAttribution: guy_schneerson commented 22 March 2013 at 14:10

Hi @Gábor Hojtsy and thanks for the info, sometimes when looking at core 8 Issues I fill like I got to a big party after it has finished. anyway I think I have a better picture now.

Setting the status to "closed and buried" :)