As per http://drupal.org/simpletest-tutorial-drupal7#t , I scanned the tests in the book module and removed t()s from assert messages, or changed them to format_string() where appropriate.

Patch follows.

Files: 
CommentFileSizeAuthor
#29 book-1741386-29.patch11.02 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 39,644 pass(es).
[ View ]
#22 1741386-21-t-book.patch1.85 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 41,984 pass(es).
[ View ]
#17 1741386-t-book-17.patch1.8 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 41,894 pass(es).
[ View ]
#14 remove-t-from-book-D7-1741386-14.patch10.98 KBlazysoundsystem
PASSED: [[SimpleTest]]: [MySQL] 39,403 pass(es).
[ View ]
#7 remove-t-from-Book-asserts-1741386-5.patch10.89 KBrasmusluckow
PASSED: [[SimpleTest]]: [MySQL] 40,566 pass(es).
[ View ]
#1 remove-t-from-Book-asserts-1741386-1.patch11.13 KBlazysoundsystem
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-t-from-Book-asserts-1741386-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new11.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-t-from-Book-asserts-1741386-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's the patch.

Status:Needs review» Needs work

The last submitted patch, remove-t-from-Book-asserts-1741386-1.patch, failed testing.

I'm working on re-rolling this patch right now.

Assigned:Unassigned» gremy

Assigned:gremy» Unassigned

Status:Needs work» Needs review
StatusFileSize
new10.89 KB
PASSED: [[SimpleTest]]: [MySQL] 40,566 pass(es).
[ View ]

I have done a re-roll.

I started this before reading through #500866: [META] remove t() from assert message, where they've been trying to solve this since 2009. Best to wait for confirmation there before putting any more work into this.

Status:Needs review» Reviewed & tested by the community
Issue tags:+needs backport to D7

Thanks @lazysoundsystem! We should follow the standard, even if core isn't fully compliant yet. So, we should continue to remove or replace t() where appropriate in assertion message texts. The more we clean up now, the less I have to review in that monster, generated patch in #500866: [META] remove t() from assert message. :)

All the cleanups in this patch are correct.

Status:Reviewed & tested by the community» Fixed
Issue tags:-needs backport to D7

Confirmed this doesn't conflict with any of the NR/RTBC "avoid commit conflicts" issues, so committed and pushed to 8.x. Thanks!

I'd rather not backport this to D7... I'm not really into the idea of changing the existing assertion pattern for tests (even if that pattern is, admittedly, "inconsistency") in a stable release, personally. It requires module developers to make changes in order to stay in compliance with a rule that wasn't formalized until after 8.x opened (and, arguably, still isn't formalized since we are rolling patches like this ;)).

Open to discussion if you feel strongly otherwise.

One reason to backport changes like this is that when there's less divergence in the codebase, it's easier to backport subsequent patches. E.g., with this patch applied to both D8 and D7, I could potentially add additional assertions around one of these and create the backport pretty easily with patch -p1 (supplying the old filename as the file to patch). When the context lines are different, I'd have to recreate my patch entirely by hand. We noticed this last winter when rolling docs cleanups--catch and I observed that cleaning up the docblocks in D8 but not D7 definitely interfered with backports.

Admittedly, this becomes less relevant as the codebases continue to diverge more radically, but there's also a lot of core that's still the same.

Also, I don't see how this patch in D7 would require any module developers to make any change. It's completely transparent.

Version:8.x-dev» 7.x-dev
Status:Fixed» Patch (to be ported)
Issue tags:+needs backport to D7

Hm. I guess the backport point is a decent one. Though patch -p1 doesn't work with tests anymore. :) Stupid PSR-0. :P~

Assigned:Unassigned» lazysoundsystem

Thanks @xjm, thanks @webchick - only the testbot will notice, but it's good to see this go in.

I'm busy this week, but after that I'll prepare this patch for backporting to D7.

There's also a few more of these for other modules already prepared referenced on #500866: [META] remove t() from assert message, and, if they're getting committed, I can make patches for the remaining modules too.

Status:Patch (to be ported)» Needs review
StatusFileSize
new10.98 KB
PASSED: [[SimpleTest]]: [MySQL] 39,403 pass(es).
[ View ]

And here's the backport for D7.

Assigned:lazysoundsystem» Unassigned

Reviewing D8 version, I see that there is one stray t() in bookTest.php that needs to be addressed. Not sure whether to roll a D8 patch now or wait until D7 patch is applied.

Version:7.x-dev» 8.x-dev
StatusFileSize
new1.8 KB
PASSED: [[SimpleTest]]: [MySQL] 41,894 pass(es).
[ View ]

Hey, if we are cleaning this - let's make it right!

Status:Needs review» Reviewed & tested by the community

Thanks @andypost. Looks good to me! RTBC!

Assigned:Unassigned» jhodgdon

Status:Reviewed & tested by the community» Needs work

Hm. I'm not sure about this change:

-      $this->pass(t('Node ' . $number . ' doesn\'t have outline.'));
+      $this->pass("Node $number doesn't have outline.");

Obviously t() was not right, and definitely that call to t() has all kinds of trouble!! but in other tests, we're using format_string() for this type of thing.

Assigned:jhodgdon» Unassigned

Assigned:Unassigned» jhodgdon
Status:Needs work» Needs review
StatusFileSize
new1.85 KB
PASSED: [[SimpleTest]]: [MySQL] 41,984 pass(es).
[ View ]

Here is a revised patch addressing #20.

Status:Needs review» Reviewed & tested by the community

That's better, thanks! I'll get this in once it turns green. :)

As I think back on the last week's work, this issue may come again in a few other patches. Unfortunately, I do not now recall which ones of the approximately 20 I rolled. Happy to re-roll if this type of issue pops up again.

Don't worry about it in advance. I review everything one final time before I commit patches, and hopefully either I or another reviewer will catch stuff like that.

@jhodgdon If you have a chance can you focus on #1797200: Remove t() from assertion messages in node.module tests.

I ask because that is just the first part of cleaning up the node module and there is more conversions to be done after that one is in. I think that is the only module/issue that needs more work at the moment.

I'm actually avoiding that one, sorry. Someone tagged a node.module issue as "avoid commit conflicts" and until they clarify what exactly is being patched, I'm trying to avoid any patches in that module for the moment.

Version:8.x-dev» 7.x-dev
Assigned:jhodgdon» Unassigned
Status:Reviewed & tested by the community» Patch (to be ported)

Meanwhile, this one has been committed to 8.x, so let's go back to 7.x porting.

Status:Patch (to be ported)» Needs review
StatusFileSize
new11.02 KB
PASSED: [[SimpleTest]]: [MySQL] 39,644 pass(es).
[ View ]

Rerolled the D7 backport in #14 with the changes in #22.

#29: book-1741386-29.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Hi,
#29 seems good for me :)
RTBCing

Status:Reviewed & tested by the community» Fixed

Automatically closed -- issue fixed for 2 weeks with no activity.