Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Assigned: xjm » Unassigned
Status: Active » Needs review
FileSize
92.92 KB

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.

Lars Toomre’s picture

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

xjm’s picture

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.

Lars Toomre’s picture

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

xjm’s picture

Assigned: Unassigned » jhodgdon
swentel’s picture

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

swentel’s picture

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

swentel’s picture

Status: Reviewed & tested by the community » Needs work

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

xjm’s picture

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.

xjm’s picture

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.

xjm’s picture

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.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x.

jhodgdon’s picture

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.

jhodgdon’s picture

Status: Fixed » Needs work

oops, forgot status.

Lars Toomre’s picture

Title: Remove t() from asserts messages in tests for the field module » Remove t() from asserts messages in tests for the Field module
Status: Needs work » Needs review
FileSize
28.86 KB

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.

lazysoundsystem’s picture

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");
Lars Toomre’s picture

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.

Lars Toomre’s picture

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.

xjm’s picture

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

Lars Toomre’s picture

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

Lars Toomre’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

dcam’s picture

Status: Needs work » Needs review
FileSize
29.37 KB

Rerolled #21.

Status: Needs review » Needs work

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

jhodgdon’s picture

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

nils.destoop’s picture

FileSize
22.23 KB

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

nils.destoop’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

nils.destoop’s picture

FileSize
29.39 KB

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

nils.destoop’s picture

Status: Needs work » Needs review
dcam’s picture

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.

Lars Toomre’s picture

Glad to see this RTBC.

webchick’s picture

Assigned: Unassigned » jhodgdon

Tum te tum...

jhodgdon’s picture

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

dcam’s picture

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

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.

dcam’s picture

FileSize
43.91 KB

This one is a backport of #30.

Status: Needs review » Needs work

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

dcam’s picture

Status: Needs work » Needs review
FileSize
89.32 KB

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

lazysoundsystem’s picture

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.

dcam’s picture

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.

dcam’s picture

Issue tags: +Novice

Tagging as Novice.

dcam’s picture

Status: Needs work » Needs review
FileSize
126.15 KB

Rerolled and combined #36 and #37.

izus’s picture

Issue tags: -Novice, -Needs backport to D7

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

dcam’s picture

Status: Needs work » Needs review
FileSize
125.72 KB

Rerolled #44.

izus’s picture

Status: Needs review » Reviewed & tested by the community

Hi,
this looks good to me
Thanks

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.