Problem/Motivation

PHPStan has identified possible undefined variables in cases where we include files, in the color module and the transliteration component. This is because the variables are set inside the included files, but PHPStan cannot be sure this is happening.

Steps to reproduce

Proposed resolution

Set default values for the variables before including the file.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3259110-2.patch2.75 KBlongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Active » Needs review
FileSize
3259110-2.patch2.75 KB
mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Reviewed the code and ran PHPStan locally. Fixes the errors and ensures that the variables initiated before including the files.

Not entirely relevant to the issue at hand, but it'd be better if we ran the include in a static closure with the minimum variables passed.

Here's an example from PHPStan itself:

            (static function (string $file) use($container) : void {
                require_once $file;
            })($file);

In these cases we could leverage use to pass items by reference.

daffie’s picture

daffie CreditAttribution: daffie commented

+1 for RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed f29ceb6 and pushed to 10.0.x. Thanks!

+++ b/core/lib/Drupal/Component/Transliteration/PhpTransliteration.php
@@ -283,12 +283,10 @@ protected function readLanguageOverrides($langcode) {
-    if (!isset($overrides) || !is_array($overrides)) {
-      $overrides = [$langcode => []];
-    }

@@ -311,14 +309,10 @@ protected function readGenericData($bank) {
-    if (!isset($base) || !is_array($base)) {
-      $base = [];
-    }
-
-    // Save this data.

There is a subtle behaviour change here. If the file sets $overrides to something other then an array this would result in it being set. However setting it to something other than an array is an error so it's probably better to fail here.

#3 seems worth a follow-up as it'd further insulate from side-effects.

  • alexpott committed f29ceb6 on 10.0.x 
    Issue #3259110 by longwave, mglaman: Fix undefined variables where files...

Status: Fixed » Closed (fixed)

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