Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#29 | book-1741386-29.patch | 11.02 KB | dcam |
#22 | 1741386-21-t-book.patch | 1.85 KB | Lars Toomre |
#17 | 1741386-t-book-17.patch | 1.8 KB | andypost |
#14 | remove-t-from-book-D7-1741386-14.patch | 10.98 KB | lazysoundsystem |
#7 | remove-t-from-Book-asserts-1741386-5.patch | 10.89 KB | rasmusluckow |
Comments
Comment #1
lazysoundsystem CreditAttribution: lazysoundsystem commentedHere's the patch.
Comment #2
rasmusluckow CreditAttribution: rasmusluckow commented#1: remove-t-from-Book-asserts-1741386-1.patch queued for re-testing.
Comment #4
rasmusluckow CreditAttribution: rasmusluckow commentedI'm working on re-rolling this patch right now.
Comment #5
gremy CreditAttribution: gremy commentedComment #6
gremy CreditAttribution: gremy commentedComment #7
rasmusluckow CreditAttribution: rasmusluckow commentedI have done a re-roll.
Comment #8
lazysoundsystem CreditAttribution: lazysoundsystem commentedI 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.
Comment #9
xjmThanks @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.
Comment #10
webchickConfirmed 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.
Comment #11
xjmOne 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.
Comment #12
webchickHm. I guess the backport point is a decent one. Though patch -p1 doesn't work with tests anymore. :) Stupid PSR-0. :P~
Comment #13
lazysoundsystem CreditAttribution: lazysoundsystem commentedThanks @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.
Comment #14
lazysoundsystem CreditAttribution: lazysoundsystem commentedAnd here's the backport for D7.
Comment #15
lazysoundsystem CreditAttribution: lazysoundsystem commentedComment #16
Lars Toomre CreditAttribution: Lars Toomre commentedReviewing 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.
Comment #17
andypostHey, if we are cleaning this - let's make it right!
Comment #18
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @andypost. Looks good to me! RTBC!
Comment #19
webchickComment #20
jhodgdonHm. I'm not sure about this change:
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.
Comment #21
jhodgdonComment #22
Lars Toomre CreditAttribution: Lars Toomre commentedHere is a revised patch addressing #20.
Comment #23
jhodgdonThat's better, thanks! I'll get this in once it turns green. :)
Comment #24
Lars Toomre CreditAttribution: Lars Toomre commentedAs 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.
Comment #25
jhodgdonDon'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.
Comment #26
Lars Toomre CreditAttribution: Lars Toomre commented@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.
Comment #27
jhodgdonI'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.
Comment #28
jhodgdonMeanwhile, this one has been committed to 8.x, so let's go back to 7.x porting.
Comment #29
dcam CreditAttribution: dcam commentedRerolled the D7 backport in #14 with the changes in #22.
Comment #30
izus CreditAttribution: izus commented#29: book-1741386-29.patch queued for re-testing.
Comment #31
izus CreditAttribution: izus commentedHi,
#29 seems good for me :)
RTBCing
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/9569999