codeigniter4
Bug: BaseConfig issue on registrar
PHP Version
8.2
CodeIgniter4 Version
4.5.5
CodeIgniter4 Installation Method
Manual (zip or tar.gz)
Which operating systems have you tested for this bug?
Windows, Linux
Which server did you use?
apache
Database
MySQL
What happened?
New registrar with the same method with other registrars replaces their contents on array_merge which works on only first level array. I hope array_merge_recursive will be better for modules
Steps to Reproduce
\Codeigniter\Config\BaseConfig
/**
* Provides external libraries a simple way to register one or more
* options into a config file.
*
* @return void
*
* @throws ReflectionException
*/
protected function registerProperties()
{
if (! static::$moduleConfig->shouldDiscover('registrars')) {
return;
}
if (! static::$didDiscovery) {
$locator = service('locator');
$registrarsFiles = $locator->search('Config/Registrar.php');
foreach ($registrarsFiles as $file) {
$className = $locator->findQualifiedNameFromPath($file);
if ($className === false) {
continue;
}
static::$registrars[] = new $className();
}
static::$didDiscovery = true;
}
$shortName = (new ReflectionClass($this))->getShortName();
// Check the registrar class for a method named after this class' shortName
foreach (static::$registrars as $callable) {
// ignore non-applicable registrars
if (! method_exists($callable, $shortName)) {
continue; // @codeCoverageIgnore
}
$properties = $callable::$shortName();
if (! is_array($properties)) {
throw new RuntimeException('Registrars must return an array of properties and their values.');
}
foreach ($properties as $property => $value) {
if (isset($this->{$property}) && is_array($this->{$property}) && is_array($value)) {
$this->{$property} = array_merge($this->{$property}, $value);
} else {
$this->{$property} = $value;
}
}
}
}
Expected Output
Expected merge the old contents array with the new one
Anything else?
Yes i used array_merge_recursive. I discovered the issue to so i extend the base config to child class today fix this but i think the usage of array_merge_recursive is a better option help on this to update the core code
\Codeigniter\Config\BaseConfig
/**
* Provides external libraries a simple way to register one or more
* options into a config file.
*
* @return void
*
* @throws ReflectionException
*/
protected function registerProperties()
{
if (! static::$moduleConfig->shouldDiscover('registrars')) {
return;
}
if (! static::$didDiscovery) {
$locator = service('locator');
$registrarsFiles = $locator->search('Config/Registrar.php');
foreach ($registrarsFiles as $file) {
$className = $locator->findQualifiedNameFromPath($file);
if ($className === false) {
continue;
}
static::$registrars[] = new $className();
}
static::$didDiscovery = true;
}
$shortName = (new ReflectionClass($this))->getShortName();
// Check the registrar class for a method named after this class' shortName
foreach (static::$registrars as $callable) {
// ignore non-applicable registrars
if (! method_exists($callable, $shortName)) {
continue; // @codeCoverageIgnore
}
$properties = $callable::$shortName();
if (! is_array($properties)) {
throw new RuntimeException('Registrars must return an array of properties and their values.');
}
foreach ($properties as $property => $value) {
if (isset($this->{$property}) && is_array($this->{$property}) && is_array($value)) {
$this->{$property} = array_merge_recursive($this->{$property}, $value);
} else {
$this->{$property} = $value;
}
}
}
}
Please give us an example of the problem: a config file and a registrar.
Are you defining the same registrar in many modules?
class MyModuleRegistrar {
public static function MyModule() {
return [
'database' => 'mysql',
'cache' => ['enabled' => true, 'driver' => 'file'],
];
}
}
The problem is visible on the Filters. Try disabling only the Toolbar or adding filters to the previous config. They will be overwritten from the module
I will assume this is the proper example:
<?php
namespace Config;
use CodeIgniter\Config\BaseConfig;
class Example extends BaseConfig
{
public array $arrayNested = [
'key1' => 'val1',
'key2' => ['val2' => 'subVal2', 'val3' => 'subVal3'],
];
}
<?php
namespace Modules\MyModule\Config;
class Registrar
{
public static function Example(): array
{
return [
'arrayNested' => [
'key2' => ['val4' => 'subVal4']
],
];
}
}
Now, you will get this:
public 'arrayNested' -> array (2) [
'key1' => string (4) "val1"
'key2' => array (1) [
'val4' => string (7) "subVal4"
]
]
But you expect this:
public 'arrayNested' -> array (2) [
'key1' => string (4) "val1"
'key2' => array (3) [
'val2' => string (7) "subVal2"
'val3' => string (7) "subVal3"
'val4' => string (7) "subVal4"
]
]
This is not a bug, but a feature request. Personally, I'm not a fan of this change.
You can avoid this problem very simply - by defining the structure of the config file differently.
how? let's say I initially want to disable the toolbar filter in app/Registrar.php, and add middlewares in other modules modules/auth, modules/job? Some add data, others replace it. the .env file will not work. It is not rational to create another config class - then why use a standard interceptor?
If we start using array_merge_recursive, we will introduce a BC break.
If modules add new options, they can use the Publisher class to add them to the main config.
I could be wrong, but I think the intention was to prevent everything from being easily overwritten from within modules, as there should be a "single source of truth" for some options.
Yes, this is BC. Therefore, another solution is needed. How about passing the current configuration properties to the Config?
// in registerProperties()
static::$registrars[] = new $className();
//...
// in foreach()
$callable->__parentProps = get_class_vars($this);
// in Registrars, delete some keys
$parentProps = $this->__parentProps;
unset($parentProps['key1']);
return [
'arrayNested' => [
'key2' => array_merge_recursive($parentProps, ['val4' => 'subVal4'])
],
];
This is about the order of work, code is inaccurate
Certainly! Let's explore this issue in detail, starting with examples of the problem.
Example of the Problem: Config File and Registrar
In a typical modular CodeIgniter 4 application, you might have several independent modules, each with its own Registrar.php file. These registrars are meant to register specific configuration settings, such as filters, routes, or other properties, into a central configuration system.
Example Configuration Files
-
Blog Module Registrar:
// Modules/Blog/Config/Registrar.php namespace Modules\Blog\Config; class Registrar { public function filters() { return [ 'aliases' => [ 'blogFilter' => \Modules\Blog\Filters\BlogFilter::class, ], 'globals' => [ 'before' => ['blogFilter'], ], ]; } } -
Shop Module Registrar:
// Modules/Shop/Config/Registrar.php namespace Modules\Shop\Config; class Registrar { public function filters() { return [ 'aliases' => [ 'shopFilter' => \Modules\Shop\Filters\ShopFilter::class, ], 'globals' => [ 'before' => ['shopFilter'], ], ]; } } -
Paypal Module Registrar:
// Modules/Paypal/Config/Registrar.php namespace Modules\Paypal\Config; class Registrar { public function filters() { return [ 'aliases' => [ 'paypalFilter' => \Modules\Paypal\Filters\PaypalFilter::class, ], 'globals' => [ 'before' => ['paypalFilter'], ], ]; } }
These files define filters that are supposed to be registered into the main configuration. The expectation is that all these configurations should be combined, so the application uses filters from all modules.
The Problem: Overwriting Instead of Merging
The issue arises due to the way the registerProperties method handles the merging of configuration arrays. The current implementation uses array_merge, which works fine for single-dimensional arrays but doesn't perform a deep merge. This means that when you have nested arrays, the nested arrays from one module can overwrite those of another module instead of merging them.
For example, if the filters method is defined similarly in multiple modules, the last one processed will overwrite the previous ones rather than integrating all of them.
Are You Defining the Same Registrar in Many Modules?
Yes, in this context, the same registrar method (filters) is defined in multiple modules. Each module has its own implementation of the filters method within its Registrar.php file. The intention is for each module to contribute its filters to the central configuration.
Solution: Deep Merging
To solve this issue, a deep merge function is needed. This function should recursively merge arrays to ensure that all configurations are retained and integrated properly. Here's a simple example of how I implement a deep merge function in PHP:
function deep_merge(array &$array1, array &$array2)
{
$merged = $array1;
foreach ($array2 as $key => &$value) {
if (is_array($value) && isset($merged[$key]) && is_array($merged[$key])) {
$merged[$key] = deep_merge($merged[$key], $value);
} else {
$merged[$key] = $value;
}
}
return $merged;
}
Applying the Solution
In the registerProperties method, I replace the array_merge operation with this deep_merge function to ensure that all configurations from different modules are properly merged.
Solution Output:
After applying the deepMerge function, the combined configuration will look like this:
// Combined configuration
$combinedFilters = [
'aliases' => [
'blogFilter' => \Modules\Blog\Filters\BlogFilter::class,
'shopFilter' => \Modules\Shop\Filters\ShopFilter::class,
'paypalFilter' => \Modules\Paypal\Filters\PaypalFilter::class,
],
'globals' => [
'before' => ['blogFilter', 'shopFilter', 'paypalFilter'],
],
];
Explanation of the Output
-
Aliases: The
aliasesarray now contains all filter aliases from the Blog, Shop, and Paypal modules, ensuring that each module's filter is registered. -
Globals: The
globalsarray for thebeforekey contains an array with all filter names, ensuring that all specified filters are applied before global processing.
This method guarantees that all configuration data is retained, allowing each module to seamlessly contribute to the overall application setup. By using a deep merge function, the solution ensures that nested arrays are combined effectively, thereby preserving the unique configurations intended by each module's developer. This approach respects the configurations from all modules and integrates them cohesively, maintaining the integrity of the application's intended settings.
I think filter aliases can be configured in module, but all filters should be configured in app/Config/Filters.php for security.
What about the appendBeforeProps(), appendAfterProps(), replaceProps() methods Registrar? Should I use the current (old $this) copy of the config inside?
I think filter aliases can be configured in module, but all filters should be configured in
app/Config/Filters.phpfor security.
What of the case the registrar it was stated in the doc that this should be better option
What about the
appendBeforeProps(), appendAfterProps(), replaceProps()methodsBaseConfig? Should I use the current (old$this) copy of the config inside?
Explain your question better
This is a solution option. New methods will add or replace Config properties.
// merge recursive
// 'aliases' => ['blogFilter' => BlogFilter::class, 'paypalFilter' => PaypalFilter::class]
'aliases' => $this->appendAfterProps([
'paypalFilter' => \Modules\Paypal\Filters\PaypalFilter::class,
])
// merge recursive
// 'aliases' => ['paypalFilter' => PaypalFilter::class, 'blogFilter' => BlogFilter::class]
'aliases' => $this->appendBeforeProps([
'paypalFilter' => \Modules\Paypal\Filters\PaypalFilter::class,
])
// full replace array
// 'aliases' => ['paypalFilter' => PaypalFilter::class]
'aliases' => $this->replaceProps([
'paypalFilter' => \Modules\Paypal\Filters\PaypalFilter::class,
])
Let's get back to the question.
I think filter aliases can be configured in module, but all filters should be configured in app/Config/Filters.php for security.
In this case, there is already a security hole - the module can disable all filters.
// Registrar
public static function Filters(): array {
return [
'required' => [
'before' => [],
'after' => ['csrf'],
],
];
}
// before
public required -> array (2) [
'before' => array (2) [
0 => string (10) "forcehttps"
1 => string (9) "pagecache"
]
'after' => array (3) [
0 => string (9) "pagecache"
1 => string (11) "performance"
2 => string (7) "toolbar"
]
]
// after
public required -> array (2) [
'before' => array (0) []
'after' => array (1) [
0 => string (4) "csrf"
]
]
Can we think about how to solve the issue painlessly? Add an option to the Feature class?
Any news on this issue? Just ran into it as i was trying to add ci-shield permissions for my module like this:
public static function AuthGroups(): array
{
return [
'permissions' => [
'shippinglabel-logos.access' => 'Can access Shipping Label Logos',
'shippinglabel-logos.create' => 'Can create Shipping Label Logos',
'shippinglabel-logos.edit' => 'Can edit Shipping Label Logos',
'shippinglabel-logos.delete' => 'Can delete Shipping Label Logos',
],
'matrix' => [
'superadmin' => [
'shippinglabel-logos.*',
],
],
];
}
And after merging, 'shippinglabel-logos.*' is the only permission left for usergroup 'superadmin' which is very irritating.
Any news on this issue?
There haven’t been any updates so far. Unless more maintainers express agreement on introducing such changes, we won’t be moving forward at this time.
IMHO, permission definitions should remain centralized in a single main file for security and maintainability reasons.
However, if you're confident in what you're doing, you can simply use ReflectionClass in your Registrar.php file to merge the base superadmin permission array with additional items.