Allow grouped config loader to determine adaptor from file extension.

[joeyhub]

Looking at the code for Config\Factory I can see that if it's given a string then it'll attempt to determine the adaptor from the extension. Grouped is able to take a list of filenames as strings and also uses Factory.

However it always converts those strings to arrays using the default adaptor! This means when it's passed down to factory that it doesn't behave quite as you'd expect if you were to imagine the array given the Grouped as an array of input parameters for Factory.

I don't think it's the end of the world but as an aggregate proxy for Factory Grouped is losing something in translation.

Keep in mind that: I haven't tested any of this including the original assertion. I don't really know zephir's syntax (if it even supports continue or === null). This would change the behaviour of the library.

[CameronHall]

Hi @joeyhub,

Thank you for your contribution! It's much appreciated.

Could you please change your target branch to phalcon:4.0.x as we do not accept pull requests directly to master. Please also update CHANGELOG-4.0.md with a brief summary of your patch.

Thanks again!

[codecov]

Codecov Report

Merging #13762 into 4.0.x will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##            4.0.x   #13762      +/-   ##
==========================================
+ Coverage   66.13%   66.13%   +<.01%     
==========================================
  Files         429      429              
  Lines       88899    88906       +7     
==========================================
+ Hits        58790    58800      +10     
+ Misses      30109    30106       -3

Impacted Files	Coverage Δ
ext/phalcon/config/adapter/grouped.zep.c	92.75% <0%> (+5.65%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73aa0d6...feb3f98. Read the comment docs.

[joeyhub]

Maybe I should update the tests too.

[joeyhub]

Hit a wall here because the param isn't nullable (so now I know what ! means, no juggling). The obvious choice is to remove the null check but then the behaviour will change in a way with specific compatibility and usability concerns (it'll ignore the param when users might expect it to take precedence over extension and in that fashion it'll also break compatibility).

Expanding the param to support null would by possible though would then require an is string or is null check. It's also only this one case where I'd want null to be applicable but instead I'd have to consider all the other cases.

A nasty hack would be to instead abuse empty string which otherwise has no meaning which I've done in the mean time.

It really smells to me though like having an extension mapper adapter that would choose which sub adapter to us would do the trick then that could be explicitly specified.

[joeyhub]

I'm a bit stuck on this one as it was something that looked like a simple fix but now has a potential scope blossom with various conflicts between different priorities and principles so some feedback would be good.

[CameronHall]

@joeyhub I'm having trouble understanding what you're trying to achieve.I feel it could be best explained with a test. Would you mind providing a test (independent to the tests already written) to explain the problem you're encountering?

Thanks :)

[niden]

@joeyhub I am with @CameronHall I kinda got lost on what we are trying to achieve here. If not a test maybe some pseudo code in a reply

Thanks!

[joeyhub]

It's a bit of a while back now but basically in pseudo code you have something like this...

Conceptually what you have is like this:

function load(string|array arg)
    if arg is string
        arg = {name: arg, type: arg.split('.').last()}
    do things

function polyload(array args)
    for each args as arg
        load(arg)

In reality you're implementation is like this:

function polyload(array args, type = 'php')
    for each args as arg
        if arg is string
            arg = {name: arg, type}
        load(arg)

Instead of passthru it changes the behaviour. In your implementation you never get arg = {name: arg, type: arg.split('.').last()}. That feature becomes blocked by the aggregations transmutation of parameters to pass.

[joeyhub]

Test cases were already updated. Beyond that, I'm afraid you have to look at the code, to read that and to understand that. I am not a very good translator.

[niden]

Thank you @joeyhub