Files: 
CommentFileSizeAuthor
#61 1797252-61-t-file.patch53.56 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 40,216 pass(es).
[ View ]
#57 1797252-55-t-file.patch106.43 KBpwieck
PASSED: [[SimpleTest]]: [MySQL] 57,975 pass(es).
[ View ]
#55 1797252-55-t-file .patch106.43 KBpwieck
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797252-55-t-file .patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#53 1797252-53-t-file .patch106.43 KBpwieck
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797252-53-t-file .patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#47 1797252-47-t-file.patch106.41 KBpwieck
FAILED: [[SimpleTest]]: [MySQL] 57,961 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#45 1797252-42-t-file.patch106.41 KBpwieck
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/lib/Drupal/file/Tests/SaveUploadTest.php.
[ View ]
#42 1797252-42-t-file.patch106.41 KBpwieck
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/lib/Drupal/file/Tests/SaveTest.php.
[ View ]
#38 1797252-38-t-file.patch104.9 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 55,264 pass(es).
[ View ]
#32 1797252-32-t-file.patch105.73 KBdcam
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/lib/Drupal/file/Tests/FileFieldValidateTest.php.
[ View ]
#26 1797252-t-file-26.patch105.73 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797252-t-file-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#22 1797252-t-file-22.patch104.61 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,357 pass(es).
[ View ]
#18 1797252-t-file-18.patch104.62 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,936 pass(es).
[ View ]
#13 interdiff.txt973 bytesandypost
#13 1797252-t-file-13.patch104.02 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797252-t-file-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 t-file-1799752-11.patch104.07 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 52,257 pass(es).
[ View ]
#7 file-1799752-7.patch103.84 KBdcam
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file-1799752-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 1799752-6-t-file.patch107.27 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 41,898 pass(es).
[ View ]
#1 file-1797252-1.patch70.1 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 41,696 pass(es).
[ View ]

Comments

xjm’s picture

Assigned:xjm» Unassigned
Status:Active» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new70.1 KB
PASSED: [[SimpleTest]]: [MySQL] 41,696 pass(es).
[ View ]

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

StatusFileSize
new107.27 KB
PASSED: [[SimpleTest]]: [MySQL] 41,898 pass(es).
[ View ]

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

StatusFileSize
new103.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file-1799752-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

#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
StatusFileSize
new104.07 KB
PASSED: [[SimpleTest]]: [MySQL] 52,257 pass(es).
[ View ]

Rerolled #7.

dcam’s picture

Issue tags:+Novice

Tagging as Novice.

andypost’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new104.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797252-t-file-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new973 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

#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
StatusFileSize
new104.62 KB
PASSED: [[SimpleTest]]: [MySQL] 54,936 pass(es).
[ View ]

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
StatusFileSize
new104.61 KB
PASSED: [[SimpleTest]]: [MySQL] 55,357 pass(es).
[ View ]

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

StatusFileSize
new105.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797252-t-file-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Cottser’s picture

Issue tags:+Needs reroll

Tagging for reroll.

dcam’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new105.73 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/lib/Drupal/file/Tests/FileFieldValidateTest.php.
[ View ]

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.

Cottser’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
StatusFileSize
new104.9 KB
PASSED: [[SimpleTest]]: [MySQL] 55,264 pass(es).
[ View ]

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
StatusFileSize
new106.41 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/lib/Drupal/file/Tests/SaveTest.php.
[ View ]

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
StatusFileSize
new106.41 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/file/lib/Drupal/file/Tests/SaveUploadTest.php.
[ View ]

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
StatusFileSize
new106.41 KB
FAILED: [[SimpleTest]]: [MySQL] 57,961 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

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
StatusFileSize
new106.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797252-53-t-file .patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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
StatusFileSize
new106.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797252-55-t-file .patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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
StatusFileSize
new106.43 KB
PASSED: [[SimpleTest]]: [MySQL] 57,975 pass(es).
[ View ]

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
StatusFileSize
new53.56 KB
PASSED: [[SimpleTest]]: [MySQL] 40,216 pass(es).
[ View ]

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.