My head and my lint checker are spinning at the repeated redefinition of the $module variable in update.inc as an incorrectly duplicate loop variable.

Formally the lint checker reports

The 'module' variable used as a key foreach loop is already used in the same way by the outer foreach loop.

the nested loops are defined on line 603 ( outer )
foreach (\Drupal::keyValue('system.schema')->getAll() as $module => $schema) {

and 622 ( inner )
foreach ($result as $module => $module_data) {

the problem comes seeing at a glance just which $module is intended here.

if (!isset($return[$module][$update][$module_dependency]) || $update_dependency > $return[$module][$update][$module_dependency]) {
$return[$module][$update][$module_dependency] = $update_dependency;

CommentFileSizeAuthor
#4 interdiff-1-4.txt3.1 KBmartin107
#4 module-2342247-4.patch2.23 KBmartin107
#1 module-2342247-1.patch2.29 KBmartin107

Comments

martin107’s picture

martin107 commented
Issue summary: View changes
StatusFileSize
new module-2342247-1.patch2.29 KB

Ok so not that difficult to unpick.

I have taken the liberty of fixing problems with the annotation blocks I had to read to grok what was going on.

martin107’s picture

martin107 commented
Issue summary: View changes
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work 
  1. +++ b/core/includes/module.inc
@@ -94,7 +94,7 @@ function system_register($type, $name, $uri) {
  *
- * @return
+ * @return array
  *   The name of the module's install file, if successful; FALSE otherwise.

    module_load_install() returns either a string or false, since it just returns whatever module_load_include() returns (or FALSE). 

  2. +++ b/core/includes/update.inc
@@ -619,7 +619,7 @@ function update_retrieve_dependencies() {
       if (isset($result) && is_array($result)) {
...
           foreach ($module_data as $update => $update_data) {

    Might as well go for even better readability and change $result to $updated_dependencies and $update to $update_version.

martin107’s picture

martin107 commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new module-2342247-4.patch2.23 KB
new interdiff-1-4.txt3.1 KB

1) Thanks for correcting me ... the more I look at module.inc, the more I see documentation issues ...I will split those changes off into another issue.

2) Yep, those changes make sense to me.

I see you are on a review spree, lots of my issues are being picked up :)

Thank you

Mile23++

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Reviewed & tested by the community

Here's an exercise: Open the Drupal issue list. Show only Needs Review. Click the 'last update' column header until its showing you the oldest issues. Go. :-)

The tests pass, the changes look good. RTBC yay!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Wow, nice catch.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 5b5d403 on 8.0.x 
    Issue #2342247 by martin107, Mile23: Within nested foreach loop you...

Status: Fixed » Closed (fixed)

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