Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mrsinguyen’s picture

Status: Active » Needs review
FileSize
873 bytes

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
FileSize
881 bytes
+++ 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
FileSize
2.05 KB

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
FileSize
2.22 KB

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
FileSize
69.45 KB

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
FileSize
2.22 KB

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

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
FileSize
1.83 KB

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.