codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.3k stars 1.9k forks source link

[Registrar] Duplicate entries leads to duplicate config properties which leads to duplicated items in the debug toolbar. #9069

Closed Zoly closed 1 month ago

Zoly commented 1 month ago

PHP Version

7.4

CodeIgniter4 Version

4.4.8

CodeIgniter4 Installation Method

Composer (as dependency to an existing project)

Which operating systems have you tested for this bug?

Windows

Which server did you use?

apache

Database

MySQLi 8.2.0

What happened?

This all started with the fact that on my end the debug toolbar has duplicate items for more then on composer package I installed. After digging for two days in the code I arrived at the source of the problem, that during the registration process the newly associated classes make recursive calls and ends up registering the packages multiple times in the registrar, thus when the debug toolbar is generate it's elements the toolbar items are duplicated.

During the debugging process I made the registrar unique, but that lead to breaking the rendering process. Later I discovered that the duplicated registrar leads to duplicated config properties, but only in case the property is an index based array. To merge the data array_merge() is used, with behaves differently for indexed and associative array respectively:

If the input arrays have the same string keys, then the later value for that key will overwrite the previous one. If, however, the arrays contain numeric keys, the later value will not overwrite the original value, but will be appended. Values in the input array with numeric keys will be renumbered with incrementing keys starting from zero in the result array.

The registrar is duplicated, and that leads to some of the config values to be duplicated as well, including the collectors property used by the debug toolbar.

When I tried to make the config property generation unique, the rendering process broke again. At this point I've given up on fixing the toolbar problem from the roots and just did a dirty patch in the toolbar service making the collectors property there unique with good results.

In conclusion, the registrar process needs to be revisited by brighter minds then mine, because there are two implementations that are expecting different things from the same property. The rendering process uses the duplicated items as a map to how many times and in witch order to apply the rendering, meanwhile the toolbar expects to see a unique list of registered packages to be processed.

Steps to Reproduce

Just the basic installation and some packages to populate the config like:

simpletine/codeigniter4-hmvc datamweb/shield-oauth lonnieezell/bonfire

Expected Output

Components referring to the same package to appear only once on the toolbar, and at the same time a way for pages to still render properly (ex: in Bonfire 2 -> my account ->save button).

Anything else?

composer.json

{
    "name": "simpletine/codeigniter4-hmvc",
    "description": "CodeIgniter4 HMVC Architecture",
    "license": "MIT",
    "type": "project",
    "homepage": "https://simpletine.com",
    "require": {
        "php": "^7.4 || ^8.0",
        "codeigniter4/framework": "^4.0",
        "datamweb/shield-oauth": "dev-develop",
        "lonnieezell/bonfire": "dev-develop",
        "ext-json": "*"
    },
    "require-dev": {
        "fakerphp/faker": "^1.23.1",
        "mikey179/vfsstream": "^1.6",
        "phpstan/phpstan": "2.0.x-dev",
        "phpunit/phpunit": "^9.1"
    },
    "autoload": {
        "exclude-from-classmap": [
            "**/Database/Migrations/**"
        ]
    },
    "config": {
        "optimize-autoloader": true,
        "preferred-install": "dist",
        "sort-packages": true
    },
    "scripts": {
        "test": "phpunit",
        "phpstan": "phpstan analyse -c phpstan.neon"
    },
    "minimum-stability": "dev"
}

image

kenjis commented 1 month ago

I was able to reproduce with CI 4.5.3. Screenshot 2024-07-25 11 10 06

vendor/codeigniter4/shield/src/Config/Registrar.php

    public static function Toolbar(): array
    {
        return [
            'collectors' => [
                Auth::class,
            ],
        ];
    }

vendor/tatter/alerts/src/Config/Registrar.php

    public static function Toolbar(): array
    {
        return [
            'collectors' => [
                Alerts::class,
            ],
        ];
    }
$ ./spark config:check Toolbar

CodeIgniter v4.5.3 Command Line Tool - Server Time: 2024-07-25 02:23:45 UTC+00:00

Config\Toolbar#54 (7) (
    public 'collectors' -> array (11) [
        0 => string (43) "CodeIgniter\Debug\Toolbar\Collectors\Timers"
        1 => string (45) "CodeIgniter\Debug\Toolbar\Collectors\Database"
        2 => string (41) "CodeIgniter\Debug\Toolbar\Collectors\Logs"
        3 => string (42) "CodeIgniter\Debug\Toolbar\Collectors\Views"
        4 => string (42) "CodeIgniter\Debug\Toolbar\Collectors\Files"
        5 => string (43) "CodeIgniter\Debug\Toolbar\Collectors\Routes"
        6 => string (43) "CodeIgniter\Debug\Toolbar\Collectors\Events"
        7 => string (31) "Tatter\Alerts\Collectors\Alerts"
        8 => string (34) "CodeIgniter\Shield\Collectors\Auth"
        9 => string (31) "Tatter\Alerts\Collectors\Alerts"
        10 => string (34) "CodeIgniter\Shield\Collectors\Auth"
    ]
    public 'collectVarData' -> boolean true
    public 'maxHistory' -> integer 20
    public 'viewsPath' -> string (97) "/Users/kenji/work/codeigniter/tmp/ci453/vendor/codeigniter4/framework/system/Debug/Toolbar/Views/"
    public 'maxQueries' -> integer 100
    public 'watchedDirectories' -> array (1) [
        0 => string (3) "app"
    ]
    public 'watchedExtensions' -> array (7) [
        0 => string (3) "php"
        1 => string (3) "css"
        2 => string (2) "js"
        3 => string (4) "html"
        4 => string (3) "svg"
        5 => string (4) "json"
        6 => string (3) "env"
    ]
)

Config Caching: Disabled
kenjis commented 1 month ago

This is because of a dirty hack in Bonfire2. Bonfire\Config\Registrar runs Bonfire\Config\Registrar::registerNamespaces(). https://github.com/lonnieezell/Bonfire2/blob/b892532257ee49604075596b32881767634a978e/src/Config/Registrar.php#L160-L163

First of all, BaseConfig::registerProperties() (Auto-Discovery of Registrars) must run only once before any Config class instantiation.

But Bonfire\Config\Registrar::registerNamespaces() https://github.com/lonnieezell/Bonfire2/blob/b892532257ee49604075596b32881767634a978e/src/Config/Registrar.php#L106 calls config('Bonfire'). https://github.com/lonnieezell/Bonfire2/blob/b892532257ee49604075596b32881767634a978e/src/Config/Registrar.php#L126C28-L126C45 Therefore during Bonfire\Config\Registrar loading, it instantiates Bonfire\Config\Bonfire and it calls BaseConfig::registerProperties() (Auto-Discovery of Registrars) again because registerProperties() (Auto-Discovery of Registrars) is not completed.

kenjis commented 1 month ago

I don't think this is a bug in the framework. This is a bug or misuse in Bonfire.

neznaika0 commented 1 month ago

need a test on a clean app installation.

Zoly commented 1 month ago

Thanks for clearing things up for me and making ci more stable and reliable for us to use.

Zoly commented 1 month ago

Added a bug report in Bonfire 2 https://github.com/lonnieezell/Bonfire2/issues/464#issue-2430638724