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.
Part of meta #500866: [META] remove t() from assert message.
Comment | File | Size | Author |
---|---|---|---|
#47 | 1797106-47-t-field.patch | 125.72 KB | dcam |
#44 | 1797106-44-t-field.patch | 126.15 KB | dcam |
#39 | 1797106-36-t-field.patch | 89.32 KB | dcam |
#37 | 1797106-37-t-field.patch | 43.91 KB | dcam |
#36 | 1797106-36-t-field.patch | 89.32 KB | dcam |
Comments
Comment #1
xjmTip for reviewers: apply the patch locally and use
git diff --color-words
.Edit: Note that, since this one is so large, it converts only the plain string messages. For
format_string()
we can likely do a followup.Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedI have reviewed each of the proposed message changes in #1 and all are appropriate. Hence, with a green bot response, this is RTBC.
The tests in this module definitely will need another patch to catch the changes to format_string().
Comment #3
xjmJust talked to @swentel in IRC about #1040790: _field_info_collate_fields() memory usage. We should make sure not to collide with that. Risk is low with these cleanups, but he already had to reroll it once today because of the Field API cleanup patch.
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commentedThis is a big patch @xjm, All of the message changes are appropriate. Hence this is RTBC.
Comment #5
xjmComment #6
swentel CreditAttribution: swentel commentedI think some t() asserts were missed in FieldInfoTest.php - I mistakenly first thought that was a new file from the patch mentioned in #3, but it isn't
eg: t("Field type $t_key key $key is $val")
So this is still needs work imo, but I'll let you guys review first. Hopefully by then I get #1040790: _field_info_collate_fields() memory usage green too :)
Comment #7
swentel CreditAttribution: swentel commentedOk, nevermind for now, I'm really freaking here.
Comment #8
swentel CreditAttribution: swentel commentedOk, so I was not freaking, there are still t() leftovers in there.
Comment #9
xjmThe patch is already 100k, so I didn't look for missing ones when I reviewed it. Let's fix this and then set back to NW if you can confirm a specific message that is not fixed?
Edit: And I don't quite follow #3, but anything that uses variables inside the assertion message is deliberately omitted from this patch. :) See the discussion in #500866: [META] remove t() from assert message.
Comment #10
xjmI also tagged #1040790: _field_info_collate_fields() memory usage to avoid conflicts because it looks like you didn't yesterday. :) The absolute last thing we want to do is disrupt major API cleanups that have been open for a year.
Comment #11
xjmAnd taking back what I said in #10 after discussing with @swentel in IRC. That patch is broken on account of widgets-as-plugins, so this patch is good to go in whenever.
Comment #12
Dries CreditAttribution: Dries commentedCommitted to 8.x.
Comment #13
jhodgdonSetting back to needs work so the rest of the assertions can be found/fixed, as per previous comments. Then we need to backport to 7.
Comment #14
jhodgdonoops, forgot status.
Comment #15
Lars Toomre CreditAttribution: Lars Toomre commentedHere is a untested patch that I believe cleans up all of the remaining t() removal around assert messages. This is very heavily oriented to changing t() to format_string(), but there are some straight removals as well.
Comment #16
lazysoundsystem CreditAttribution: lazysoundsystem commentedA few more
format_string
functions required in FieldInfoTest.php. The rest are good.And #1040790: _field_info_collate_fields() memory usage has been committed for 8.x, so that's okay.
Comment #17
Lars Toomre CreditAttribution: Lars Toomre commentedThanks for the review @lazysoundsystem. I must have been half-asleep when first editing FieldInfoTest.php.
Here is a corrected patch and interdiff after going through every assert again in that Test file. I think that this corrects all of your comments in #16.
Comment #19
Lars Toomre CreditAttribution: Lars Toomre commentedI am wondering if the last four changes in the interdiff are correct. I do not immediately see what are causing the exceptions in #17.
Comment #20
xjm@Lars Toomre, if you click the "detailed results" link, you can see
These changes are incorrect--the original message uses
$f_key
and then$s_key
but the updated one uses$t_key
.This change is also incorrect;
%type
should not have curlies.If you check the "detailed results" for the test above, you can see that the error is:
htmlspecialchars() expects parameter 1 to be string, array given
Maybe that could be caused by the way
var_export()
is being used. I'd suggest running it locally to debug.Comment #21
Lars Toomre CreditAttribution: Lars Toomre commented@xjm Thanks for your review. On this machine, I do not have a local test environment for D8. Hence, my patches are untested locally.
The attached patch addresses the comments in #20 and for convenience an interdiff is provided vs #15.
Comment #22
Lars Toomre CreditAttribution: Lars Toomre commentedComment #24
dcam CreditAttribution: dcam commentedRerolled #21.
Comment #26
jhodgdonThe testing errors are in field tests, so are probably real. (Click "View details" to see them.) Probably a syntax error in the patch -- if you can't find it, post here to ask for help. :)
Comment #27
nils.destoop CreditAttribution: nils.destoop commentedRerolled for latest dev + fixed the failing tests. When testing testFieldInfo. The value is an array, when the key is settings.
Comment #28
nils.destoop CreditAttribution: nils.destoop commentedComment #30
nils.destoop CreditAttribution: nils.destoop commentedReroll was not needed. Patch now with the 2 latest test fixes only.
Comment #31
nils.destoop CreditAttribution: nils.destoop commentedComment #32
dcam CreditAttribution: dcam commentedSorry about the unneeded reroll. I'm not certain why #21 wouldn't apply for me and I thought it was necessary.
Anyway, I applied #30 and checked all the field tests. I didn't find any more t()'s around assert messages. The patch looks good to me.
Comment #33
Lars Toomre CreditAttribution: Lars Toomre commentedGlad to see this RTBC.
Comment #34
webchickTum te tum...
Comment #35
jhodgdonCommitted to 8.x - that's 49 fewer strings for translators to deal with. :) Thanks! Ready for port...
Comment #36
dcam CreditAttribution: dcam commentedThis is a backport of #1 ONLY. It does not include #30 or any additional t() removals that may be needed. The patch is large enough as it is, so I figure it's best to split them up just as the D8 patches were.
Comment #37
dcam CreditAttribution: dcam commentedThis one is a backport of #30.
Comment #39
dcam CreditAttribution: dcam commented#37 probably doesn't apply because it expected changes in #36. I'm only re-submitting #36 so the issue turn green and it can get reviewed. Once #36 is committed, #37 can be retested.
Comment #40
lazysoundsystem CreditAttribution: lazysoundsystem commentedI've reviewed #36/#39 and all the changes are good.
I suggest including the changes in #37 and an interdiff, so it could go in as one commit. Otherwise happy to RTBC this part.
Comment #41
dcam CreditAttribution: dcam commented#39: 1797106-36-t-field.patch queued for re-testing.
Comment #43
dcam CreditAttribution: dcam commentedTagging as Novice.
Comment #44
dcam CreditAttribution: dcam commentedRerolled and combined #36 and #37.
Comment #45
izus CreditAttribution: izus commented#44: 1797106-44-t-field.patch queued for re-testing.
Comment #47
dcam CreditAttribution: dcam commentedRerolled #44.
Comment #48
izus CreditAttribution: izus commentedHi,
this looks good to me
Thanks
Comment #49
jhodgdonThanks all! Committed to 7.x.