saloonphp / saloon

🤠 Build beautiful API integrations and SDKs with Saloon
https://docs.saloon.dev
MIT License
2.09k stars 107 forks source link

Xdebug has detected a possible infinite loop #448

Open ajithMagic opened 3 months ago

ajithMagic commented 3 months ago

Hi, I'm using Saloon v3.10.0 and saloon/laravel-plugin v3.5.0 running on a Laravel project v10.48.19. Was using this package for very long time and I never had an issue like this before.

Today, while I was trying to generate a connector/request I got the following error. Xdebug has detected a possible infinite loop, and aborted your script with a stack depth of '512' frames.

ajithMagic commented 3 months ago

I have googled and found there was an issue with the Laravel framework itself on the above issue https://github.com/laravel/framework/issues/49968. But it was fixed on the next release.

So I have gone through the saloon/laravel-plugin package to understand how this works. This is the function where infinite loop error hits. https://github.com/laravel/framework/blob/05a9554ac0c7361504a54e350787aba84d028ce7/src/Illuminate/Console/GeneratorCommand.php#L204

specifically this line. https://github.com/laravel/framework/blob/05a9554ac0c7361504a54e350787aba84d028ce7/src/Illuminate/Console/GeneratorCommand.php#L216

Since that is at the laravel level, I looked into the plugin itself and found the function,https://github.com/saloonphp/laravel-plugin/blob/15be0d587e61f11076f93d6df30561b88ff837c1/src/Console/Commands/MakeCommand.php#L58 is causing the issue.

Removing the $rootNamespace from this line https://github.com/saloonphp/laravel-plugin/blob/15be0d587e61f11076f93d6df30561b88ff837c1/src/Console/Commands/MakeCommand.php#L62 has fixed the issue.

ajithMagic commented 3 months ago

I'm happy to make a PR for the above but I'm not certain that is the right function and correct refactoring to fix the issue. I'm happy to provide further details if required and assist on this.

juse-less commented 3 months ago

@ajithMagic Hey.

It'd be good to know why Laravel can't find the class. They call the qualifyClass() method that you link to in your 2nd link. If it can't resolve the class name, it'll call itself again, passing the result of the getDefaultNamespace(). Like your 4th link shows, Saloon is overwriting that method. Whatever Saloon returns is then not found by qualifyClass(), which is then causing qualifyClass() to again call itself with the result of getDefaultNamespace(), that, again, Saloon has overwritten. Rinse and repeat.

Even if removing the $rootNamespace variable seemingly solves it, it'd be good to know why. Like.. what's Saloon actually returning? Is it possible that some backslash replacement and/or concatenation goes wrong? Is it possible Saloon need to handle values from the config file differently? Like.. ensuring the namespace value starts and/or ends with backslash.

It's a bit inconvenient for me to test right now, as I'm on my phone. But I hope it was somewhat helpful. 🙂

ajithMagic commented 3 months ago

Hi @juse-less based, based on your comment I have looked into the code again specifically MakeCommand class. Found the function is not stripping the App from the string.

protected function getNamespaceFromIntegrationsPath(): string
    {
        $namespace = (array)str_replace(['\\App', '\\app'], '', str_replace('/', '\\', str_replace(base_path(), '', config('saloon.integrations_path'))));

        return $namespace[0];
    }

Specifically str_replace(['App', 'app'], '', ...) this piece of code. Because the rest of the str_replace function returns the string "App\Http\Integrations" which doesn't have \ in front of it.

juse-less commented 3 months ago

@ajithMagic Hmm. I wonder if it'd be better to replace that specific str_replace() with a preg_replace() instead. Maybe something like

$namespace = preg_replace('/^\\?app/i', '', ...);

return $namespace;

Just be aware that this is a breaking change, as we're matching from the start, rather than any part of the namespace. I haven't tested the regexp, so it's possible there's a typo. It'd be good with a second opinion about a possible solution, though.

ajithMagic commented 3 months ago

Agreed :+1: