Clean up Drupal\system\Tests\Database\SelectTest.php

Closing the following as duplicates:

#2080113: Remove Unused local variable $plugin_id from /core/modules/system/lib/Drupal/system/Tests/Plugin/CacheDecoratorLanguageTest.php
#2080115: Remove Unused local variable $cached from /core/modules/system/lib/Drupal/system/Tests/Cache/GenericCacheBackendUnitTestBase.php
#2080117: Add assertion to Drupal/system/Tests/Database/DeleteTruncateTest.php
#2080119: Remove Unused local variable $result from /core/modules/system/lib/Drupal/system/Tests/Database/InsertDefaultsTest.php
#2080121: Remove Unused local variable $ringo from /core/modules/system/lib/Drupal/system/Tests/Database/DatabaseTestBase.php
#2080317: Remove Unused local variable $task_field from /core/modules/system/lib/Drupal/system/Tests/Database/SelectComplexTest.php
#2080289: Remove Unused local variable $name_field from /core/modules/system/lib/Drupal/system/Tests/Database/SelectOrderedTest.php
#2080291: Remove Unused local variable $john from /core/modules/system/lib/Drupal/system/Tests/Database/CaseSensitivityTest.php
#2080295: Remove Unused local variable $ret from /core/modules/system/lib/Drupal/system/Tests/Database/SchemaTest.php
#2080299: Remove Unused local variable $record from /core/tests/Drupal/Tests/Core/Database/EmptyStatementTest.php
#2080303: Remove Unused local variable $log from /core/modules/system/lib/Drupal/system/Tests/Database/LoggingTest.php
#2080305: Remove Unused local variable $records from /core/modules/system/lib/Drupal/system/Tests/Database/FetchTest.php
#2080309: Remove Unused local variable $record from /core/modules/system/lib/Drupal/system/Tests/Database/AlterTest.php

Noticed some code that is using a while or for loop:
Example One

diff --git a/core/modules/system/lib/Drupal/system/Tests/Database/AlterTest.php b/core/modules/system/lib/Drupal/system/Tests/Database/AlterTest.php
index a5d08e8..70ac5bc 100644
--- a/core/modules/system/lib/Drupal/system/Tests/Database/AlterTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Database/AlterTest.php
@@ -34,7 +34,7 @@ function testSimpleAlter() {
     $result = $query->execute();
 
     $num_records = 0;
-    foreach ($result as $record) {
+    while ($result) {
       $num_records++;
     }

Example two

diff --git a/core/modules/system/lib/Drupal/system/Tests/Database/SelectComplexTest.php b/core/modules/system/lib/Drupal/system/Tests/Database/SelectComplexTest.php
index a997a34..e9ba813 100644
--- a/core/modules/system/lib/Drupal/system/Tests/Database/SelectComplexTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SelectComplexTest.php
@@ -154,13 +150,11 @@ function testGroupByAndHaving() {
    */
   function testRange() {
     $query = db_select('test');
-    $name_field = $query->addField('test', 'name');
-    $age_field = $query->addField('test', 'age', 'age');
     $query->range(0, 2);
     $result = $query->execute();
 
     $num_records = 0;
-    foreach ($result as $record) {
+    for ($i=0; $i < count($result); $i++) {
       $num_records++;
     }

to perform what the count function does. Updated patch with count function.

#5: 2080311-remove-unused-local-variables-5.patch queued for re-testing.

Why did you lump all these separate issues together like this? This greatly complicates the review process.

Although sometimes joining issues is effective, the issues closed as duplicates shouldn't have been closed for the following reasons:
0. It is easier for new people to start contributing to core with simple tasks which include changing one file and a few lines.
1. It's much easier to review them.
2. Sometimes small changes can lead to bigger complications, for example the code might now need test (see #2072597: Remove Unused local variables from tests in the Views module).

This has been discussed with YesCT at the core sprint at DrupalCon Prague.

For these reasons I'll return the issues to their previous status.
This issue should be done only regarding the file it concerns.

Addressing #10. Patch addressing issue #2080311 separately.

The unused $record variables were not yet removed. Removed them in this patch.

#12: 2080311-remove-unused-local-12.patch queued for re-testing.

The patch from #12 looks good; it keeps the tests while removing the unused variables and effectively gets rid of useless while loops. RTBC.

This, however, removes the actual counting of records and relies on rowCount working properly. Do we test that?

maybe we should use an approach like in #5: count($result) ?

Sounds a plan to me. You could do even do both the rowCount and the count() to test rowCount() while at it.

How's this?

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

Actually this looks like another one that could stand to have its own separate review. Thanks!

Tested via Netbeans. Currently there are no unused variables at Drupal\system\Tests\Database\SelectTest.php So closing this issue.

Sorry, made a mistake there and so reopening the issue.

As per #10, I have fixed only unused variables in Drupal\system\Tests\Database\SelectTest.php
As per #16, I did not replace the code with rowCount function to get number of records returned by the query.

ie. Did not replace the following code with rowCount function

$num_records = 0;
foreach ($result as $record) {
  $num_records++;
}

I've changed "while" cycles to the count and added "fetchAll". First I've planned to use rowCount() but according to this issue: #2146733: Select queries should not use rowCount() to calculate number of rows it shouldn't be used for "Select" queries.

The patch no longer applies as the file core/modules/system/lib/Drupal/system/Tests/Database/SelectTest.php does not exist there.

Re-rolled patch per new drupal/core/modules/system/src/Tests/Database/SelectTest.php path.

#30 looks good to me.

+++ b/core/modules/system/src/Tests/Database/SelectTest.php
@@ -43,19 +38,16 @@ function testSimpleSelect() {
+    $num_records = $result->fetchAll();

@@ -64,19 +56,16 @@ function testSimpleComment() {
+    $num_records = $result->fetchAll();

$num_records is now an weird name $records would be better.

Please review updated patch with fixes in #33.

This patch still applies.

Looks nice!

Committed 7fe5f2a and pushed to 8.x. Thanks!