Problem/Motivation

Within theme_system_modules_details() a nested foreach loop re-assigns the parent foreach loops iterator ($key).

  // Iterate through all the modules, which are children of this element.
  foreach (Element::children($form) as $key) {
    ....
    ...
    foreach (array('help', 'permissions', 'configure') as $key) {
      $links .= drupal_render($module['links'][$key]);
    }
}

since $key is not used below that point, there isn't any current breakage. However, this is fragile, and should code later need that key below, would cause a serious DX issue.

Proposed resolution

Use a different variable name.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug
Unfrozen changes Unfrozen because it only changes an obvious PHP logic error which also exists in Drupal 7
Prioritized changes The main goal of this issue is DX and it reduces fragility.
CommentFileSizeAuthor
#12 key-2342243-6-D7-backport.patch796 bytesserundeputy
#6 key-2342243-6.patch665 bytesmartin107
#2 key-9519161-2.patch649 bytesmartin107
key-0.patch651 bytesmartin107

Comments

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch no longer applies.

martin107’s picture

martin107 commented
Title: With nested foreach loop you cannot re-use the same variable! » Within a nested foreach loop you cannot re-use the index variable!
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new key-9519161-2.patch649 bytes

Fixed up my own text, and rerolled.

Should come back green.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

This simple patch fixes an obvious logic error (even though it hasn't thus far caused problems since the $key variable isn't used again after being redefined).

I've updated the issue summary to include a beta phase evaluation.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/system/system.admin.inc
@@ -249,8 +249,8 @@ function theme_system_modules_details($variables) {
-    foreach (array('help', 'permissions', 'configure') as $key) {
-      $links .= drupal_render($module['links'][$key]);
+    foreach (array('help', 'permissions', 'configure') as $i) {
+      $links .= drupal_render($module['links'][$i]);
     }

Can we use a more meaning name that $i?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: +Needs issue summary update

This issue is a normal bug so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

martin107’s picture

martin107 commented
Status: Needs work » Needs review
StatusFileSize
new key-2342243-6.patch665 bytes

While this it minutely limited in scope - it does not match any of the criteria from the beta phase evaluation template.

I am perfectly open to this issue being shunted to a later release..

To be clear this is just a brittleness in the code ... any future modifications to the function that make use of $key below the next foreach loop
are going to lead to confusion as the loop variable has been inappropriately overwritten.

My real concern is that lint checkers reasonably detail this problem in the "probably bug" section of their error report
and for me it seems bad practice to leave any entries, not matter how small, in this section only to be constantly ignored...

Anyway, as for the patch a more reasonable variable name is $link_type

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

I don't know how to be more clear in the beta phase evaluation. I know bug fixes are allowed, and this is a quite obvious bug (even if it has no current implications since the re-assigned value of $key isn't used below that point in the loop).

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update +Needs backport to 7.x

I took a stab at simplifying the issue summary, and I updated the beta phase evaluation to note that this fix reduces fragility (which is allowed).

The patch in #6 should address the concerns in #4.

I also added the backport tag since this issue exists in Drupal 7.

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -Needs backport to 7.x +Needs backport to D7

Committed/pushed to 8.0.x, thanks!

Moving to 7.x for backport.

  • catch committed 28e9d3e on 8.0.x 
    Issue #2342243 by martin107: Within a nested foreach loop you cannot re-...
serundeputy’s picture

serundeputy commented
Assigned: Unassigned » serundeputy

working on D7 backport.

serundeputy’s picture

serundeputy commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new key-2342243-6-D7-backport.patch796 bytes

D7 backport.

martin107’s picture

martin107 commented
Assigned: serundeputy » Unassigned

Yep that is the fix for the equivalent nested for loop in d7, so +1 from me.

Moving to unassigned, to signal it is free to review.

joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

Looks good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: key-2342243-6-D7-backport.patch, failed testing.

Status: Needs work » Needs review

dcam queued 12: key-2342243-6-D7-backport.patch for re-testing.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

Status: Fixed » Closed (fixed)

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