Icons.php error: watchout

[jannis-diggi]

The error was very annoying to debug.
Feel free to change up formatting and specific text.
I included the error message, since it seems common enough, that someone could google this

[jannis-diggi]

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

happens since Icons.php was introduced in 11.5. 13.1 has the same Code so happens there too.
Problem is the direct inclusion of the file, which can then overwrite variables in the function.
fix: extract the inclusion of the file in a separate function so the variables cant be changed.
or just make a better structure than including a php file.

[garvinhicking]

Thanks for the contribution! Which $icon variable do you refer to? The displayed code doesn't have this, it uses a return statement without such a reference?

[jannis-diggi]

@garvinhicking
The $icons variable is not in the Code from the example. But I rewrote the Code from the example like this:

$icons = [
//    'tx-ext-content-something' => 'EXT:ext/Resources/Public/BE/Skin/Icons/Content/something.svg',
//   + 50 more lines
];
$result = [];
foreach ($icons as $name => $path) {
    $result[$name] = [
        'provider' => str_ends_with($path, '.svg') ? SvgIconProvider::class : BitmapIconProvider::class,
        'source' => $path
    ];
}

return $result;

And was very surprised and upset about that error

[linawolf]

I don't really get your example yet. What kind of code caused what kind of error?

[garvinhicking]

Icons.php is meant to only really return code, similar to TCA. As with all "custom" PHP files that are included by TYPO3 you should always use a closure/anonymous function to ensure locally scoped variables; no file should ever modify any variables that are not local. I believe our docs do have this general warning at some places.

I would be fine with adding a warning/disclaimer at this place like:

"As with all .php files getting included by TYPO3, you should use a closure/anonymous function to return data. Never modify any variables (like $icons) which may be part of the global scope that includes these files."

And then we should link to an example with a return call_user_func(function() { return [...]; }); code?

I think we should avoid a very specific example like this and be more general into informing developers that possibly "global" variables shall not be touched?

[jannis-diggi]

I cant describe it more clearly, my bad

   public static function configureIcons(ContainerInterface $container, \ArrayObject $icons, ?string $path = null): \ArrayObject
    { // Core/AbstractServiceProvider.php Line 167
        $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 here
            //$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));
                // array()->exchangeArray() will lead to an error
            }
        }
        return $icons;
    }

[jannis-diggi]

@garvinhicking Your suggestion sounds good.

[jannis-diggi]

do I have to apply these changes, or can you make the changes?

[garvinhicking]

If you could do that, it would save me some time, and would be awesome! Maybe you could find a place in the docs where we explain that anonymous function usage (probably somewhere where ext_localconf.php and ext_tables.php or TCA includes get described). Then we could add the reference easily.

Currently on a train with limited ability to look this up. Many thanks!

[jannis-diggi]

Sry, I couldnt find a page where the anonymous function usage is explained.
The best I found was this SO Post
https://stackoverflow.com/questions/49514481/is-it-better-to-use-call-user-func-in-e-g-tt-content-php-tca-and-why-typo

[garvinhicking]

I think the best place would be this one:

https://docs.typo3.org/m/typo3/reference-coreapi/main/en-us/ExtensionArchitecture/BestPractises/ConfigurationFiles.html

But we would need to create a specific section for that because I forgot that ext_tables/ext_localconf actually does NOT use return statements of course. So we need to:

• Document that ext_tables/localconf should use a call_user_func to locally scope variables
• Then add a section that applies to Icons.php, ContentSecurityPolicies.php, RequestMiddlewares.php, Extbase/Persistence/Classes.php - all these use a "return" statement and could possible affect "local" variables from outside

Then we can link to this section.

What do you think, do you want to have a try to enhance it like this? That would be great, otherwise I can try to do it in the next days :-)

[jannis-diggi]

Im at work currently. When im home later I will leave a bad draft for you to review :)

[garvinhicking]

No worries, I really appreciate your help. :)

[brotkrueml]

(removed)

[jannis-diggi]

draft: notizen:

I searched (cmd-shift-f) for 'require ' in my Typo12.4 Installations vendor/typo3

what: where (in Typo12.4): Error?

ExpressionLanguage.php: ProviderConfigurationLoader L52: $provider (array())
RequestMiddlewares.php: AbstractServiceProvider L72: $middlewares (ArrayObject)
Routes.php: AbstractServiceProvider L91: $path (string), $packageName (string), $routes (ArrayObject)
AjaxRoutes.php: AbstractServiceProvider L101: $path (string), $packageName (string), $routes (ArrayObject)
Modules.php: AbstractServiceProvider L150: $path (string), $packageName (string), $modules (ArrayObject)
ContentSecurityPolicies: AbstractServiceProvider L174: $packageName (string), $mutations (ArrayObject)
Icons.php: AbstractServiceProvider L191: $icons (ArrayObject)
Classes.php: ClassesConfigurationFactory L52: $classes (array())
ext_localconf.php: ExtensionManagementUtility L1328: not wrapped, but no local $vars changeable
TCA/: ExtensionManagementUtility L1460: ok
ext_tables.php: ExtensionManagementUtility L1562: not wrapped, but no local $vars changeable
TCA/: LoadTcaService - Used during Install? L66: $file (name/string), $tcaConfigurationDirectory (string), $activePackages (PackageInterface[]), $backup (array()?)
ext_tables.php: LoadTcaService - Used during Install? L120: $backup (array()?)
ext_localconf.php: UpgradeController L1234: not wrapped, but no local $vars changeable
ext_tables.php: UpgradeController L1246: not wrapped, but no local $vars changeable

I excluded code parts that sounded like Cache
I excluded a part called SiteConfiguration as I dont know what that was
I excluded a part in the MatcherFactory that is active during install that loads 'configuration's and I didnt find out which

'require_once ' yielded
Cache and Configuration/Backend/T3editor/Modes.php and Addons.php

'include ' yielded
ext_emconf.php from EmConfUtility, LanguagePackService, PackageArtifactBuilder, PackageManager
-> dont use $package, $title, $path, $activeLanguages, $key, $extensions, $_EXTKEY, $packageKey
PackageStates.php which is internal, so fine
a PackageArtifacts (File) which is probably internal and fine
and a weird codesnippets.php

[jannis-diggi]

@garvinhicking
sry for the delay, i was focussed on other stuff
I think this is enough of a very bad draft to leave this to you.
I wanted to render and test this, but i cant get gitpod running.

[garvinhicking]

Thank you! I'll have a loom, but probably only next week after my vacation 😇

[garvinhicking]

Sorry it has taken me quite long to get back to this. I wonder, since the TYPO3 core actually merged changes that localize the require/included files, do you think documenting this would still be useful? I believe it was only implemented to v13 and upwards, not sure it was backported to v12.