chrisdicarlo / laravel-config-checker

Checks if configuration values exist by scanning for usage in the codebase.
MIT License
83 stars 4 forks source link

[Bug]: foreach() argument must be of type array|object, int given #4

Closed jonny-dengro closed 2 weeks ago

jonny-dengro commented 2 weeks ago

What happened?

When running php artisan config:check I was presented with the following error:

foreach() argument must be of type array|object, int given

vendor/chrisdicarlo/laravel-config-checker/src/Support/LoadConfigKeys.php:34

This may be localised to our config files however it would make sense to modify the below function since the arguments are not strictly typed:

private function flattenConfig($config, $prefix = '')
{
        if (is_array($config)) {
            // .. existing foreach loop
         }
}

Doing this allowed me to run the package

How to reproduce the bug

Install package and run php artisan config:check

Package Version

^1.0

PHP Version

8.1.12

Laravel Version

10.48.22

Which operating systems does with happen with?

macOS, Linux

Notes

No response

alzander commented 2 weeks ago

I'm experiencing this too. Adding some context as to the cause. I'm pretty sure it's because we have nested folders within the config directory to break things out, so instead of 'config' with a bunch of .php files, we have:

chrisdicarlo commented 2 weeks ago

Thanks for the context, that helps. I don’t nest folders in my config folders but that makes sense to support that scenario. I'll get that fixed up.

chrisdicarlo commented 2 weeks ago

What happened?

When running php artisan config:check I was presented with the following error:

foreach() argument must be of type array|object, int given

vendor/chrisdicarlo/laravel-config-checker/src/Support/LoadConfigKeys.php:34

This may be localised to our config files however it would make sense to modify the below function since the arguments are not strictly typed:

private function flattenConfig($config, $prefix = '')
{
        if (is_array($config)) {
            // .. existing foreach loop
         }
}

Doing this allowed me to run the package

How to reproduce the bug

Install package and run php artisan config:check

Package Version

^1.0

PHP Version

8.1.12

Laravel Version

10.48.22

Which operating systems does with happen with?

macOS, Linux

Notes

No response

@jonny-dengro Can you provide an example of the config file it failed on?

chrisdicarlo commented 2 weeks ago

I've made a fix for this issue. @alzander @jonny-dengro Would you be willing to test it out with your setups? Use dev-allow-nested-config-folders branch.

jonny-dengro commented 2 weeks ago

What happened?

When running php artisan config:check I was presented with the following error:

foreach() argument must be of type array|object, int given

vendor/chrisdicarlo/laravel-config-checker/src/Support/LoadConfigKeys.php:34 This may be localised to our config files however it would make sense to modify the below function since the arguments are not strictly typed:

private function flattenConfig($config, $prefix = '')
{
        if (is_array($config)) {
            // .. existing foreach loop
         }
}

Doing this allowed me to run the package

How to reproduce the bug

Install package and run php artisan config:check

Package Version

^1.0

PHP Version

8.1.12

Laravel Version

10.48.22

Which operating systems does with happen with?

macOS, Linux

Notes

No response

@jonny-dengro Can you provide an example of the config file it failed on?

@chrisdicarlo Turns out the config file that it was failing on is an empty file therefore no array to loop through, My apologies, we have ~50 config files so I must have missed this before opening the bug report.

I've tested the dev-allow-nested-config-folders branch and it still failed with the empty config file however this is probably a good thing as this empty file should have been removed a long time ago! Could be a nice future improvement to support empty files and alert users to this.

Package is working well now - thanks for your help!

chrisdicarlo commented 2 weeks ago

Ah, good - I'll add some tests around empty files to make sure it gets caught in the future. Out of curiosity, was the file completely empty, or was it just returning an empty array?

jonny-dengro commented 2 weeks ago

It was a completely empty file, I've deleted it now but I believe it was returning 1 (int) when I was dumping $config at the beginning of the flattenConfig method.

chrisdicarlo commented 2 weeks ago

Yeah, the assumption (which obviously was incorrect! :laughing: ) was that a config file would, at the very least, return an empty array. I will add a check, though - that's a good idea and helps keep the config directory trim.

jonny-dengro commented 2 weeks ago

😆 I wouldn't have expected an empty file either, looks like it was added to our codebase by a previous developer many moons ago!

alzander commented 2 weeks ago

I just had a chance to try out the branch. Amusingly, it threw the same error at first and then I read the rest of this thread. I have a config file which has return [ null ]; We have some settings that are commented out in that file which we periodically enable and that's apparently how we handle it when we want those settings ignored.

I replaced it with an empty array and the checker started working.

This branch is still need though and does fix/work with nested config files. Testing with 1.0.2 produced a lot of missing config keys whereas with this branch, they were correctly found.

Seems like a great tool.. have a few reports I have to go investigate now :)

chrisdicarlo commented 2 weeks ago

I just had a chance to try out the branch. Amusingly, it threw the same error at first and then I read the rest of this thread. I have a config file which has return [ null ]; We have some settings that are commented out in that file which we periodically enable and that's apparently how we handle it when we want those settings ignored.

I replaced it with an empty array and the checker started working.

This branch is still need though and does fix/work with nested config files. Testing with 1.0.2 produced a lot of missing config keys whereas with this branch, they were correctly found.

Seems like a great tool.. have a few reports I have to go investigate now :)

@alzander I'll add a guard against a basically empty array - good catch!

Glad it's working!