Remove unused variables from core

This is safe to remove. This test was refactored in #2895271: Convert web tests to JTB and KTB tests for user module part-3 but the now-unused private function was not spotted at the time.

+++ b/core/modules/user/src/Plugin/migrate/process/d6/ProfileFieldOptionTranslation.php
@@ -29,7 +29,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
         }

This is often done intentionally for readbility. Let's remove it from this patch and open a coding standards issue to discuss if we want to.

As side note, Drupal 9.0.x cannot be tested with PHP 7.2, since it requires PHP 7.3.

drupal/core 9.0.x-dev requires php >=7.3 -> your PHP version (7.2.20) does not satisfy that requirement.

Never mind, I misread the code: $account->access('create') is verifying the currently logged-in user is allowed to create $account, which is exactly what it's supposed to do.

I apologize.

thanks @Hardik_Patel_12 for the patch. Reviewed the patch, applied successfully.

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 start is the child issues of #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard.

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.

As unused variables can be detecting via static analysis of the code potentially we could use https://github.com/sirbrillig/phpcs-variable-analysis + PHPCS to automatically check for unused variables and ensure that core doesn't have any. Doing this will be a bit of a big task because they are quite few patterns Drupal adopts that makes this tricky - for example hooks - even if the argument is not used it is best if it is declared on the hook implementation.

If someone want to take on the task of adding the above library as a dev dependency and configuring and getting it to run on core that'd be great.

I'm turning this issue into a plan as doing this requires a few steps.

1. We need to improve the upstream library - we need to address https://github.com/sirbrillig/phpcs-variable-analysis/issues/126
2. We need to open an issue in the coder module to add that library as a dependency and replace coder's unused variable rule with the one from the library.
3. We then need to get a release of coder and update core to use that release.
4. We then need an issue to configure the rule in core/phpcs.xml.dist and fix it in core.

If someone can update the issue summary with these steps and link this issue in the parent and open the issue for coder that'd be a great start. Also if someone can work on the upstream bugfix that'd be awesome.

I've done step one and improved the upstream library - see https://github.com/sirbrillig/phpcs-variable-analysis/pull/128.

Now we need to do step 2.

I opened #3119378: Use PHP_CodeSniffer VariableAnalysis for static analysis, replace Coder unused variable rule with sirbrillig/phpcs-variable-analysis, but do not enable it, as described in comment #11.

Since unused variables are dead code, this could probably be backported to the patch release branch. (Assuming they're actually unused and we don't introduce regressions.)

I actually consider unused variable removal to be a bit different from most codestyle cleanups, because they can point to other bugs. I usually like to use git blame to figure out why the unused variable is there in the first place. And of course there's a minimum requirement of verifying the variable is actually unused, which has to be done on a case-by-case basis for each variable.

@xjm Do you mean a patch can be provided to remove the unused variables?

#3119378: Use PHP_CodeSniffer VariableAnalysis for static analysis, replace Coder unused variable rule with sirbrillig/phpcs-variable-analysis, but do not enable it has now updated coder to use the third party library - so we could start experimenting here by upgrading coder to the dev version and enabling the rule in core/phpcs.xml.dist

minimum requirement of verifying the variable is actually unused, which has to be done on a case-by-case basis for each variable.

Tools can determine where a variable is actually used. We've now update the tool used for this in coder so now we can try and apply it to core.

Here's a start of all non-test unused variables being fixed. Unfortunately I've identified another thing to fix - https://github.com/sirbrillig/phpcs-variable-analysis/issues/183 - before we can adopted this for core. As this is a coding standard change moving to the dev branch for now. It's likely we'll backport some of this to make cross branch patches simpler.

