Files: 
CommentFileSizeAuthor
#47 1797106-47-t-field.patch125.72 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 40,321 pass(es).
[ View ]
#44 1797106-44-t-field.patch126.15 KBdcam
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797106-44-t-field.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#39 1797106-36-t-field.patch89.32 KBdcam
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797106-36-t-field_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#37 1797106-37-t-field.patch43.91 KBdcam
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797106-37-t-field.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#36 1797106-36-t-field.patch89.32 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 39,550 pass(es).
[ View ]
#30 1797106-30-remove_t.patch29.39 KBzuuperman
PASSED: [[SimpleTest]]: [MySQL] 42,605 pass(es).
[ View ]
#27 1797106-27-remove_t.patch22.23 KBzuuperman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797106-27-remove_t.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#24 field-1797106-24.patch29.37 KBdcam
FAILED: [[SimpleTest]]: [MySQL] 42,567 pass(es), 0 fail(s), and 6 exception(s).
[ View ]
#21 1797106-21-t-field.patch30.78 KBLars Toomre
FAILED: [[SimpleTest]]: [MySQL] 42,179 pass(es), 0 fail(s), and 12 exception(s).
[ View ]
#21 interdiff-1797106-15-21.txt6.96 KBLars Toomre
#17 1797106-17-t-field.patch30.79 KBLars Toomre
FAILED: [[SimpleTest]]: [MySQL] 42,173 pass(es), 0 fail(s), and 12 exception(s).
[ View ]
#17 interdiff-1797106-15-17.txt6.97 KBLars Toomre
#15 1797106-15-t-field.patch28.86 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 41,900 pass(es).
[ View ]
#1 field-1797106-1.patch92.92 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 41,700 pass(es).
[ View ]

Comments

Assigned:xjm» Unassigned
Status:Active» Needs review
StatusFileSize
new92.92 KB
PASSED: [[SimpleTest]]: [MySQL] 41,700 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

I 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().

Issue tags:+needs backport to D7

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

This is a big patch @xjm, All of the message changes are appropriate. Hence this is RTBC.

Assigned:Unassigned» jhodgdon

I 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 :)

Ok, nevermind for now, I'm really freaking here.

Status:Reviewed & tested by the community» Needs work

Ok, so I was not freaking, there are still t() leftovers in there.

Status:Needs work» Reviewed & tested by the community

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

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

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

Status:Reviewed & tested by the community» Fixed

Committed to 8.x.

Assigned:jhodgdon» Unassigned

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

Status:Fixed» Needs work

oops, forgot status.

