Problem/Motivation

Date formats like html_datetime and html_time are meant to be machine-readable therefore its unfortunate to translate them to different languages.

Proposed resolution

Apply the right non-translatable configuration schema types as appropriate based on locked status.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review
Issue tags: +Needs tests, +language-config
FileSize
713 bytes

Something like this should theoretically work if the boolean value is resolved properly to a number... Not sure if it does and of course needs tests.

Status: Needs review » Needs work

The last submitted patch, 1: 2533746.patch, failed testing.

Gábor Hojtsy’s picture

Well, the patch is silly because the label of the type should always be translatable. The format itself should not be, so the format should get this conditional typing not the label, but you get the idea :)

webflo’s picture

Status: Needs work » Needs review
FileSize
664 bytes
webflo’s picture

+++ b/core/config/schema/core.data_types.schema.yml
@@ -416,9 +416,17 @@ core.date_format.*:
+core_date_format_pattern.:

The property looks odd but it is correct because TypedConfigManager::replaceVariable casts the value to string which result in empty string for FALSE.

webflo’s picture

FileSize
2.85 KB
Gábor Hojtsy’s picture

I personally think it would be worth an API change to cast booleans first to an int and then string for this replacement. Otherwise this will end up in consecutive dots where a boolean is used in the middle of some dynamic type :/ I don't think this was considered before.

webflo’s picture

FileSize
3.54 KB
1.16 KB
Jose Reyero’s picture

I've tried the patch and it works, though it looks like pushing too much the possibilities of the config schema API. It is an interesting idea anyway, but I'm wondering whether we could make this less hackish and more "readable", since we are changing the API anyway, and make it more reusable...

core.date_format.*:
  type: config_entity
  label: 'Date format'
  mapping:
    id:
      type: string
      label: 'ID'
    label:
      type: label
      label: 'Label'
    locked:
      type: boolean
      label: 'Locked'
    pattern:
      type: core_date_format
      label: 'PHP date format'
      translatable: !%parent.locked

Note the translatable: !%parent.locked could be an API addition as simple as this one, but more readable / reusable.

About the approach itself I think there's something missing in the whole thing, not only the translation but the idea of 'locked' configuration which I find puzzling. It certainly looks weird that this configuration cannot be edited but it can be translated.

Could we think better of a generic approach for this "locked" configuration, maybe handling it at the controller level and entirely removing the "Translate" option? Can we consider "locked" a reserved word for the configuration that would trigger this behavior?

If we consider this configuration some hardcoded piece of data maybe the label should be translatable only in the locale UI (User interface translation) -and it is translatable there btw, maybe we don't need it to be translatable here too-.

Btw, the date format is still translatable using the locale UI, which may be some inconsistency, see screenshot. Though maybe reinstalling with this patch it wouldn't make it to locales_source table, I don't know

The idea could be something like:
- Locked configuration (configuration with 'locked:1') doesn't get a "Translate" option, just the same as not getting a "Edit" option.
- Labels (still translatable for locked configuration) are translated using the Interface translation only.

So not sure about this, it may be too complex, but in any case I think we should add some specific handling into the config translation system for 'locked configuration'. Otherwise we'll be rewriting config types again for each future case.

Gábor Hojtsy’s picture

I am not sure this is pushing config schema too far, it was supposed to help us define dynamic types based on data properties, so there! I think trying to introduce dynamic features in the schema property value level would set the wrong expectations. Eg. why not the ability to set dynamic schema labels then? With the proposed patch I am not sure how locale is exposed to those strings? I guess due to the .po file downloaded in install? That should loose these entries once this change goes through to localize.drupal.org. (Although them being empty does not support that assumption, because l.d.o sources strings should not be empty). Hm.

Jose Reyero’s picture

@Gábor,
We can say dynamic types do work then, that is fine :-) But really, when we are defining two data types named 'core_date_format_pattern.0' and 'core_date_format_pattern.1' I tend to think this is the kind of solution that looks nice in a contrib module, not that much in Drupal core.

No real objections about the patch though so if you think other options are too complex/long at this point, then ok, let's move on with this one. I just thought if we are going to introduce some api change, let's do some more powerful one..

About the locale issue, yes, I think once this is fixed, those strings will just vanish by themselves...

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +sprint

I don't think its worth to further complicate the schema API while it already has the flexibility we need. Let's get this in then :)

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.37 KB

I am also not a fan of using the config schema like this. We have worked hard to achieve type safety in config land and now to be able to perform this workaround we need to go back to using 0 and 1 instead of true and false and actually have them mean something entirely different as well (locked/unlocked), as they are now used not as values but as part of the key/name.

