There are several instances of the strings <none>
and <Hidden>
inside t() functions in Drupal core. Unsuspecting translators translate these to strings like <keine>
or <Versteckt>
. Translation strings which contain those translated strings cannot be imported by Drupal due to locale_string_is_safe() in locale.inc however. The result are misleading error messages during language import and lots of head scratching in IRC.
Attached is a patch that changes all occurences of t('<none>')
to t('(none)')
and the sole occurence of t('<Hidden>')
to t('Hidden')
(without brackets).
- I retained the brackets around 'none' (but changed them to rounded brackets) because it denotes the absence of any defined image styles or number formatters in /modules/image/image.module or /modules/field/modules/number/number.module.
- I removed the brackets around the term 'Hidden' because it logically is on the same level as 'Inline' or 'Above' when used to select the diplay options for field labels in /modules/field_ui/field_ui.admin.inc.
- I added a blank space to
<br/>
(resulting in the correct<br />
) in /modules/simpletest/tests/form.test and /modules/simpletest/tests/batch_test.callbacks.inc since a<br/>
in a translated string also results in an import error.
Cross-referencing #1020842: Error messages when importing translated strings fails are not helpful at all which provides for better error messages and logging of failed string imports.
Comment | File | Size | Author |
---|---|---|---|
#46 | untranslatable_strings-1317884-46.patch | 5.86 KB | smiletrl |
#46 | interdiff-34-46.txt | 3.71 KB | smiletrl |
#41 | untranslatable_strings-1317884-41.patch | 5.86 KB | YesCT |
#41 | interdiff-37-41.txt | 705 bytes | YesCT |
#37 | untranslatable_strings-1317884-37.patch | 5.87 KB | smiletrl |
Comments
Comment #1
reglogge CreditAttribution: reglogge commentedAttaching the patch.
Comment #2
yoroy CreditAttribution: yoroy commentedIs 'None' really so special that it needs brackets at all? Or is it mostly a little hack to get it to show on top when alpha-sorting?
Comment #3
xjmI think the punctuation around
(none)
is intended to denote that none is not actually just a string value like the other options. I could come up with a use case where I wanted to have a field with the literal value'None'
among the options, but also wanted the field to be optional.However, elsewhere, we use
- None -
. Let's use that instead of parens for consistency.And, of course, this needs to be rerolled.
Tagging as novice to switch over to hyphens around none (use whatever pattern you find elsewhere in core for that) and to reroll the patch on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.
Comment #4
kathyh CreditAttribution: kathyh commentedPer #3 updated to use - None - and #22336: Move all core Drupal files under a /core folder to improve usability and upgrades
Comment #5
xjmI thought about it for awhile and I can't think of a sane way to test this. The patch is clean, improves the consistency of our UI text, and helps resolve a localization issue.
Comment #6
chx CreditAttribution: chx commentedWhile I can think of some ways to test this , this another case where the effort to test simply doesn't worth it.
Comment #7
danillonunes CreditAttribution: danillonunes commentedIt's possible/practicable to test all core strings against whatever validation a translation string needs to pass?
Comment #8
xjmAs far as I know, we don't have the architecture to do so currently. That's what I meant by "sane" way to test it. :) However, it seems like something we should maybe have.
There's some issue with localization and simpletest and/or the testbots, but I don't know exactly what it is.
Maybe we should open a followup issue?
Comment #9
xjmI found another example of this: the taxonomy module has
<root>
as the option to set a term as the top-level parent. I assume this would have the same issue? Should we change all instances of that to- Root -
?Setting at NR to follow up on that and on the test question.
Comment #10
droplet CreditAttribution: droplet commentedComment #11
xjmAha! So #10 actually calls the approach of our patch into question here. Perhaps we should just be moving the "special" characters outside and concatenating them with the t().
I do still think that making
None
consistent is a good idea, and obviously we still need to patch the<br />
tags.Comment #12
kathyh CreditAttribution: kathyh commentedUpdates per #10 and #11
Comment #13
xjmAlso related: our use of hyphens for "indentation" to denote hierarchy in select boxes. See:
<front>
also cause translation problems? We shouldn't change that here because that has much larger implications; it's an actual token we use and not just a placeholder string in select boxes. If it does cause problems, that'd be a separate followup issue (though it seems surely that would have come up before)...Comment #14
reglogge CreditAttribution: reglogge commentedI did a quick check and
<front>
isn't used in any translatable strings in Drupal 8, so it shouldn't be a problem.Comment #15
Gábor HojtsyWould be great to have some consistency. The patch suggests we modify to -none- at places. Is this to be consistent with something else? Also, what kind of testing feedback are you looking for? I don't understand that question.
Comment #16
droplet CreditAttribution: droplet commentedI thought few times and have a question.
eg.
what's the different between these strings.
If we give a warning message and let admin to select import or not, would it be a problem?
comment #6 ~ #10 removed/hided at #1020842: Error messages when importing translated strings fails are not helpful at all, do you know anyone working on it?
Comment #17
Gábor Hojtsy@droplet: t('<unsafe>') is problmatic, because string are translated / assumed as HTML, and unsafe is not a valid HTML tag.
Comment #18
c960657 CreditAttribution: c960657 commentedI agree that we should eliminate
t('<none>')
, but not because they are rejected by the import system, but because the surrounding brackets are not part of the text itself. The former issue (whether we should allow important unsafe HTML) is really a separate issue - let's keep that discussion in #514926: translated strings shouldn't be validated.I support comment #15 about finding a standard approach so that we don't mix
t('<none>')
andt('- None -')
in core, preferably based on a theme function or similar.Comment #19
Gábor HojtsyRight, just let translators translate the text and how it is presented whether with brackets or hyphens is a different question.
Comment #20
RobLoach#12: untranslatable-strings-1317884-12.patch queued for re-testing.
Comment #22
izus CreditAttribution: izus commentedHi,
here is a new patch adapted to the last codebase.
Hopefully it went green !
Comment #23
Gábor HojtsyAll changes look good.
Comment #24
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #25
Gábor HojtsyThanks!
Comment #27
droplet CreditAttribution: droplet commentedahh. I've grepped git source and git log, can't find any changes ?
No changes:
http://drupalcode.org/project/drupal.git?a=search&h=HEAD&st=commit&s=131...
http://drupalcode.org/project/drupal.git/blob?f=core/modules/image/image...
But it may not safe to commit:
http://drupal.org/node/514926#comment-6497228
Comment #28
smiletrl CreditAttribution: smiletrl commentedThe reason that changes are invisible maybe the moving files.
Number module has moved to other place. And I didn't find such translation strings '', inside t() through UI 'admin/config/regional/translate/translate' filtering string ''.
So I think it's fine here.
Comment #30
xjmActually it looks like @droplet is correct; I can't find any commit for this issue.
git log | grep "1317884"
returns nothing. And:Comment #31
xjm#22: untranslatable-strings-1317884-22.patch queued for re-testing.
Comment #33
xjmComment #34
smiletrl CreditAttribution: smiletrl commentedThis is the rerolled patch.
Comment #35
smiletrl CreditAttribution: smiletrl commentedThere're still some other sort of strings needed to change.
After apply the patch
Comment #36
smiletrl CreditAttribution: smiletrl commentedTo keep consistency, I guess we need to change all these
'<none>'
to'- None -'
. Of course, some are for the import error concern, e.g.,core/modules/number/lib/Drupal/number/Plugin/field/formatter/DefaultNumberFormatter.php: '' => t('<none>'),
Comment #37
smiletrl CreditAttribution: smiletrl commentedI'm not sure is it appropriate to change all to '- None -' in #35. Anyway, a final patch attached.
interdiff-34-37.txt indicates all the changes according to #35.
Comment #39
YesCT CreditAttribution: YesCT commentedremoved needs reroll tag
Comment #40
Gábor HojtsyThis causes the notices due to the string being terminated :) '...'- None -'...' :)
Comment #41
YesCT CreditAttribution: YesCT commentedjust fixed that one line by removing the 's
Comment #42
smiletrl CreditAttribution: smiletrl commented@Gábor Hojtsy, ha, yeah, I missed that!
Wish #41 will pass~
Comment #43
RobLoachComment #44
catch#41: untranslatable_strings-1317884-41.patch queued for re-testing.
Comment #46
smiletrl CreditAttribution: smiletrl commented#41 is out of date.
Comment #47
smiletrl CreditAttribution: smiletrl commentedComment #48
Gábor HojtsyThanks for the reroll.
Comment #49
xjmGreat work @smiletrl!
Comment #50
webchickCommitted and pushed to 8.x. Thanks!
Comment #51
Gábor HojtsyWoot, thanks!
Comment #53
YesCT CreditAttribution: YesCT commentedsome instances were missed. See #421118-48: [Meta] Standardize capitalization on actions