Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Part of meta #500866: [META] remove t() from assert message
Comment | File | Size | Author |
---|---|---|---|
#61 | 1797252-61-t-file.patch | 53.56 KB | dcam |
#57 | 1797252-55-t-file.patch | 106.43 KB | pwieck |
#55 | 1797252-55-t-file .patch | 106.43 KB | pwieck |
#53 | 1797252-53-t-file .patch | 106.43 KB | pwieck |
#47 | 1797252-47-t-file.patch | 106.41 KB | pwieck |
Comments
Comment #1
xjmLarger one. No
format_string()
.git diff --color-words
to review.Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedI did not apply this patch yet, but it appears several straight removals were missed. Shouldn't these be included?
I am not sure what was going on here... Looks like several straight t() removals were missed her. Am I wrong? Also down below in this test too.
Comment #3
xjmThese weren't matched by the script because they contained apostrophes, and I didn't go through every file by hand for this issue because I'd already had to proof 70 K of changes. Feel free to update the patch to add them.
Also, correcting the component.
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commentedAh, thanks... All of the files on this topic I generated by hand. I did not realize you were using a script. Thanks.
Comment #5
xjmSee #500866-209: [META] remove t() from assert message. It's very conservative in order to avoid false positives, other than the false positive with the
$group
parameter, which is why I assumed the other issues had those! :) So you still need to check its changes by hand, and for a smaller set of tests I still have been going through every use oft()
to see which can be removed.Comment #6
Lars Toomre CreditAttribution: Lars Toomre commentedThanks for your explanation in #5 @xjm.
I took the scripted patch results from #1, applied it locally, and then went through and did a hand review of all resulting t() occurances that remained. Those that needed to be converted, including those that needed format_string() conversion, have now been done so. The results are in this now complete patch, at least for me locally according to grep.
Note: I did not check for all assert messages (which might have included one or more string concatenations, which should be converted to format_string()). I figured that this patch was going to be big enough.
I have not yet tested this so let's see what the test bots think.
Comment #7
dcam CreditAttribution: dcam commented#6 needed to be rerolled due to changes in DeleteTest.php and UsageTest.php
I checked all the asserts after rerolling it. I didn't find any more t()'s in the messages. I can't RTBC it though due to the reroll.
Comment #8
Lars Toomre CreditAttribution: Lars Toomre commentedThanks for the re-roll @dcam. Unfortunately, I do not think I can RTBC this one either as I rolled the expanded patch in #6.
Comment #9
dcam CreditAttribution: dcam commented#7: file-1799752-7.patch queued for re-testing.
Comment #11
dcam CreditAttribution: dcam commentedRerolled #7.
Comment #12
dcam CreditAttribution: dcam commentedTagging as Novice.
Comment #13
andypostRe-roll for current HEAD
Found small issue once reviewed
Comment #14
xjmComment #15
jhodgdonThis patch touches some test files that an "avoid commit conflicts" issue also touches, so I'm going to wait on committing it until
#1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence
is taken care of. Unfortunately, it might need to be rerolled at that point.
Comment #16
webchick#13: 1797252-t-file-13.patch queued for re-testing.
Comment #18
andypostRe-roll for current head
Comment #19
andypostAnd while #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence still needs work we can commit this
Comment #20
jhodgdonRE #19 - that other issue is tagged "avoid commit conflicts", so this one will have to wait. Even if it is temporarily at "needs work", I would not commit this issue until it is done, as it would likely impede progress.
Comment #21
jhodgdonActually I'm unassigning this until it gets reviewed.
Comment #22
andypostre-roll again, there's nothing to review just simple merge
Also it could hev conflicts with #1875992: Add EntityFormDisplay objects for entity forms
Comment #23
jhodgdonThanks! Assigning to me to do final review/commit if/when there are no conflicts with "avoid commit conflicts" issues.
Comment #24
jhodgdonThere's yet another "avoid commit conflicts" patch that touches some of the same files as this one and may conflict:
#731724: Convert comment settings into a field to make them work with CMI and non-node entities (the latest patch is on page 2 of the comments: http://drupal.org/node/731724?page=1 down at the bottom; the "latest comment" and "latest patch" links do not work on pages with more than 300 comments, which is a known issue)
So this will need to wait for commit until that other issue is resolved.
Comment #25
andypost@jhodgdon Patch still in progress see #1907960-177: Helper issue for "Comment field" and as one of authors I just dont want to add another comment to #731724: Convert comment settings into a field to make them work with CMI and non-node entities just to remove tag
Comment #26
andypostre-roll
Comment #28
jhodgdonComment #29
jhodgdon#26: 1797252-t-file-26.patch queued for re-testing.
Comment #31
star-szrTagging for reroll.
Comment #32
dcam CreditAttribution: dcam commentedRerolled #26.
Comment #34
dcam CreditAttribution: dcam commented#32: 1797252-32-t-file.patch queued for re-testing.
Comment #36
star-szrThanks @dcam!
FileFieldValidateTest.php and FileFieldWidgetTest.php have unmerged changes - I think #26 was not a complete merge.
e.g.:
Comment #37
dcam CreditAttribution: dcam commentedOooh. Yeah. I knew I should have just checked the file. I'll get right on it.
Comment #38
dcam CreditAttribution: dcam commentedRerolled #22.
Comment #39
jhodgdonbump! We only have a couple of these left. Can someone review this patch?
Comment #40
andypostPatch needs re-roll but everythinf is fine
funny!
Comment #41
pwieck CreditAttribution: pwieck commentedWorking on reroll.
Comment #42
pwieck CreditAttribution: pwieck commentedReroll to current head. No diff file because it was almost a complete redo since head files had changed so much. The diff was as near as long as patch.
Comment #44
pwieck CreditAttribution: pwieck commentedMy mistake. Fixing
Comment #45
pwieck CreditAttribution: pwieck commentedFixed
'Timestamp didn't go backwards.' --> "Timestamp didn't go backwards."
Comment #47
pwieck CreditAttribution: pwieck commentedHolly cow! did it again
'The image file we're going to upload exists.'); --> "The image file we're going to upload exists.");
Comment #49
pwieck CreditAttribution: pwieck commented#47: 1797252-47-t-file.patch queued for re-testing.
Comment #51
pwieck CreditAttribution: pwieck commented#47: 1797252-47-t-file.patch queued for re-testing.
Comment #53
pwieck CreditAttribution: pwieck commentedmissed a format_string. Sure taking the long road on this one. sigh :-(
Comment #55
pwieck CreditAttribution: pwieck commentedReroll to current head. No diff file because it was almost a complete redo since head files had changed so much. The diff was as near as long as patch.
Comment #57
pwieck CreditAttribution: pwieck commentedI'm sooo tied I can't seem to get this one right. Keep putting space in file name.
Reroll to current head. No diff file because it was almost a complete redo since head files had changed so much. The diff was as near as long as patch.
Comment #58
andypost@pwieck thanx a lot for serious re-roll, let's get this in
Comment #59
alexpottCommitted 795e1f8 and pushed to 8.x. Thanks!
Comment #60
alexpottComment #61
dcam CreditAttribution: dcam commentedBackported #57 to D7.
Comment #62
lazysoundsystem CreditAttribution: lazysoundsystem commentedI've checked it, applies cleanly, passes tests and all looks good. RTBC.
Comment #63
lazysoundsystem CreditAttribution: lazysoundsystem commentedSorry, didn't mean to change version number. Back to 7.x still RTBC.
Comment #64
jhodgdonThanks all! Committed to 7.x.