It would help DX (developer usability) to change token_replace() to support passing in langcodes in $options, rather than having to provide a full object. Unless we allow the simpler parameter we will continue to have modules do something like $options['language'] = (object) array('language' => $language);
which is just wrong. Let's standardize on functions like t() and accept a 'langcode' option instead.
Maybe this also needs wider input as a language parameter standardization by the D8MI team, so adding the core initiative tag.
Comment | File | Size | Author |
---|---|---|---|
#34 | tokens-langcode-option-1305378-34.patch | 29.31 KB | penyaskito |
#33 | tokens-langcode-option-1305378-33.patch | 29.31 KB | penyaskito |
#32 | tokens-langcode-option-1305378-32.patch | 28.77 KB | penyaskito |
#30 | tokens-langcode-option-1305378-30.patch | 29.31 KB | Gábor Hojtsy |
#23 | 16-19.txt | 561 bytes | Schnitzel |
Comments
Comment #1
Gábor HojtsyLangcode is more lightweight, I think we used 'language' at places where we already knew we had the language loaded and we did not want to do it again, but it backfired with inconsistency and worsened developer experience. I agree we should standardize on one method and langcode is better. Also, we don't have a a load function to load one language for a langcode, which makes it cumbersome to get that language object. We started to attempt to solve that at #1260510: Introduce a language_load($langcode) but probably shoot at too big a goal, maybe we can get improvements in once we scale our goal back there.
BTW langcode might not be the final name for language codes in Drupal 8, but we don't have anything better at the moment. Eg. if that langcode comes from a node language code then it would be 'langcode' => $node->language which looks odd, right? (all schema columns use language to signify a langcode, uh) This is being endlessly debated in #1216094: We call too many things 'language', clean that up and we did not seem to be close to reaching a conclusion on that.
Comment #2
Gábor HojtsyPoint #2 in this comment should help you understand the background and current D8 status even more: https://drupal.org/node/1216094#comment-5101648
Comment #3
sunYes, let's go with 'langcode' for now.
However, I do wonder though -- if we pass in a simple language identifier string instead of an object, then either individual token replacement callback functions need to convert that into language objects as needed (and also, individually, have to answer the question for how to deal with an invalid language identifier) - or - we make token_replace() do that string to object conversion on behalf of the caller, and only once for all token callback functions.
I'd prefer the latter, and would even dare to say that this might be backported.
Comment #4
Gábor Hojtsy@sun: ok I think that makes a lot of sense. Should be a good way to go. Also tagging for the base language system.
Comment #5
Gábor HojtsyAs well as the langcode spread tag :)
Comment #6
Dave ReidMost of the token replacement code I've encountered has only needed a langcode string. Very, very rarely have we needed a full language object, and we should have language_load() for that.
Comment #7
Gábor HojtsyDave! Would love to see if you could work on this. Thanks!
Comment #8
Gábor HojtsyComment #9
bforchhammer CreditAttribution: bforchhammer commentedWorking on this...
Comment #10
bforchhammer CreditAttribution: bforchhammer commentedHere's a first attempt.
Do we need tests for the langcode/language parameter synchronisation?
Comment #11
bforchhammer CreditAttribution: bforchhammer commentedDone for today..
Comment #12
Gábor HojtsyNeed to remove the language argument, not just make it optional by supporting langcode :)
Comment #13
bforchhammer CreditAttribution: bforchhammer commentedHm, like this? ;-) Let's see if this still passes tests...
Comment #14
k4v CreditAttribution: k4v commentedoops double post =)
Comment #15
Kristen PolThanks for the patch. The changes look good to me, but I'm wondering about the usage of both
$langcode
and$language_code
variables. Would it be good to standardize on one variable name for consistency throughout the codebase? I've called out the usage below. If it should be standardized,$language_code
is used more than$langcode
so it would be easiest to use that.Uses $langcode.
Uses $language_code.
Uses $language_code.
Uses $language_code.
Uses $language_code.
Uses $language_code.
Uses $langcode.
Comment #16
bforchhammer CreditAttribution: bforchhammer commentedUpdated patch fixing #15. I decided to go with
$langcode
, as that seems to be the standard variable name throughout the code-base ($language_code
was actually only used in test classes). This now includes one change which is not related to tokens in the TranslationTest class.Comment #17
Gábor Hojtsy$langcode should be good!
Comment #18
PrabhuG CreditAttribution: PrabhuG commentedThe document need to be updated in in "mail.inc" where $option['langcode'] to be update for langcode. (But currently it is not affecting any thing). Also, user_mail_tokens is also a callback function from user.module.
* $options['language'] = $message['language'];
* user_mail_tokens($variables, $data, $options);
Comment #19
bforchhammer CreditAttribution: bforchhammer commentedThat sounds like NW ;-)
Comment #20
Gábor HojtsyWell, that is an artifact of the hook_mail() semantics as far as I see, no? That should be a different issue, let's not overload this IMHO. Any other problems?
Comment #21
PrabhuG CreditAttribution: PrabhuG commentedFine by me.
Comment #22
Gábor HojtsyOk, so there indeed seems to be an issue here, since user_mail_tokens is not updated, BUT that does not use the langcode at all: http://api.drupal.org/api/drupal/core%21modules%21user%21user.module/fun...
So this seems like an unrelated bug here that the reset and cancel links are not generated for the email's or the user's language.
@webflo is opening an issue about it.
As for passing on the langcode here, since user_mail_tokens() would rely on the passed language object, we should pass the language object here for now IMHO, and then fix it later when/if hook_mail() and friends get converted to langcode instead of $language.
Comment #23
Schnitzel CreditAttribution: Schnitzel commentedadapted the documentation
Comment #24
Gábor HojtsySo the user mail tokens line is taken care of by #1754162: Implement language aware tokens for one time login link and cancel link and would be bugos to change here. It should be removed from this patch.
Comment #25
Schnitzel CreditAttribution: Schnitzel commented#23: tokens-langcode-option-1305378-19.patch queued for re-testing.
Comment #26
Gábor HojtsyReviewed this again with @webflo and @Schnitzel, and we figured out user_mail_tokens does not language currently either way and @webflo is fixing that in #1754162: Implement language aware tokens for one time login link and cancel link. So it is good either way for now, and the two patches can be independently worked on.
Comment #27
Gábor HojtsyAlso, this inspired me to open #1754208: Convert hook_mail() and hook_mail_alter() to langcode that should further improve consistency.
Comment #28
Gábor HojtsyThe testing issue was a glitch with the bot. So as said above, this should be good to go, and remaining language calls have other issues.
Comment #29
webchickShoot. Based on other stuff committed, this no longer applies. However, looks like a straight-forward DX improvement (and possibly performance improvement since we're passing around strings rather than objects) so I'm happy to commit once that's done.
Comment #30
Gábor HojtsyQuick reroll.
Comment #31
webchickRe-Bumping to RTBC since that's where it was before so I don't lose track of it.
Comment #32
penyaskitoRerolled by hand, so waiting for tests results and a second check would be useful.
Comment #33
penyaskitoForgot one chunk.
Comment #34
penyaskitoI broke code style on last attempt. Comparing patches, now looks good. The conflict was because a
true
was changed toTRUE
.Comment #36
penyaskito#1688144-19: tnid does not updated to zero after node delete maybe related?
Comment #37
webchick#34: tokens-langcode-option-1305378-34.patch queued for re-testing.
Comment #39
webchick#30: tokens-langcode-option-1305378-30.patch queued for re-testing.
Comment #40
webchickOk, committed and pushed #30 to 8.x, since the remainder were necessary (and failing) due to #1688144: tnid does not updated to zero after node delete, which was rolled back.
Thanks!!
Comment #41
Gábor HojtsyAdded a change notice for this at http://drupal.org/node/1776688 - I don't think a CHANGELOG.txt entry is needed. Removing off of sprint as a result.