I think we can solve this without adding any complexity to the config schema with the alternate patch. It has the downside of hardcoding the list of locked date formats, meaning that any module providing another locked date format would have to declare it in the schema and a module allowing to create locked date formats from the UI would have to implement hook_config_schema_alter().

What do you folks think about this?

Gábor Hojtsy’s picture

+++ b/core/config/schema/core.data_types.schema.yml
@@ -424,6 +427,27 @@ core.date_format.*:
+date_format_entity_locked:
+  type: date_format_entity
+  mapping:
+    pattern:
+      type: string
+
+core.date_format.html_date:
+  type: date_format_entity_locked
+core.date_format.html_datetime:
+  type: date_format_entity_locked
+core.date_format.html_month:
+  type: date_format_entity_locked
+core.date_format.html_time:
+  type: date_format_entity_locked
+core.date_format.html_week:
+  type: date_format_entity_locked
+core.date_format.html_year:
+  type: date_format_entity_locked
+core.date_format.html_yearless_date:
+  type: date_format_entity_locked

I don't think this looks nicer personally and is also less extensible as you explained. Keeping two lists of the same things in check is not a very good idea in general.

If we are to do this, we'd need to at least update the tests to test that ALL locked date formats were untranslatable, not just one sample, so that future regressions are averted.

Generally not too fond of manually listing them like this.

As for data typing, we could make bool true resolve to 'true' in dynamic types and false to 'false' if that sounds better? It is very probable that people in contrib will run into making dynamic types based on bools for whatever other reasons IMHO.

Jose Reyero’s picture

I think we are looking in the wrong place. All of our schema hacks for this are going to be ugly. And that's because the 'locked configuration' feature is poorly implemented.

Maybe we shouldn't look at dates, but at 'locked configuration'. We do need some way of handling locked configuration and that is not nice at the ConfigEntity level either.

Just look at two examples:

- Locked Dates.... Listed on admin pages, Not editable, Partially translatable (?)

- Locked Languages... Not even listed on admin pages, Not editable, Not translatable (?)

I am sure there are some other examples of locked configuration around..(?)

What is really ugly and would need some fixing is how the configuration system manages locked configuration: Not in an uniform way, handled in a case by case basis, Poor on the usability side - why should users see some locked configuration (Dates), though not being able to edit them, but they don't see some other (Languages)....

What we do need is some 'LockedConfigInterface' and checking for translation access just the same way we check for edit access...

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
3.54 KB

Reuploading #8, so I can RTBC again.

Jose Reyero’s picture

Yes, let's apply this one, it seems to be the best option we have atm.

If any of the other API changes lands eventually, we may look for other solutions.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

I think we should not add this strange schema core_date_format_pattern.0 and core_date_format_pattern.1 without some documentation in core.data_types.schema.yml. Maybe something like

We want to set the translatability of the date format pattern depending on whether the date format is locked. Configuration schema, however, does not allow setting properties dynamically based on (the negation of) the value of other properties. Therefore we define two fake date format pattern types (one translatable, one not) making use of the fact that TRUE and FALSE get converted to their integer representations when used as a data type name.

Marking needs work for that.

I also thought about avoiding this some more and came up with a different solution: We could add a translationAccess() (or similar) method to ConfigMapperInterface and then we could add a specialized date format mapper that checks for the locked property. Then we could call that method when providing the translate operation and when checking route access for the translation overview page. If we do ever get generic "locked" handling for config entities, we could generically check for that in ConfigEntityMapper but either way I think that would be a much less convoluted solution than the current. Thoughts?

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

@tstoeckler: translatability is supposed to be parseable from the schema only, as it will be on localize.drupal.org. This needs to be defined in the schema. We could refine how we express this in the schema but doing it in the runtime will lead people to translate things on localize that are not used in Drupal at the end. (Apart from needing a lot more PHP code to cover a simple case).

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/config/schema/core.data_types.schema.yml
@@ -421,9 +421,17 @@ core.date_format.*:
 
+core_date_format_pattern.0:
+  type: date_format
+  label: 'Date format'
+
+core_date_format_pattern.1:
+  type: string
+  label: 'Date format'

Let's add a comments about both these schemas to explain what's going on.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
637 bytes
3.66 KB

There! I re-RTBC-ed because its such a minor update.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8f028e6 and pushed to 8.0.x. Thanks!

  • alexpott committed 8f028e6 on 8.0.x
    Issue #2533746 by Gábor Hojtsy, webflo, tstoeckler, Jose Reyero: Remove...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.