Part of meta-issue #2002650: [meta] improve maintainability by removing unused local variables

File /core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php

Line 58: Unused local variable $conf
Line 183: Unused local variable $i

Files: 
CommentFileSizeAuthor
#21 remove-unused-local-variable.patch1.83 KBBladedu
PASSED: [[SimpleTest]]: [MySQL] 58,414 pass(es).
[ View ]
#19 remove-unused-local-variable.patch2.22 KBBladedu
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-unused-local-variable_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 remove-unused-local-variable.patch2.22 KBBladedu
PASSED: [[SimpleTest]]: [MySQL] 58,686 pass(es).
[ View ]
#12 1952062-translation-module-56.patch69.45 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 58,099 pass(es).
[ View ]
#11 remove-unused-local-variable.patch2.22 KBBladedu
PASSED: [[SimpleTest]]: [MySQL] 58,947 pass(es).
[ View ]
#7 remove-unused-local-variable.patch2.05 KBBladedu
PASSED: [[SimpleTest]]: [MySQL] 59,010 pass(es).
[ View ]
#3 remove-unused-local-variable-2080401-3.patch881 byteslokapujya
PASSED: [[SimpleTest]]: [MySQL] 58,481 pass(es).
[ View ]
#1 drupal-core-remove-unused-local-variable-2080401.patch873 bytesmrsinguyen
FAILED: [[SimpleTest]]: [MySQL] 59,090 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new873 bytes
FAILED: [[SimpleTest]]: [MySQL] 59,090 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal-core-remove-unused-local-variable-2080401.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new881 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,481 pass(es).
[ View ]

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php
@@ -179,9 +178,6 @@ function stubTest() {
-    // Generates a warning.
-    $i = 1 / 0;
-

The test is purposely generating a divide by zero here.

I removed the assignment; Seems weird, but should work.

Status:Needs review» Reviewed & tested by the community

Look good.

Status:Reviewed & tested by the community» Needs work

I think we should leave the $i = 1/ 0 in. It's redundant but it looks really weird not to have it.

Can we generate the error another way or USE the variable so that we accomplish the goal of getting rid of the unused variable?

Status:Needs work» Needs review
StatusFileSize
new2.05 KB
PASSED: [[SimpleTest]]: [MySQL] 59,010 pass(es).
[ View ]

I changed the way the warning error is triggered by using trigger_error() function without passing any argument. This should get rid of the unused variable.

This works for me. I'll let a 2nd eye RTBC it.

Status:Needs review» Needs work

On second thought, I think the error message is a required parameter to trigger_error().

The trigger_error requires at least one parameter. Omitting it I was able to trigger the E_WARNING error. Unfortunately trigger_error accepts only E_USER_* types of errors, so it can't be used properly. I will update the patch with some documentation.

Status:Needs work» Needs review
StatusFileSize
new2.22 KB
PASSED: [[SimpleTest]]: [MySQL] 58,947 pass(es).
[ View ]

Re-rolled the patch in comment #7 with some comment on how trigger_error() function has been used.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new69.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,099 pass(es).
[ View ]

Adding the comment clarifies the usage. Funny that you are triggering an error inside of trigger_error() :)

If the test comes back green this is RTBC for me.

Ignore the patch from #12, I must have had too many beers. The right patch is #11.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Needs review
StatusFileSize
new2.22 KB
PASSED: [[SimpleTest]]: [MySQL] 58,686 pass(es).
[ View ]

Rerolled patch #11.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php
@@ -214,8 +215,10 @@ function confirmStubTestResults() {
+    /// Check that a warning is caught by simpletest. The exact error message

Comment contains an extra /

Not a big deal, but can the comments be condensed to 2 lines?

You're absolutely right! I made a mistake while rerolling the patch and I haven't noticed it. I'll fix the patch and post it back.

Issue tags:-Needs reroll

Removing tags

StatusFileSize
new2.22 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-unused-local-variable_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Removed the extra "/".
Comments cannot be condensed in two lines because it will overflow 80 character length as specified in the Drupal coding standards document.

Status:Needs review» Needs work

The last submitted patch, remove-unused-local-variable.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.83 KB
PASSED: [[SimpleTest]]: [MySQL] 58,414 pass(es).
[ View ]

Let's try one more time...

Status:Needs review» Reviewed & tested by the community

Thanks for condensing the comment.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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