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

mrsinguyen’s picture

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.

lokapujya’s picture

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.

mrsinguyen’s picture

Status:Needs review» Reviewed & tested by the community

Look good.

catch’s picture

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.

lokapujya’s picture

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

Bladedu’s picture

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.

lokapujya’s picture

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

lokapujya’s picture

Status:Needs review» Needs work

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

Bladedu’s picture

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.

Bladedu’s picture

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.

pfrenssen’s picture

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.

pfrenssen’s picture

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

alexpott’s picture

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

Patch no longer applies.

Bladedu’s picture

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

Rerolled patch #11.

lokapujya’s picture

+++ 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?

Bladedu’s picture

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.

tim.plunkett’s picture

Issue tags:-Needs reroll

Removing tags

Bladedu’s picture

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.

Bladedu’s picture

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

Let's try one more time...

lokapujya’s picture

Status:Needs review» Reviewed & tested by the community

Thanks for condensing the comment.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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