In core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php, testSchemaMapping() initializes $a without using it.

The variable should be removed from testSchemaMapping().

CommentFileSizeAuthor
#2 3156070-2.patch777 byteshardik_patel_12

Comments

Hardik_Patel_12 created an issue. See original summary.

hardik_patel_12’s picture

Status: Active » Needs review
StatusFileSize
new777 bytes
siddhant.bhosale’s picture

Assigned: Unassigned » siddhant.bhosale
siddhant.bhosale’s picture

Assigned: siddhant.bhosale » Unassigned
Status: Needs review » Reviewed & tested by the community

Hi, I have tested the patch and the test runs successfully.

vendor/bin/phpunit -c core/ --filter ConfigSchemaTest
PHPUnit 8.5.5 by Sebastian Bergmann and contributors.

Testing
...............                                                   15 / 15 (100%)

Time: 1.79 minutes, Memory: 833.00 MB

OK (15 tests, 167 assertions)

Looks good to be merged.

alexpott’s picture

Status: Reviewed & tested by the community » Closed (duplicate)
Related issues: +#3106216: Remove unused variables from core

Thank you for your work on cleaning up Drupal core's code style!

In order to fix core coding standards in a maintainable way, all our coding standards issues should be done on a per-rule basis across all of core, rather than fixing standards in individual modules or files. We should also separate fixes where we need to write new documentation from fixes where we need to correct existing standards. This all should be done as part of #2571965: [meta] Fix PHP coding standards in core, stage 1. A good place to for unused variables is #3106216: Remove unused variables from core.

For background information on why we usually will not commit coding standards fixes that aren't scoped in that way, see the core issue scope guidelines, especially the note about coding standards cleanups. That document also includes numerous suggestions for scoping issues including documentation coding standards cleanups.

Contributing to the overall plan above will help ensure that your fixes for core's coding standards remain in core the long term.

avpaderno’s picture

Issue summary: View changes
avpaderno’s picture

Status: Closed (duplicate) » Needs work

I guess this issue should be re-opened in the same way other similar issues have been re-opened.

See the comment left for similar issues, for example #3156040: Avoid initializing a local variable to an empty array before adding items to that array.

This can point to broken code or incomplete testing see #3157369: Use unused variable $filters from DateTimeSchemaTest for example. A useful tool for this is git log -S “SOME TEXT” which will search git commits for matching text to find out when the variable might have become unused. Without doing the work to show why the variable is unused the patch will not be committed. Also git blame can be useful as well.

avpaderno’s picture

Looking at the core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php can also help to understand when that variable was introduced, and how the current code needs to be fixed.

avpaderno’s picture

The $a = \Drupal::config('config_test.dynamic.third_party'); line was added in the patch provided in #2432791: Skip Config::save schema validation of config data for trusted data.

diff --git a/core/modules/config/src/Tests/ConfigSchemaTest.php b/core/modules/config/src/Tests/ConfigSchemaTest.php
index 8a8f649..11865ef 100644
--- a/core/modules/config/src/Tests/ConfigSchemaTest.php
+++ b/core/modules/config/src/Tests/ConfigSchemaTest.php
@@ -209,6 +209,7 @@ function testSchemaMapping() {
 
     $this->assertEqual($definition, $expected, 'Retrieved the right metadata for the first effect of image.style.medium');
 
+    $a = \Drupal::config('config_test.dynamic.third_party');
     $test = \Drupal::service('config.typed')->get('config_test.dynamic.third_party')->get('third_party_settings.config_schema_test');
     $definition = $test->getDataDefinition()->toArray();

It seems it's not anymore necessary, since removing that line doesn't cause any test failure.

avpaderno’s picture

Status: Needs work » Reviewed & tested by the community

I would use a different name for the $test variable, but it would be an off-topic change that doesn't add much.

alexpott’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

This looks some debug I might have added - oops.

Committed and pushed 700b1c92f1 to 9.1.x and b2b3ddbba5 to 9.0.x and 583402844d to 8.9.x. Thanks!

  • alexpott committed 700b1c9 on 9.1.x
    Issue #3156070 by Hardik_Patel_12, kiamlaluno: Unused local variables...

  • alexpott committed b2b3ddb on 9.0.x
    Issue #3156070 by Hardik_Patel_12, kiamlaluno: Unused local variables...

  • alexpott committed 5834028 on 8.9.x
    Issue #3156070 by Hardik_Patel_12, kiamlaluno: Unused local variables...

Status: Fixed » Closed (fixed)

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