Bug #104509
closedoverwriting $icons variable in Icons.php leads to an error
100%
Description
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;
};
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.
Updated by Jannis Bell 4 months ago
- Description updated (diff)
Found this other version being used while further research
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
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
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
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
Updated by Georg Ringer 4 months ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset 70a6aee7ffb8593baa160ce02c682706f0208b3b.
Updated by Jannis Bell 3 months ago
- Related to Task #104588: overwriting certain $variables in included (require) files causes Problems added