Unused local variable $key (at line 49) in file.field.inc

Need Review.

@Studiographene

I am not finding any difference between the two patches. Both seems to be equal. See the attached interdiff file

HI #rakesh.gectcr,

According to the Bug i just remove the $key variable from the file file.field.inc ,"Unused local variable $key (at line 70), 'file' module, 'file.field.inc' file."

What else we suppose to do to fixed this Bug,

Thanks
Studiographene

The patch applies correctly. Reviewed.

I'm not sure that this issue meets the guidelines for issue scope. There are plenty of other instances of foreach ($something as $key => $value) { where $key is unused. See https://www.drupal.org/core/scope for guidelines and examples for Drupal core issue scope.

It works fine.

@alexpott: i go through this link, which very nice. but when we see title of this issue it can be meets the guidelines for issue scope. this is also true that , There are plenty of other instances of foreach ($something as $key => $value) { where $key is unused.

Hello @alexpott,

As you said in comment #54 from issue #2002650: [meta, no patch] improve maintainability by removing unused local variables, we should look each unused variable on its own merit. So is this issue in the scope?

If yes, here is a new patch for 8.9.x-dev. The $key variable is really not used in the foreach at line 49.

The $key variable was unused when issue #853926: Wrong order of multi-value file fields when form rebuilt (preview, failed validation) was fixed.

Cheers,
Paulo.

Back to RTBC on this one, now we are considering them case by case this seems ok.

We've avoided doing foreach key fixes in the other issues and I don't think they should be done on their own we can do these as part of the automated fix in #3106216: Remove unused variables from core there's an argument that these variables are not unused - PHPStorm, for example, does not mark them as unused. I think this is the exception to the rule that each one deserves it's own scope because it's not the same as an issue where we have to work why something is longer used.

Applied patch #24 on 9.2.x and it removes the unwanted $key variable from it.

RTBC +1

@Abhijith S, Thank you for your interest in this issue. Adding screenshots of a file with the patch applied does not advance the issue, removing credit. The contributor guide on Drupal.org has information about how to contribute, particularly the process to Review a patch or merge request. There are also guidelines for How is credit granted for Drupal core issues..

Simplified the title, removed quotes.

I have no idea why this is not found with the rule added in #3106216: Remove unused variables from core. I think it probably has something to do with the fact that there are several $key variables in the function but changing those to another variable doesn't seem to change the outcome here.
I'm also not sure if phpcs + that rules is the right tool to fix this issue, I think phpstan would make more sense. In any case, As a part of #2571965: [meta] Fix PHP coding standards in core, I think we should find something that actually finds this unused variable?

Running for 10.1 but don't expect a failure.

If we're going to remove this I think we should take the opportunity to make the code inside more consistent. In the loop we have

    foreach (Element::children($widget) as $sub_key) {
      if (isset($widget[$sub_key]['#type']) && $widget[$sub_key]['#type'] == 'submit') {
        hide($widget[$sub_key]);
        $operations_elements[] = &$widget[$sub_key];
      }
    }

and

    foreach (Element::children($operations_elements) as $key) {
      show($operations_elements[$key]);
    }

I think now we're removing the $key from the containing loop we should be changing the first internal Element::children() to use $key instead of $sub_key because now sub_ is pretty meaningless.

Change parent to the issue implementing the sniff.

Addressed the points mentioned in #38

foreach (Element::children($operations_elements) as $sub_key) {

Will let the committer decide this one but not sure if this should be sub_key but since we used $key in the loop above it maybe it's fine.

No, making this sub key is super confusing, back to needs work imho.

Addressed the points mentioned in #38 in Patch #40, Currently, Parent loops is not using $key So used $key instead of $sub_key and that loop ended in line no 65. IMO line number 91 changes not required as it starts with different loop. Please review.

Sorry wrong interdiff.

Checked the patch given on #43 , it apply's cleanly and have also addressed the previous comments

+RTBC

@aziza_a, adding a screenshot that code applies is not helpful, the bots tell us that. Please don't do that.

The patch in #43 has made the changes I requested in #42, looks good to me now.

In general I don't think we should treat array keys in foreach loops as unused but in this case it's improving readability overall so seems like a good change. Committed/pushed to 10.1.x, thanks!