As per #500866: [META] remove t() from assert message, for test cases, the array returned from getInfo() should not have its values passed through t(). Patch fixes the few instance where these examples deviate from the standard.

(Note that although the cited issue is still "Needs Review", the actual change of removing t() was committed to core in comment #29 on July 13, 2009.)

CommentFileSizeAuthor
#1 1094308.patch2.38 KBtr

Comments

tr’s picture

tr commented
Status: Active » Needs review
StatusFileSize
new 1094308.patch2.38 KB

... and the patch:

ilo’s picture

ilo commented
Status: Needs review » Reviewed & tested by the community

Patch looks good to me. Other .test files should be reviewed, have you done it already, TR?

tr’s picture

tr commented

I checked every .test in the "Examples for Developers" repository. Plus all the code I maintain, of course :-)

Anonymous’s picture

Anonymous (not verified) commented
Status: Reviewed & tested by the community » Needs work

I'm a little confused and this might be because I didn't read through the entire first issue.

In comment #5 of that issue, boombatower says that this should be the new standard:

$this->assertTrue(TRUE, 'Text explaining the result');
...

In all of the patches from that issue that I've looked at, they remove the t from assert messages.

However, it seems that in this patch, only the instances of t() in getInfo were removed, not the ones in the assert messages. For instance, in trigger example:

$this->assertText($action_label, t('The triggersomething event executed the action.'));

Any reason why only the getInfo instances were removed and not the t() instances in assert messages?

ilo’s picture

ilo commented

Linclark, making such a topic change would require to change the issue title also.. because afaik, we were only talking about the getInfo() method, and from this point of view, the game is over with the patch.

After reading the whole issue I'm unsure we should remove all the t() calls from the tests, please, linkclark, can you develop the idea a little bit? considering the issue status and the rationale behind http://drupal.org/node/500866#comment-3295432, I think we must wait before applying it to all the t() use cases.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Title: getInfo() contents should not be passed through t() » Update tests to remove unnecessary t() per #500866

/me didn't know that #500866: [META] remove t() from assert message ever went in, or that that discussion was finished. But I fully support it and am glad that silliness is resolved.

By changing the title I'm suggesting that we do all the matching work here.

Wow, am I happy about this though.

If I understood correctly, the essence of that issue was that creating a string t(something) and then asserting t(something) is just a waste and a silliness, as there was no reason to translate it in any context. Might as well just create the string and assert the string.

Anonymous’s picture

Anonymous (not verified) commented
Title: Update tests to remove unnecessary t() per #500866 » getInfo() contents should not be passed through t()

Ok, this makes sense to me now based on the issue summary TR provided in comment #120 of the other issue. I wasn't sure what had happened in that other issue since the title doesn't reflect the commit that was made.

Changing back to RTBC.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Reviewed & tested by the community

Whoops, randy and I must have been commenting at the same time.... changing the title back to the one he set just a minute ago.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Title: getInfo() contents should not be passed through t() » Update tests to remove unnecessary t() per #500866

Fine to commit as is and then continue.

Anonymous’s picture

Anonymous (not verified) commented
Status: Reviewed & tested by the community » Postponed

OK, I've committed the changes from the patch in #1
http://drupalcode.org/project/examples.git/commit/fd12ba5

I'm changing the issue to postponed because we should probably watch #500866: [META] remove t() from assert message to see if it is eventually fully resolved. Anyone feel free to change the issue status if you disagree, though.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Version: » 7.x-1.x-dev
Status: Postponed » Closed (duplicate)

Marking as duplicate of #1803844: Remove t() from test assertion messages