Bug: Registrar is messed up having duplicated items because a recursive call

[Zoly]

CI Registrar is made more consistent in a future release and will probably brake Bonfire 2 if this is not fixed.

I was able to reproduce with CI 4.5.3.

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

Originally posted by @kenjis in https://github.com/codeigniter4/CodeIgniter4/issues/9069#issuecomment-2249226767

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.

Originally posted by @kenjis in https://github.com/codeigniter4/CodeIgniter4/issues/9069#issuecomment-2249269595

[Zoly]

A quick patch fix could be:

Index: vendor/lonnieezell/bonfire/src/Config/Registrar.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vendor/lonnieezell/bonfire/src/Config/Registrar.php b/vendor/lonnieezell/bonfire/src/Config/Registrar.php
--- a/vendor/lonnieezell/bonfire/src/Config/Registrar.php   
+++ b/vendor/lonnieezell/bonfire/src/Config/Registrar.php   (date 1721951376946)
@@ -122,8 +122,29 @@
             $namespaces["Bonfire\\{{$name}}"] = [realpath(__DIR__ . "/../{$name}")];
         }

-        // Now define app modules nemespaces
-        $appModulesPaths = config('Bonfire')->appModules;
+        // Now define app modules namespaces
+        // but instantiated classes can't be used in registrar
+        $appModulesPaths = null;
+
+        // Locate the config file
+        $locator = service('locator');
+
+        if (isset($locator) && $file = $locator->locateFile('Bonfire', 'Config')) {
+            /** @var object $className */
+            $className = $locator->findQualifiedNameFromPath($file);
+
+            // Get the config class without instantiation
+            try {
+                $configBonfire = (new ReflectionClass($className))->newInstanceWithoutConstructor();
+            } catch (\ReflectionException $e) {
+                $configBonfire = null;
+            }
+
+            // Retrieve the property
+            if (isset($configBonfire)) {
+                $appModulesPaths = $configBonfire->appModules;
+            }
+        }

         if (is_array($appModulesPaths) && $appModulesPaths !== []) {
             foreach ($appModulesPaths as $baseName => $path) {

[Zoly]

Must be noted that if a fix is applied to the Registrar in any way it will break the rendering of save buttons in the admin pages (ex: my account), at least it does for me.

[kenjis]

Why do you need to run Registrar::registerNamespaces(); in Config/Registrar.php? Can't you move it to somewhere? E.g., Event?

[Zoly]

Hi @kenjis, I hear you loud and clear.

I'm just a system administrator who started a project migration from ci3 to ci4 on behalf of a friend and run in some issues during the process while at it.

Sure, I can make a quick patch to keep the ship from sinking, but I don't have the toolset nor the overview of the whole project to make decisions for such changes. This is why the code is in the comments and not as a PR ( I tried that route and didn't and up well :) )

Hope someone can solve the problem the right way, but it's a working solution and can be used for a last resort or quick fix to keep the wheels rolling, and for now this is the best I can offer.

[kenjis]

@Zoly Okay, you don't need to send a PR to fix this issue when you are not familiar with Bonfire.

[Zoly]

I may not be great at it, but I still hope to contribute with my two cents a little. If you find any value in my codes feel free to use them any way you like, even if it's just for inspiration.

That being said, this is how I solved the button rendering problems in my Bonfire:

Index: vendor/lonnieezell/bonfire/src/View/ComponentRenderer.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vendor/lonnieezell/bonfire/src/View/ComponentRenderer.php b/vendor/lonnieezell/bonfire/src/View/ComponentRenderer.php
--- a/vendor/lonnieezell/bonfire/src/View/ComponentRenderer.php 
+++ b/vendor/lonnieezell/bonfire/src/View/ComponentRenderer.php (date 1722064297548)
@@ -103,7 +109,14 @@
     private function renderPairedTags(string $output): string
     {
         //        ini_set("pcre.backtrack_limit", "-1");
-        $pattern = '/<\s*x[-\:](?<name>[\w\-\:\.]*?)(?<attributes>[\s\S\=\'\"]*)[^>]?>(?<slot>.*)<\/\s*x-\1\s*>/uiUsm';
+        $pattern = '/(?(DEFINE)(?<marker>\s*?x-))
+                        <\g<marker>(?<name>\w[\w\-\:\.]+[^\>\/\s\/])
+                                   (?<attributes>[\s\S\=\'\"]+?)??>
+                            (?(?<!\/>) # Not pired so ignore the rest
+                                (?<slot>.*?)??
+                        <\/\g<marker>\k<name>\s*>
+                            )
+                    /uismx';

         /*
             $matches[0] = full tags matched and all of its content
@@ -111,16 +124,25 @@
             $matches[attributes] = string of tag attributes (class="foo")
             $matches[slot] = the content inside the tags
          */
-        return preg_replace_callback($pattern, function ($match) {
-            $view               = $this->locateView($match['name']);
-            $attributes         = $this->parseAttributes($match['attributes']);
-            $attributes['slot'] = $match['slot'];
-            $component          = $this->factory($match['name'], $view);
+
+        do {
+            try {
+                $output = preg_replace_callback($pattern, function ($match) {
+                    $view = $this->locateView($match['name']);
+                    $attributes = $this->parseAttributes($match['attributes']);
+                    $attributes['slot'] = $match['slot'];
+                    $component = $this->factory($match['name'], $view);

-            return $component instanceof Component
-                ? $component->withView($view)->withData($attributes)->render()
-                : $this->renderView($view, $attributes);
-        }, $output) ?? preg_last_error();
+                    return $component instanceof Component
+                        ? $component->withView($view)->withData($attributes)->render()
+                        : $this->renderView($view, $attributes);
+                }, $output, -1, $replaceCount);
+            } catch (\Throwable $e) {
+                break;
+            }
+        } while (!empty($replaceCount));
+
+        return $output ?? preg_last_error();
     }

     /**

I had to rewrite the regex a little. Here's the link if you want to take a deeper look: Regex demo

We all hope to see the Bonfire grow by putting one log at a time, gather around it and have fun together.

[michalsn]

The fix for Registrars has been merged into the 4.6 branch, so when a new minor version is released, it will cause some problems for Bonfire.

The module loading part should be refactored.

[lonnieezell]

I'll take a look at this as soon as I can, but I'm in the middle of a big freelance project outside of work so may be a couple of weeks unfortunately.