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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lazysoundsystem’s picture

Status: Active » Needs review
FileSize
11.13 KB

Here's the patch.

rasmusluckow’s picture

Status: Needs review » Needs work

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

rasmusluckow’s picture

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

gremy’s picture

Assigned: Unassigned » gremy
gremy’s picture

Assigned: gremy » Unassigned
rasmusluckow’s picture

Status: Needs work » Needs review
FileSize
10.89 KB

I have done a re-roll.

lazysoundsystem’s picture

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.

xjm’s picture

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.

webchick’s picture

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.

xjm’s picture

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.

webchick’s picture

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~

lazysoundsystem’s picture

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.

lazysoundsystem’s picture

Status: Patch (to be ported) » Needs review
FileSize
10.98 KB

And here's the backport for D7.

lazysoundsystem’s picture

Assigned: lazysoundsystem » Unassigned
Lars Toomre’s picture

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.

andypost’s picture

Version: 7.x-dev » 8.x-dev
FileSize
1.8 KB

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

Lars Toomre’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @andypost. Looks good to me! RTBC!

webchick’s picture

Assigned: Unassigned » jhodgdon
jhodgdon’s picture

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.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Lars Toomre’s picture

Assigned: Unassigned » jhodgdon
Status: Needs work » Needs review
FileSize
1.85 KB

Here is a revised patch addressing #20.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

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

Lars Toomre’s picture

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.

jhodgdon’s picture

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.

Lars Toomre’s picture

@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.

jhodgdon’s picture

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.

jhodgdon’s picture

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.

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
11.02 KB

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

izus’s picture

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

izus’s picture

Status: Needs review » Reviewed & tested by the community

Hi,
#29 seems good for me :)
RTBCing

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

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