Bug: BaseConfig issue on registrar

[ChibuezeAgwuDennis]

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;
                }
            }
        }
    }

[michalsn]

Please give us an example of the problem: a config file and a registrar.

Are you defining the same registrar in many modules?

[PANKAJNAGAR433]

class MyModuleRegistrar {
    public static function MyModule() {
        return [
            'database' => 'mysql',
            'cache' => ['enabled' => true, 'driver' => 'file'],
        ];
    }
}

[neznaika0]

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

[michalsn]

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.

[neznaika0]

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?

[michalsn]

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.

[neznaika0]

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

[ChibuezeAgwuDennis]

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

1. 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'],
               ],
           ];
       }
   }

2. 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'],
               ],
           ];
       }
   }

3. 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 aliases array now contains all filter aliases from the Blog, Shop, and Paypal modules, ensuring that each module's filter is registered.

• Globals: The globals array for the before key 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.

[kenjis]

I think filter aliases can be configured in module, but all filters should be configured in app/Config/Filters.php for security.

[neznaika0]

What about the appendBeforeProps(), appendAfterProps(), replaceProps() methods Registrar? Should I use the current (old $this) copy of the config inside?

[ChibuezeAgwuDennis]

I think filter aliases can be configured in module, but all filters should be configured in app/Config/Filters.php for security.

What of the case the registrar it was stated in the doc that this should be better option

[ChibuezeAgwuDennis]

What about the appendBeforeProps(), appendAfterProps(), replaceProps() methods BaseConfig? Should I use the current (old $this) copy of the config inside?

Explain your question better

[neznaika0]

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,
])