Title:Remove t() from asserts messages in tests for the field moduleRemove t() from asserts messages in tests for the Field module
Status:Needs work» Needs review
StatusFileSize
new28.86 KB
PASSED: [[SimpleTest]]: [MySQL] 41,900 pass(es).
[ View ]

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

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

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.php
@@ -38,27 +38,27 @@ class FieldInfoTest extends FieldTestBase {
-        $this->assertEqual($info[$t_key][$key], $val, t("Field type $t_key key $key is $val"));
+        $this->assertEqual($info[$t_key][$key], $val, "Field type $t_key key $key is $val");

-        $this->assertEqual($info[$f_key][$key], $val, t("Formatter type $f_key key $key is $val"));
+        $this->assertEqual($info[$f_key][$key], $val, "Formatter type $f_key key $key is $val");

-        $this->assertEqual($info[$s_key][$key], $val, t("Storage type $s_key key $key is $val"));
+        $this->assertEqual($info[$s_key][$key], $val, "Storage type $s_key key $key is $val");
<code>
-      $this->assertEqual($fields[$field['field_name']]['settings'][$key], $val, t("Field setting $key has correct default value $val"));
+      $this->assertEqual($fields[$field['field_name']]['settings'][$key], $val, "Field setting $key has correct default value $val");

-        $this->assertEqual($info[$t_key][$key], $val, t("Field type $t_key key $key is $val"));
+        $this->assertEqual($info[$t_key][$key], $val, "Field type $t_key key $key is $val");

-        $this->assertEqual($info[$f_key][$key], $val, t("Formatter type $f_key key $key is $val"));
+        $this->assertEqual($info[$f_key][$key], $val, "Formatter type $f_key key $key is $val");

-        $this->assertEqual($info[$s_key][$key], $val, t("Storage type $s_key key $key is $val"));
+        $this->assertEqual($info[$s_key][$key], $val, "Storage type $s_key key $key is $val");

-      $this->assertEqual($fields[$field['field_name']]['settings'][$key], $val, t("Field setting $key has correct default value $val"));
+      $this->assertEqual($fields[$field['field_name']]['settings'][$key], $val, "Field setting $key has correct default value $val");

StatusFileSize
new6.97 KB
new30.79 KB
FAILED: [[SimpleTest]]: [MySQL] 42,173 pass(es), 0 fail(s), and 12 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1797106-17-t-field.patch, failed testing.

I am wondering if the last four changes in the interdiff are correct. I do not immediately see what are causing the exceptions in #17.

@Lars Toomre, if you click the "detailed results" link, you can see

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.phpundefined
@@ -38,33 +38,33 @@ function testFieldInfo() {
-        $this->assertEqual($info[$f_key][$key], $val, "Formatter type $f_key key $key is $val");
+        $this->assertEqual($info[$f_key][$key], $val, format_string('Formatter type %t_key key %key is %value', array('%t_key' => $t_key, '%key' => $key, '%value' => $val)));
...
-        $this->assertEqual($info[$s_key][$key], $val, "Storage type $s_key key $key is $val");
+        $this->assertEqual($info[$s_key][$key], $val, format_string('Storage type %t_key key %key is %value', array('%t_key' => $t_key, '%key' => $key, '%value' => $val)));

These changes are incorrect--the original message uses $f_key and then $s_key but the updated one uses $t_key.

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.phpundefined
@@ -320,18 +320,18 @@ function testSettingsInfo() {
-      $this->assertIdentical(field_info_widget_settings($type), $info['settings'], "field_info_widget_settings returns {$type}'s widget settings");
+      $this->assertIdentical(field_info_widget_settings($type), $info['settings'], format_string("field_info_widget_settings returns {%type}'s widget settings", array('%type' => $type)));
...
-      $this->assertIdentical(field_info_formatter_settings($type), $data['settings'], "field_info_formatter_settings returns {$type}'s formatter settings");
+      $this->assertIdentical(field_info_formatter_settings($type), $data['settings'], format_string("field_info_formatter_settings returns {%type}'s formatter settings", array('%type' => $type)));

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.

StatusFileSize
new6.96 KB
new30.78 KB
FAILED: [[SimpleTest]]: [MySQL] 42,179 pass(es), 0 fail(s), and 12 exception(s).
[ View ]

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

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1797106-21-t-field.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new29.37 KB
FAILED: [[SimpleTest]]: [MySQL] 42,567 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

Rerolled #21.

Status:Needs review» Needs work

The last submitted patch, field-1797106-24.patch, failed testing.

The 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. :)

StatusFileSize
new22.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797106-27-remove_t.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled for latest dev + fixed the failing tests. When testing testFieldInfo. The value is an array, when the key is settings.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1797106-27-remove_t.patch, failed testing.

StatusFileSize
new29.39 KB
PASSED: [[SimpleTest]]: [MySQL] 42,605 pass(es).
[ View ]

Reroll was not needed. Patch now with the 2 latest test fixes only.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

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

Glad to see this RTBC.

Assigned:Unassigned» jhodgdon

Tum te tum...

Version:8.x-dev» 7.x-dev
Assigned:jhodgdon» Unassigned
Status:Reviewed & tested by the community» Patch (to be ported)

Committed to 8.x - that's 49 fewer strings for translators to deal with. :) Thanks! Ready for port...

Status:Patch (to be ported)» Needs review
StatusFileSize
new89.32 KB
PASSED: [[SimpleTest]]: [MySQL] 39,550 pass(es).
[ View ]

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

StatusFileSize
new43.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797106-37-t-field.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This one is a backport of #30.

Status:Needs review» Needs work

The last submitted patch, 1797106-37-t-field.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new89.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797106-36-t-field_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

Issue tags:-needs backport to D7

#39: 1797106-36-t-field.patch queued for re-testing.

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

The last submitted patch, 1797106-36-t-field.patch, failed testing.

Issue tags:+Novice

Tagging as Novice.

Status:Needs work» Needs review
StatusFileSize
new126.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797106-44-t-field.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled and combined #36 and #37.

#44: 1797106-44-t-field.patch queued for re-testing.

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

The last submitted patch, 1797106-44-t-field.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new125.72 KB
PASSED: [[SimpleTest]]: [MySQL] 40,321 pass(es).
[ View ]

Rerolled #44.

Status:Needs review» Reviewed & tested by the community

Hi,
this looks good to me
Thanks

Status:Reviewed & tested by the community» Fixed

Thanks all! Committed to 7.x.

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