Remove Unused local variables from system module

Remove an unused variable.

can't apply patch

The test moved to PHPUnitTest in #2130551: Convert system modules MimeTypeMatcherTest to phpunit, but we still have the unused variable.

4: unused-local-variables-2080343-3.patch queued for re-testing.

Looks good; the patch applies and removes the variable while keeping the test.

Please roll all system.module local variable removals into one patch, and confirm that all unused local variables are removed from the module directory before RTBCing. Also, these issues should be filed as minor.

Please combine this patch with #2080343: Remove Unused local variables from system module.

Whoops, I just closed this as a duplicate of itself. :D

includes:
#2080117: Add assertion to Drupal/system/Tests/Database/DeleteTruncateTest.php

but not:
#2056445: Ensure that all config can not be deleted using the config importer (has a bug associated with it.)
#2080311: Clean up Drupal\system\Tests\Database\SelectTest.php (xjm asked for it to have a separate review)

Both issues have already individually been RTBC. The latest patch combines the open "unused variables" tasks for the system module.

Looks good to me. Applies fine.

Updating title and adding some more files for update.

Adding patch to be review.
@xjm, Please suggest if more needs to care.

raul.shinde, those issues should to be reviewed separately since they are more complicated. If you agree with that plan, please set this back to RTBC.

18: 2080343-18-work_arround_for_unused_variable.patch queued for re-testing.

@lokapujya, As per @xjm we should take care all variables in one place. So i did. You suggest the way that we can work upon. I don;t have any problem in any case :)

Sorry but the latest patch doesn't apply..

#2080117: Add assertion to Drupal/system/Tests/Database/DeleteTruncateTest.php was committed. So #4 is the one to review.

The other 2 issues should not be combined, see comment #14.

@lokapujya, i think we are loosing on the issue title here. @xjm, please pitch in here n throws some suggestion, thought #14 says much but the title says other thing.

Review for the latest patch

The patch seems to be secure. Fields are checked for errors and unexpected content. No sign of 'smells'. Code is not duplicated and its very purpose is to make the codebase more efficient in which it succeeds. There are no tests provided but as far as I know, the change is not unit testable (since this is the essence of the patch, I do not count this as a 'smell'). The patch applies cleanly (as of commit 588e6783d29759b66d22651285b9e2c49640b345) and fixes the issue.

Ok, some other issues need to be combined.

ProgrammaticTest.php and FrontPageTest.php changes are new changes. The other changes are patches applied from the "individual file" issues.

+++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/ElementValidationTest.php
@@ -28,11 +28,10 @@ public static function getInfo() {
-    $web_user = $this->drupalCreateUser();

Also, I removed this line.

anyone get a chance to review this before we have to re-roll again?

hi,
i reviewed it and made a last check. i found some unused use statements.
will upload a patch in minutes

so here it is !
i deted unused use statements, extra unused variables and some extra semicolons !
Hopefully this goes green.

adding the interdiff.

I didn't create that interdiff correctly. Here is a redo.

izus, those extra unused variable are being handled in other issues (that are not RTBC'd) mentioned in the issue summary, specifically:
/core/modules/system/lib/Drupal/system/SystemConfigSubscriber.php
/core/modules/system/lib/Drupal/system/Tests/Database/SelectTest.php

+++ b/core/modules/system/entity.api.php
@@ -355,11 +355,13 @@ function hook_entity_translation_insert(\Drupal\Core\Entity\EntityInterface $tra
-  $variables = array(
-    '@language' => $languages[$langcode]->name,
-    '@label' => $entity->label(),
-  );
-  watchdog('example', 'The @language translation of @label has just been deleted.', $variables);
+  foreach ($languages as $langcode => $entity) {
+    $variables = array(
+      '@language' => $languages[$langcode]->name,
+      '@label' => $entity->label(),
+    );
+    watchdog('example', 'The @language translation of @label has just been deleted.', $variables);
+  }

This change is bug fix, and should be filed as a separate issue.

I think that the unused use statements is out of scope and should be done in another issue.

#2056445: Ensure that all config can not be deleted using the config importer is RTBC'd and #2080311: Clean up Drupal\system\Tests\Database\SelectTest.php is active, so we don't need those changes in this patch. So, I think patch #32 is still the one to review.

Perhaps the unused use statements and extra semicolons from #38 can be a new issue.

Spawned a new issue #2187477: Remove Unused use statements from system module for the unused use statements.

corrected info about which issue would handle MimeTypeMatcherTest

38: 2080343-38.patch queued for re-testing.

32: 2080343-32.patch queued for re-testing.

Reroll.

54: 2080343-54.patch queued for re-testing.

Rerolled.

$routes in the RouteBuilderTest is actually used now, so it shouldn't be removed. Fixed.

I gave this a quick look. Nothing worrying jumps out.

In terms of a review, i think what this needs is:
someone to look at the vars that are removed and
*think* about if the values should have been used:

should we have tested the results of the functions?

Were there conditions we were missing or asserts we might have wanted to do?

Just rerolled this patch.
Still needs a full review.

+++ b/core/modules/system/src/Tests/Form/ProgrammaticTest.php
   function testSubmissionWorkflow() {
-    // Backup the current batch status and reset it to avoid conflicts while
-    // processing the dummy form submit handler.
-    $current_batch = $batch =& batch_get();
-    $batch = array();
...
-    // Restore the current batch status.
-    $batch = $current_batch;

These lines should not be touched by this patch.

I noticed phpStorm was saying that these variables are used, but I noticed they are unused anywhere else in the function.

I realized that these lines are tricking phpStorm into thinking they are used because they are using each other.
If there is another explanation for why these should not be removed then please explain.

Removing them does not make the patch fail during testing. If they really need to be there that means we are missing an assertion.

I agree with the change to needs work though as there are still a ton of unused variables.
I'll be working on that now.

Removed all the unused files in just one file so far.
I found an example of how to fix the unused $key in a foreach loop here: #2080309: Remove Unused local variable $record from /core/modules/system/lib/Drupal/system/Tests/Database/AlterTest.php
That's how I fixed it here.
Still working on this patch though.

Setting to needs review to see what the testbot says while I continue to work.

Those lines are changing the static $batch variable by reference, so as to account for a potential batch being started by the subsequently following form submission. That's a functional change, out of scope for this issue.

@connork: The changes that you added in #64 are already taken care of in this issue: #2080311: Clean up Drupal\system\Tests\Database\SelectTest.php.

Also, I agree with Sun that the change in #62 should be removed from this patch. $batch is not an unused variable, it's a reference variable.

So I went back to the reroll patch I made, added back in the $batch code and since I went back to the reroll the changes I made having to do with #2080311: Clean up Drupal\system\Tests\Database\SelectTest.php has been reverted as-well.
Now I'll work on the rest of the module.

Check the issue summary under "The following will be handled by separate issues:". A couple other issues take care of some of the other unused variables that may be the same as the ones you are seeing. The reason they are in a separate issue is that those changes fixed bugs and need more thorough review.

I removed some unused variables that look obvious but the rest look as if they may be needed for asserts or tests that haven't been completed.
It's honestly a bit over my head so I think I'll leave it at this.
I'll set it to needs review so that my patches can be tested.

The interdiff removes some unused variables that were added after the original patch was made.

Committed and pushed to 8.x. Thanks!