Project

General

Profile

Actions

Bug #104509

closed

overwriting $icons variable in Icons.php leads to an error

Added by Jannis Bell 4 months ago. Updated about 1 month ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
System/Bootstrap/Configuration
Target version:
-
Start date:
2024-07-31
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
11
PHP Version:
Tags:
Complexity:
easy
Is Regression:
Sprint Focus:
Needs Decision

Description

[[https://github.com/TYPO3/typo3/blob/main/typo3/sysext/core/Classes/Package/AbstractServiceProvider.php#L167]]

public static function configureIcons(ContainerInterface $container, \ArrayObject $icons, ?string $path = null): \ArrayObject
{
    $path = $path ?? static::getPackagePath();
    $iconsFileNameForPackage = $path . 'Configuration/Icons.php';
    if (file_exists($iconsFileNameForPackage)) {
        $definedIconsInPackage = require $iconsFileNameForPackage;//<-- Here the Icons.php is called
        //Code from Icons.php is executed
        //$icons = somethingThatsNotAnArrayObject

        if (is_array($definedIconsInPackage)) {
            //Now $icons is probably not an ArrayObject anymore, and $icons->exchangeArray() is called
            $icons->exchangeArray(array_merge($icons->getArrayCopy(), $definedIconsInPackage));
            // e.g. array()->exchangeArray() will lead to an error
        }
    }
    return $icons;
}

This code from the AbstractServiceProvider, with my comments should display how the bug comes to life.
It is in the Code since 11.5 as far as I have seen.
Can you please wrap the inclusion of the Icons.php in a new function?

public static function requirePHPFile(string $filepath): mixed 
{
    //this protects the local variables from being changed accidentally by another user/developer
    return require $filepath;
}

I just found this version being used:
[[https://github.com/TYPO3/typo3/blob/13.1/typo3/sysext/core/Classes/Configuration/Tca/TcaFactory.php#L109]]

// To require TCA in a safe scoped environment avoiding local variable clashes.
// Note: Return type 'mixed' is intended, otherwise broken TCA files with missing "return [];" statement would
//       emit a "return value must be of type array, int returned" PHP TypeError. This is mitigated by an array
//       check below.
$scopedReturnRequire = static function (string $filename): mixed {
    return require $filename;
};


Related issues 1 (1 open0 closed)

Related to TYPO3 Core - Task #104588: overwriting certain $variables in included (require) files causes ProblemsNew2024-08-10

Actions
Actions #1

Updated by Garvin Hicking 4 months ago

  • Category set to System/Bootstrap/Configuration
  • Sprint Focus set to Needs Decision

IMO I would rather see extension developers not overwriting local variables, and always use advocated anonmyous functions/enclosures.

Moving this in the core would mean it's even outside readability scope for the required files, which may be a valid use case.

Actions #2

Updated by Jannis Bell 4 months ago

  • Description updated (diff)

Found this other version being used while further research

Actions #3

Updated by Gerrit Code Review 4 months ago

  • Status changed from New to Under Review

Patch set 1 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/85501

Actions #4

Updated by Gerrit Code Review 4 months ago

Patch set 2 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/85501

Actions #5

Updated by Gerrit Code Review 4 months ago

Patch set 3 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/85501

Actions #6

Updated by Gerrit Code Review 4 months ago

Patch set 1 for branch 12.4 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/85465

Actions #7

Updated by Georg Ringer 4 months ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
Actions #8

Updated by Jannis Bell 4 months ago

  • Related to Task #104588: overwriting certain $variables in included (require) files causes Problems added
Actions #9

Updated by Benni Mack about 1 month ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF