Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#22 | 2533746-22.patch | 3.66 KB | Gábor Hojtsy |
#22 | interdiff.txt | 637 bytes | Gábor Hojtsy |
#17 | 2533746-8.patch | 3.54 KB | Gábor Hojtsy |
#13 | 2533746-alternate.patch | 3.37 KB | tstoeckler |
#9 | 2533746-locked_date_formats-interface_translation.png | 43.13 KB | Jose Reyero |
Comments
Comment #1
Gábor HojtsySomething 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.
Comment #3
Gábor HojtsyWell, 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 :)
Comment #4
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #5
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedThe property looks odd but it is correct because TypedConfigManager::replaceVariable casts the value to string which result in empty string for FALSE.
Comment #6
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #7
Gábor HojtsyI 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.
Comment #8
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #9
Jose Reyero CreditAttribution: Jose Reyero as a volunteer commentedI'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...
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.
Comment #10
Gábor HojtsyI 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.
Comment #11
Jose Reyero CreditAttribution: Jose Reyero as a volunteer commented@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...
Comment #12
Gábor HojtsyI 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 :)
Comment #13
tstoecklerI 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?
Comment #14
Gábor HojtsyI 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.
Comment #15
Jose Reyero CreditAttribution: Jose Reyero as a volunteer commentedI 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...
Comment #16
Gábor HojtsySee #1831928: Support a 'locked' property on ConfigEntities, #2289551: Clarify what 'locked' means for a config entity and whether it's okay for code to rely on a locked config entity existing, #2084567: Implement a EntityLockedInterface and service to allow locking entities for some unification efforts.
Comment #17
Gábor HojtsyReuploading #8, so I can RTBC again.
Comment #18
Jose Reyero CreditAttribution: Jose Reyero as a volunteer commentedYes, 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.
Comment #19
tstoecklerI think we should not add this strange schema
core_date_format_pattern.0
andcore_date_format_pattern.1
without some documentation incore.data_types.schema.yml
. Maybe something likeMarking 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 toConfigMapperInterface
and then we could add a specialized date format mapper that checks for the locked property. Then we could call that method when providing thetranslate
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 inConfigEntityMapper
but either way I think that would be a much less convoluted solution than the current. Thoughts?Comment #20
Gábor Hojtsy@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).
Comment #21
alexpottLet's add a comments about both these schemas to explain what's going on.
Comment #22
Gábor HojtsyThere! I re-RTBC-ed because its such a minor update.
Comment #23
alexpottCommitted 8f028e6 and pushed to 8.0.x. Thanks!
Comment #25
Gábor HojtsyYay, thanks!