I think we can fix tests in a follow-up issue.

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -143,7 +143,7 @@ public static function allowedIfHasPermissions(AccountInterface $account, array
   -        if (!$permission_access = $account->hasPermission($permission)) {
   +        if (!$account->hasPermission($permission)) {
   
   @@ -151,7 +151,7 @@ public static function allowedIfHasPermissions(AccountInterface $account, array
   -        if ($permission_access = $account->hasPermission($permission)) {
   +        if ($account->hasPermission($permission)) {
   

   Changes like this are nice because assignments in ifs are head-scratchers.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -768,7 +768,7 @@ protected function setDefaultLangcode() {
   -    foreach ($this->fields as $name => $items) {
   +    foreach ($this->fields as $items) {
   

   Originally I was going to file an issue against the github project because an unused key in a foreach doesn't feel like an unused variable. But the more conversions I did the more convinced I became the removing them is correct. It adds to things for a developer to hold in their head whilst reviewing the code. And reducing this load is a good thing.

3. +++ b/core/lib/Drupal/Core/Entity/EntityConstraintViolationList.php
   @@ -106,7 +106,7 @@ public function getByField($field_name) {
   -    foreach (array_intersect_key($this->violationOffsetsByField, array_flip($field_names)) as $field_name => $offsets) {
   +    foreach (array_intersect_key($this->violationOffsetsByField, array_flip($field_names)) as $offsets) {
   

   Also some long lines get a little bit shorter which is also good from a cognitive load perspective.

So next steps here are:

• to fix https://github.com/sirbrillig/phpcs-variable-analysis/issues/183
• get a release of coder

I've rerolled this and fixed a few more instances along with ignoring all the transliteration data as they are include files.

I also ignored DrupalPractice.CodeAnalysis.VariableAnalysis.UndefinedUnsetVariable as it generated a false positive for the unset() in:

    $build['action_table'] = parent::render();
    if (!$this->hasConfigurableActions) {
      unset($build['action_table']['table']['#header']['operations']);
    }

There are three false positives remaining with this patch:

FILE: core/themes/bartik/color/color.inc
----------------------------------------------------------------------------------------------------
 8 | WARNING | Unused variable $info.
   |         | (DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable)
----------------------------------------------------------------------------------------------------


FILE: core/modules/field_ui/src/Element/FieldUiTable.php
-------------------------------------------------------------------------------------------------------
 73 | WARNING | Unused variable $target.
    |         | (DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable)
-------------------------------------------------------------------------------------------------------


FILE: /core/lib/Drupal/Core/Form/FormErrorHandler.php
-------------------------------------------------------------------------------------------------------------------------------------------------
 113 | WARNING | Redeclaration of function parameter $elements as variable.
     |         | (DrupalPractice.CodeAnalysis.VariableAnalysis.VariableRedeclaration)
-------------------------------------------------------------------------------------------------------------------------------------------------

color.inc is an include file, maybe we just have to explicitly ignore these. FieldUiTable looks like valid code to me and I think there must be still bugs in the analyser around references:

          // Add the element in the tree.
          $target = &$trees[$region_name][''];
          foreach ($parents[$name] as $key) {
            $target = &$target['children'][$key];
          }
          $target['children'][$name] = ['name' => $name, 'weight' => $row['weight']['#value']];

FormErrorHandler is also deliberate, perhaps we also have to ignore DrupalPractice.CodeAnalysis.VariableAnalysis.VariableRedeclaration.

While there is some value here I can't help but think there are too many false positives with this method. I think exploring more advanced static analysis tools like PHPStan or Psalm is probably a better option.

The patch seems not to pass a PHPCS check.

Redeclaration of function parameter $elements as variable (core/lib/Drupal/Core/Form/FormErrorHandler.php, line 113)

// phpcs:ignore DrupalPractice.CodeAnalysis.VariableAnalysis.VariableRedeclaration should take care of the above.

Teaching cspell about the English language...

I apologize: Comment #25 made clear that was a false positive. There isn't anything to do to fix that, as it's a false positive.

Reverted erroneous change.

I apologize: Comment #25 made clear that was a false positive. There isn't anything to do to fix that, as it's a false positive.

If that's the case, I would expect 2 more false positives after I've made the first one ignored.

Anyway, I've managed to get a few issues with the patch out of the way. I'm leaving this to await the return of @longwave now.

Thanks for picking this up, fixed the remaining issues (some need ignoring) and also fixed up the list() syntax as we don't need trailing commas.

Patch #32, Working fine and Remove the unused variables from core
Adding after patch screenshot for reference.

Thanks @BhumikaVarshney for checking, but the fact that TestBot returns with a green, all test passed message already confirmed that.

Moving 3106216-32.patch into a MR, for easier (at least for me... 😇) reviewing and commenting.

Since I myself created a patch for this issue I can't RTBC it, but some extra eyes never hurts?

1 Remark about an out of scope issue, and 3 about something we _might_ wanna do.

Not bad :)

Great to see that we can remove so many unused variables! Really great. Makes core easier to understand!

Merged latest commits from 9.4.x-dev into MR.

All the code changes look good to me.
The sniff gets activated and the testbot goes green.
For me it is RTBC.

Committed 2f1c716 and pushed to 10.0.x and 9.4.x. Thanks!

Changing the parent issue, because the fixes here relate to a DrupalPractice sniff not a Drupal coding standard sniff

Changing parent again to a new parent for the relevant sniff.