Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Assigned: xjm » Unassigned
Status: Active » Needs review
Issue tags: +Needs backport to D7
FileSize
70.1 KB

Larger one. No format_string().

git diff --color-words to review.

Lars Toomre’s picture

I did not apply this patch yet, but it appears several straight removals were missed. Shouldn't these be included?

+++ b/core/modules/file/lib/Drupal/file/Tests/SaveDataTest.phpundefined
@@ -26,12 +26,12 @@ class SaveDataTest extends FileManagedTestBase {
 
     $this->assertEqual(file_default_scheme(), file_uri_scheme($result->uri), t("File was placed in Drupal's files directory."));
     $this->assertEqual($result->filename, drupal_basename($result->uri), t("Filename was set to the file's basename."));
-    $this->assertEqual($contents, file_get_contents($result->uri), t('Contents of the file are correct.'));
-    $this->assertEqual($result->filemime, 'application/octet-stream', t('A MIME type was set.'));
+    $this->assertEqual($contents, file_get_contents($result->uri), 'Contents of the file are correct.');
+    $this->assertEqual($result->filemime, 'application/octet-stream', 'A MIME type was set.');
     $this->assertEqual($result->status, FILE_STATUS_PERMANENT, t("The file's status was set to permanent."));

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.

xjm’s picture

Component: dblog.module » file.module

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

Lars Toomre’s picture

Ah, thanks... All of the files on this topic I generated by hand. I did not realize you were using a script. Thanks.

xjm’s picture

See #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 of t() to see which can be removed.

Lars Toomre’s picture

FileSize
107.27 KB

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

dcam’s picture

FileSize
103.84 KB

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

Lars Toomre’s picture

Thanks for the re-roll @dcam. Unfortunately, I do not think I can RTBC this one either as I rolled the expanded patch in #6.

dcam’s picture

Issue tags: -Needs backport to D7

#7: file-1799752-7.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, file-1799752-7.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review
FileSize
104.07 KB

Rerolled #7.

dcam’s picture

Issue tags: +Novice

Tagging as Novice.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
104.02 KB
973 bytes

Re-roll for current HEAD
Found small issue once reviewed

xjm’s picture

Assigned: Unassigned » jhodgdon
jhodgdon’s picture

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

webchick’s picture

Issue tags: -Novice, -Needs backport to D7

#13: 1797252-t-file-13.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice, +Needs backport to D7

The last submitted patch, 1797252-t-file-13.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
104.62 KB

Re-roll for current head

andypost’s picture

jhodgdon’s picture

RE #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.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned

Actually I'm unassigning this until it gets reviewed.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
104.61 KB

re-roll again, there's nothing to review just simple merge
Also it could hev conflicts with #1875992: Add EntityFormDisplay objects for entity forms

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Thanks! Assigning to me to do final review/commit if/when there are no conflicts with "avoid commit conflicts" issues.

jhodgdon’s picture

There'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.

andypost’s picture

@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

andypost’s picture

FileSize
105.73 KB

re-roll

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1797252-t-file-26.patch, failed testing.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Needs backport to D7

#26: 1797252-t-file-26.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Needs backport to D7

The last submitted patch, 1797252-t-file-26.patch, failed testing.

star-szr’s picture

Issue tags: +Needs reroll

Tagging for reroll.

dcam’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
105.73 KB

Rerolled #26.

Status: Needs review » Needs work
Issue tags: -Novice, -Needs backport to D7

The last submitted patch, 1797252-32-t-file.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review

#32: 1797252-32-t-file.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Needs backport to D7

The last submitted patch, 1797252-32-t-file.patch, failed testing.

star-szr’s picture

Thanks @dcam!

FileFieldValidateTest.php and FileFieldWidgetTest.php have unmerged changes - I think #26 was not a complete merge.

e.g.:

+++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldWidgetTest.phpundefined
@@ -216,22 +224,27 @@ function testPrivateFileSetting() {
+<<<<<<< HEAD
+    $node_file = file_load($node->{$field_name}[LANGUAGE_NOT_SPECIFIED][0]['fid']);
+    $this->assertFileExists($node_file, 'New file saved to disk on node creation.');
+=======
     $node_file = file_load($node->{$field_name}[Language::LANGCODE_NOT_SPECIFIED][0]['fid']);
     $this->assertFileExists($node_file, t('New file saved to disk on node creation.'));
+>>>>>>> 8.x
dcam’s picture

Oooh. Yeah. I knew I should have just checked the file. I'll get right on it.

dcam’s picture

Status: Needs work » Needs review
FileSize
104.9 KB

Rerolled #22.

jhodgdon’s picture

bump! We only have a couple of these left. Can someone review this patch?

andypost’s picture

Status: Needs review » Needs work

Patch needs re-roll but everythinf is fine

+++ b/core/modules/file/lib/Drupal/file/Tests/SaveUploadTest.phpundefined
@@ -131,10 +131,10 @@ function testHandleExtension() {
+    $this->assertRaw(t('Epic upload FAIL!'), 'Found the failure message.');

@@ -151,9 +151,9 @@ function testHandleExtension() {
+    $this->assertRaw(t('You WIN!'), 'Found the success message.');

funny!

pwieck’s picture

Assigned: Unassigned » pwieck

Working on reroll.

pwieck’s picture

Assigned: pwieck » Unassigned
Status: Needs work » Needs review
FileSize
106.41 KB

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.

Status: Needs review » Needs work

The last submitted patch, 1797252-42-t-file.patch, failed testing.

pwieck’s picture

Assigned: Unassigned » pwieck

My mistake. Fixing

pwieck’s picture

Assigned: pwieck » Unassigned
Status: Needs work » Needs review
FileSize
106.41 KB

Fixed
'Timestamp didn't go backwards.' --> "Timestamp didn't go backwards."

Status: Needs review » Needs work

The last submitted patch, 1797252-42-t-file.patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review
FileSize
106.41 KB

Holly cow! did it again

'The image file we're going to upload exists.'); --> "The image file we're going to upload exists.");

Status: Needs review » Needs work
Issue tags: -Novice, -Needs backport to D7

The last submitted patch, 1797252-47-t-file.patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review

#47: 1797252-47-t-file.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1797252-47-t-file.patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review

#47: 1797252-47-t-file.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Needs backport to D7

The last submitted patch, 1797252-47-t-file.patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review
FileSize
106.43 KB

missed a format_string. Sure taking the long road on this one. sigh :-(

Status: Needs review » Needs work

The last submitted patch, 1797252-53-t-file .patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review
FileSize
106.43 KB

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.

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, 1797252-55-t-file .patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review
FileSize
106.43 KB

I'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.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

@pwieck thanx a lot for serious re-roll, let's get this in

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 795e1f8 and pushed to 8.x. Thanks!

alexpott’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
dcam’s picture

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

Backported #57 to D7.

lazysoundsystem’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Reviewed & tested by the community

I've checked it, applies cleanly, passes tests and all looks good. RTBC.

lazysoundsystem’s picture

Version: 8.x-dev » 7.x-dev

Sorry, didn't mean to change version number. Back to 7.x still RTBC.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks all! Committed to 7.x.

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