This was suggested as part of the #361597: CRUD API for locale source and locale target strings issue and also by various people in the discussion at http://groups.drupal.org/node/154394. Basically, abstracting out the formatting code from t() allows coders who need to write single language modules for clients (for sites that will never use English) to use this same functionality in their strings, without going through the whole loop of the translation system.
If abstracting this out of t() is deemed a performance problem, we can still copy it in t() and have it as its own function as well. It is currently implemented as the same code in t(), st() and Drupal.t(). This patch touches t() and Drupal.t(). st() is not changed because even though st() does include theme.inc, it does not actually use any theming to render the placeholder, which is due to the possibly missing theme system I guess. Sounds like it is best left as-is for now.
Comment | File | Size | Author |
---|---|---|---|
#24 | make-formatting-available-with-tests.patch | 8.69 KB | Gábor Hojtsy |
#22 | make-formatting-available-with-tests.patch | 8.7 KB | Gábor Hojtsy |
#14 | make-formatting-available_0.patch | 6.33 KB | Gábor Hojtsy |
#7 | make-formatting-available.patch | 6.36 KB | Gábor Hojtsy |
make-formatting-available.patch | 6.35 KB | Gábor Hojtsy | |
Comments
Comment #1
KarenS CreditAttribution: KarenS commentedI think this is an excellent idea. This is useful functionality that shouldn't be limited to or dependent on the translation system.
Comment #2
pwolanin CreditAttribution: pwolanin commentedSo I often use t() even on custom things since the substitution is helpful. Do we really want to encourage people to avoid t()?
What is the problem being solved? If no localization is happening is there any performance penalty for using t() simply for replacement?
Comment #3
Gábor HojtsyPeople discuss this at the linked thread at http://groups.drupal.org/node/154394. There are modules built for markets where English is not among the supported languages. They do not have resources to make up English original strings. If they use t(), their strings will end up in the database for translation assumed to be English. This is both counter-productive for performance and goes against logic. We can either support them to at least use secure practices with this function or let them make up their own custom stuff each time. This is not about discouraging use of t() and I don't think Drupal core or drupal.org contributed modules will use this function much if ever. This is about supporting some uses cases from http://groups.drupal.org/node/154394 without going the whole way of externalizing strings that many people (including you) did not like.
Comment #4
pwolanin CreditAttribution: pwolanin commentedOk, so the use case is:
1) Site is mono-lingual using localization features already (non-English).
2) Custom code needs to do secure token substitution on non-English UI strings.
That makes more sense, thanks.
Comment #5
wojtha CreditAttribution: wojtha commentedCool, you wrote the patch first, Gábor :-) ... exactly what I need ... +1.
Is it worth for contrib to use the drupal_format_string in D6 or D7? ... since there is probably no chance to get it in D7 now, right?
Comment #6
plachIf I understood webchick right, there is a remote possibility for this to be backported to D7. Anyway:
This should be:
otherwise we get no string replacement. Tests do not cover this, that's why the bot is green.
Powered by Dreditor.
Comment #7
Gábor HojtsyRight, patch updated. Thanks!
Comment #8
wojtha CreditAttribution: wojtha commentedI think we need to cover this with test and I think we need extend the tests too ... IMO there should be unit test for the drupal_replace_string and functional for Drupal.replaceString to not break the string replacement in the future again. What do you think?
Comment #9
Gábor HojtsyThere is no support for running tests on Javascript code at the moment in the Drupal QA system. We could (probably should) expand tests to the drupal_replace_string() function. We can probably just copy/reuse some tests from t().
Comment #10
sunComment #11
Gábor Hojtsy1. Well, I just picked this up as it was before on the linked issue. I think drupal_string() can be good as well, it is still far away from looking like "in the family" of t(). Not sure if we need that to happen.
2. Yes, I think it would be great to get this one in and continue the refactoring on higher levels. I assume your discussion will need to take its paces and we can ensure this is in in the meantime :)
Comment #12
sunOn a second thought, let's use a name that's consistent with other core formatting functions:
format_string()
With that name, I'd agree we can move on independently here.
Comment #13
Gábor HojtsyWell, I was thinking about this too, and think "format" is a poor name for these functions, but we can definitely use that and get renamed later in an API cleanup :)
Comment #14
Gábor HojtsyThere you go format_string() and formatString().
Comment #15
sunComment #16
plachLet's see if this can be backported to D7.
Comment #17
sunSorry to be a pain:
Missing phpDoc suffix:
18 days to next Drupal core point release.
Comment #18
Gábor HojtsyAh, do you mean we remove the existing See format_string() and see Drupal.formatString() and convert them to @see as well at the end of the phpdoc as well? (Trying to avoid playing ping-pong for quite a while which can easily happen).
Comment #19
sunSorry if #17 was too sparse. Let me clarify:
I don't see an "existing" @see here...?
As this is a sanitization function, api.d.o should output it in the corresponding group.
And since people might mistakenly use format_string() instead of t() without knowing about t(), format_string() should at least have a final @see pointer to t().
18 days to next Drupal core point release.
Comment #20
Gábor HojtsyI meant the See's which are not @see's should be converted, and moved down to the end of phpdoc no? I'm not clear what should be done with inline "see information" notes that we have on t() and Drupal.t() now for the replacement formats.
Comment #21
sunNope, the others are fine, as they are in the context/description of a @param. The @param itself does not duplicate the description of format_string() [which makes sense], so we need to point the reader to the docs that describe the @param within its description; not in a completely off-context location. A @directive cannot contain sub-@directives; i.e, @see cannot be used within @param.
Comment #22
Gábor HojtsyAll righto. Here is a new patch with your suggested additions. Other changes:
- I've also added the @see and @ingroup in JS, although JS does not seem to be parsed for api.drupal.org it is the same code there
- I looked for but could not find any tests for t()'s sanitization/replacement behavior. Yeah. So I wrote my own, and put format_string() to the test while we were there.
- While running the tests some bugs were found. Namely the $args on format_string() was not optional before and it was not typed to an array.
The relevant tests now pass on my machine. I think this covers the test requirement for this, expands testing on t() and otherwise includes all suggestions from above.
The only thing I'm not sure about in this patch is the em placeholder test equivalent check. Looks like hardcoding a specific Drupalism there, but without calling out to the theme system and therefore making the test almost use the same code as the one tested I don't think we can do much better.
Let's get this in, can we?!
Comment #23
sunThe @see of the PHP format_string() should point to t(), not to the JavaScript Drupal.t().
15 days to next Drupal core point release.
Comment #24
Gábor HojtsyHaha, good catch. Fixed that. Thanks.
Comment #25
sunThanks!
Comment #26
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks. Moving to 7.x-dev as this could be a candidate for backporting.
Comment #27
webchickInteresting patch. I'm fine with backporting this to D7; looks to be a harmless API addition and we're finally below our bug count thresholds.
If I can be allowed to be a bit pedantic for a minute though, the function name "format_string()" doesn't remotely give any indication at all that this is what that function will do. It's also non-standard when you view it in terms the rest of the format_* functions, whose names are very descriptive:
Can we do format_placeholders() or format_placeholder_string() instead, or similar? Because unlike what Gábor says in #13, we can't actually change this function name willy-nilly in D7 once we introduce it.
Comment #28
sunIMO, format_string() is perfectly ok and sufficiently describes what it is for.
Most of our format_*() functions are actually following PHP's native formatting functions (e.g., number_format(), date_format()). Whereas there is no comparable equivalent for format_string() in PHP -- the closest functions are strtr(), str_replace(), and sprintf() - and out of those, only the latter partially sanitizes the input (but still not to the extent of format_string()).
format_placeholders() would be wrong, because it's not formatting placeholders, but a string. format_tokens() would clash with token API, and would also have aforementioned issue.
format_placeholder_string() and alike is an absolute no-go. We already did a huge mistake by introducing overly verbose function names with drupal_get_nested_array_value() - or whatever it is called - and its siblings - the function names are so long and verbose that it's kinda impossible to remember them (unless you use them daily). Likewise, you put one of these into your code and the line immediately exceeds 80 chars (decreasing readability big time). This totally defeats the purpose of general utility functions.
The only real alternatives I can see would be
s()
andf()
- but I'm fairly sure we don't want to go down that rabbit hole.Comment #29
Gábor HojtsyNo other feedback, so moving back to D7.
Comment #30
webchickYeah, ok. :\
Committed and pushed to 7.x. This could probably use a change notice.
Comment #31
Gábor HojtsyWe previously used the change notices to mark changed stuff not cool new stuff. That was the thinking behind not submitting one yet. Does this new system cover cool new things too?
Comment #32
webchickNow that it's not a complete pain in the ass to make these, I think they should, yeah. It'd certainly be useful for D7 at any rate, because it's a new capability that contrib modules have available that they didn't in 7.8 and below.
Comment #33
Gábor HojtsyDone: http://drupal.org/node/1312890
Comment #35
Gábor HojtsyTagging for the UI translation leg of D8MI.