With config functionality added.

[mfendeksilverstripe]

New feature:

• withConfig - a callback that allows to safely manipulate configuration
• very useful for unit tests which use data providers

Example use (unit test):

/**
 * @dataProvider testValuesProvider
 * @param string $value
 * @param string $expected
 */
public function testConfigValues(string $value, string $expected)
{
    $result = Config::withConfig(function(MutableConfigCollectionInterface $config) use ($value): string {
        // update your config
        $config->set(MyService::class, 'some_setting', $value);

        // your test code goes here
        return MyService::singleton()->executeSomeFunction();
    });

    $this->assertEquals($expected, $result);
}

public function testValuesProvider(): array
{
    return [
        ['test value 1', 'expected value 1'],
        ['test value 2', 'expected value 2'],
        ['test value 3', 'expected value 3'],
    ];
}

[michalkleiner]

Could this be using return call_user_func($callback, $config) to support more ways of providing a valid callback, e.g. an array of class & method pair?

[robbieaverill]

Just a stylistic comment. I'm not entirely convinced that this feature is worth having in core, to be honest, but happy to leave it open to see what the others in the core team think =) Thanks for the PR!

[robbieaverill]

Thoughts on $callback($config) instead?

[michalkleiner]

It used to be that at the beginning of the PR and I suggested to use the call_user_func notation (as above). Then I checked the $callback variable support in the docs and found it's just fine.
Sorry for the hassle @mfendeksilverstripe.

[mfendeksilverstripe]

Thanks for feedback. It's now back to $callback($config)

[dnsl48]

Looks good to me.
I think that's worth having in terms of the config API around nesting.

I believe this wrapper should also be considered to be a best practice and that we should recommend its use over nesting configs manually. Some other languages even have built-in syntax sugar specifically for with context implementations.

[robbieaverill]

I believe this wrapper should also be considered to be a best practice and that we should recommend its use over nesting configs manually

Yeah, the issue I have with this PR is that the config manifest automatically nests itself when you modify it, and SapphireTest automatically nests and unnests it for each test, so if we're talking about unit testing context I don't really see why this is useful. If you're doing more than one thing in a unit test I can see this being useful, but that is probably something we should discourage anyway.

[dnsl48]

SapphireTest automatically nests and unnests it for each test

Agree, for that particular use case it may not be really necessary. Though, that's rather specific for SapphireTest deriving functional tests. When I think about the Config class APIs in a more general way, then I see the value in having the method for covering some low-level edgy use cases.

[robbieaverill]

Ok, I'm convinced 😄one piece of feedback

[mfendeksilverstripe]

@robbieaverill Updated based on your suggestions.

[robbieaverill]

LGTM. Anyone from @silverstripe/core-team want to take quick look, since we're introducing a new API?

[ScopeyNZ]

Agree, for that particular use case it may not be really necessary. Though, that's rather specific for SapphireTest deriving functional tests. When I think about the Config class APIs in a more general way, then I see the value in having the method for covering some low-level edgy use cases.

I personally think that if you're modifying config outside of a testing environment then it's not really config. Given the automated nesting for functional tests - I'm not really sure of its use.

In saying that I don't have any strong opposing opinions to this API and it seems like a good implementation.

[maxime-rainville]

Can't say I ever recall coming across a situation where this would have been helpful. But I don't feel like it adds any significant complexity, so I'm OK with it being merged in.

[mfendeksilverstripe]

@maxime-rainville We found this useful for several unit tests, but I think there are some cases outside of unit tests too. For example this one:

https://github.com/symbiote/silverstripe-queuedjobs/blob/master/src/Services/QueuedJobService.php#L749

In this instance not using callback makes writing unit tests for this code quite challenging. As unit test may effect each other.

btw we also have another callback function which we use in unit tests:

public static function withEnvironment(array $values, callable $callback)
{
    $originalValues = [];
    foreach ($values as $key => $value) {
        if (strlen($key) === 0) {
            continue;
        }

        $originalValues[$key] = Environment::getEnv($key);
    }

    try {
        foreach ($values as $key => $value) {
            if (strlen($key) === 0) {
                continue;
            }

            Environment::setEnv($key, $value);
        }

        return $callback();
    } finally {
        foreach ($originalValues as $key => $value) {
            if (strlen($key) === 0) {
                continue;
            }

            Environment::setEnv($key, $value);
        }
    }
}

You can take this one too :D

Such callback functions are really handy in unit tests if you use dataProviders

[maxime-rainville]

Can we add a bit of doc for this new feature.

I'm thinking this page seems like the natural place to add it https://docs.silverstripe.org/en/4/developer_guides/configuration/configuration/

[mfendeksilverstripe]

@maxime-rainville I added an example use to the pull request description as I don't have access to update the docs.

[kinglozzer]

@mfendeksilverstripe The docs are pulled from the docs/ directory in this repo, so you can add to the docs by editing this file as part of this PR: https://github.com/silverstripe/silverstripe-framework/blob/4/docs/en/02_Developer_Guides/04_Configuration/00_Configuration.md

[mfendeksilverstripe]

@maxime-rainville Readme updated.

[maxime-rainville]

I've suggested some minor rewording to make some sentences a bit simpler. Apply at your discretion.

I'm happy for this to be merge.

[mfendeksilverstripe]

@maxime-rainville All good, thanks :)

[maxime-rainville]

I'll merge on green.

[chillu]

Thanks for documenting this so well @mfendeksilverstripe, great addition! :)