Problem/Motivation

Remove unused variable $field_name from FieldDiscoveryTest.

    foreach ($actual_fields['node'] as $bundle => $fields) {
      foreach ($fields as $field_name => $field_info) {
        $this->assertArrayHasKey('type', $field_info);
        $this->assertCount(22, $field_info);
        $this->assertEquals($bundle, $field_info['type_name']);
      }
    }

Steps to reproduce

Proposed resolution

Remaining tasks

Update patch to make the same change in \Drupal\Tests\migrate_drupal\Kernel\d7\FieldDiscoveryTest
Review
Commit

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 interdiff_2-5.txt739 bytesreenaraghavan
#5 3334910-5.patch1.76 KBreenaraghavan
#2 3334910-2.patch897 bytesdiegors

Issue fork drupal-3334910

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

anmolgoyal74 created an issue. See original summary.

diegors’s picture

diegors
He/Him
Primary language Portuguese, Brazil
Location Campinas - Brazil
commented
Status: Active » Needs review
StatusFileSize
new 3334910-2.patch897 bytes

Did the change, please review.

quietone’s picture

quietone commented
Title: Remove unused variable $field_name from FieldDiscoveryTest » Remove unused variable $field_name from Kernel test FieldDiscoveryTest
Issue summary: View changes
Status: Needs review » Needs work

I looked at the patch to find out which FieldDiscoveryTest is being changed. I originally thought is was \Drupal\Tests\migrate_drupal\Unit\FieldDiscoveryTest but in fact it is one of the two Kernel test. The d6 one. I am now looking at the d7 test to see if it has the same unused variable. Yes, it does. The change was made to both in #2951550: Make service for field discovery for use in migrate entity derivers.

It is the same test file in both versions so this change should be made in both files in one patch.

reenaraghavan made their first commit to this issue’s fork.

reenaraghavan’s picture

reenaraghavan commented
StatusFileSize
new 3334910-5.patch1.76 KB
new interdiff_2-5.txt739 bytes
urvashi_vora’s picture

urvashi_vora commented
Assigned: Unassigned » urvashi_vora
Status: Needs work » Needs review

Setting it to Needs Review as @reenaraghavan provided another patch in #5.

I will review the patch.

Thanks

urvashi_vora’s picture

urvashi_vora commented
Assigned: urvashi_vora » Unassigned
Status: Needs review » Reviewed & tested by the community

I reviewed the patch provided in #5.

It was applied cleanly.

urvasi@urvasi-Inspiron-15-3552:/var/www/html/contribution/drupal-3334910$ git apply -v 3334910-5.patch
Checking patch core/modules/migrate_drupal/tests/src/Kernel/d6/FieldDiscoveryTest.php...
Checking patch core/modules/migrate_drupal/tests/src/Kernel/d7/FieldDiscoveryTest.php...
Applied patch core/modules/migrate_drupal/tests/src/Kernel/d6/FieldDiscoveryTest.php cleanly.
Applied patch core/modules/migrate_drupal/tests/src/Kernel/d7/FieldDiscoveryTest.php cleanly.

Also with respect to comment #3

I looked at the patch to find out which FieldDiscoveryTest is being changed. I originally thought is was \Drupal\Tests\migrate_drupal\Unit\FieldDiscoveryTest but in fact it is one of the two Kernel test. The d6 one. I am now looking at the d7 test to see if it has the same unused variable. Yes, it does. The change was made to both in #2951550: Make service for field discovery for use in migrate entity derivers.

It is the same test file in both versions so this change should be made in both files in one patch.

I can confirm that 3334910-5.patch changes FieldDiscoveryTest.php file in both Kernel tests, i.e for d6 as well as d7. And both file changes are made in one single patch.

To me, it looks good to be committed.

As per my humble opinion, Moving it to RTBC.

Please change the issue status if you find it unappropriate.

Thank You!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Closed (works as designed)

We don't consider the $key in a foreach loop to be unused - neither phpstan nor phpcs will report it as such, so this is 'by design'. If we wanted to change that, it would need to come with a phpcs or phpstan rule and a patch that brings all of core into line with the new rule. Closing this one out.

jonathan1055’s picture

jonathan1055 commented

I agree that this is 'closed works as designed' but just for completeness and accuracy, there is no special handling of the $key of a foreach in the Coder sniff DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable. This sniff is active in core 10.1 and there are no unused $key variables in the main code, they are either used or the $key has been removed, see #3106216: Remove unused variables from core. If you edit a core file which has a foreach and add a $key which is not used, you do get:

--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 2528 | WARNING | Unused variable $jskey.
      |         | (DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable)
--------------------------------------------------------------------------------

However, the reason why the problem in the issue summary was not reported by phpcs is that the high level parent sniff DrupalPractice.CodeAnalysis.VariableAnalysis is excluded from running against any */tests/* path. See core/phpcs.xml.dist line 148

The issue #3310127: [META] Fix DrupalPractice best practice in Core covers the discussion on how best to include or ignore DrupalPractice sniffs in core