Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CLI specific config file #5581

Merged
merged 5 commits into from
Oct 9, 2023

Conversation

lukaskleinschmidt
Copy link
Contributor

@lukaskleinschmidt lukaskleinschmidt commented Sep 8, 2023

This PR …

Adds the option to define a CLI specific config file config.cli.php. You can already do something like this by using the ready callback and define your CLI specific options in there. Probably only for edge cases but when you need CLI specific options I think this would fit in quite nicely with the current system of environment (host name or ip address) specific options.

// config.php
return [
    'ready' => function (App $kirby) {
        $options = [
            //
        ];

        if ($kirby->environment()->cli()) {
            $options = array_merge($options, [
                'option.one' => 1,
                'option.two' => 2,
            ]);
        }

        return $options;
    }
];

// config.cli.php
return [
    'option.one' => 1,
    'option.two' => 2,
];

Related: https://discord.com/channels/525634039965679616/525641819854471168/1149657559746224188

Breaking changes

Hopefully none.

Ready?

  • Unit tests for fixed bug/feature
  • In-code documentation (wherever needed)
  • Tests and checks all pass

For review team

@distantnative distantnative requested review from a team and removed request for bastianallgeier September 23, 2023 15:25
Copy link
Member

@distantnative distantnative left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like the idea.

Any thoughts if this could cause problems elsewhere, @bastianallgeier @lukasbestle?

src/Http/Environment.php Outdated Show resolved Hide resolved
@distantnative distantnative added the type: enhancement ✨ Suggests an enhancement; improves Kirby label Sep 23, 2023
@distantnative distantnative added this to the 4.0.0 milestone Sep 23, 2023
@lukasbestle
Copy link
Member

In theory it could clash with a host named cli, but that's very unlikely in practice and will only take effect if the host cli was explicitly allowlisted in the main config.php.

So I'd say it should be safe.

@distantnative
Copy link
Member

@lukaskleinschmidt Would you mind adding unit tests for this? Once with cli active and extra config loading, once with inactive and ignoring the cli config file? You can force the cli state for testing as done here https://github.com/getkirby/kirby/blob/develop/tests/Http/EnvironmentTest.php#L223-L231

@distantnative distantnative added the needs: tests 🧪 Requires missing tests label Sep 24, 2023
@lukaskleinschmidt
Copy link
Contributor Author

I will try to get to it. But currently too busy

@bastianallgeier
Copy link
Member

I really like it as well!

@lukaskleinschmidt
Copy link
Contributor Author

I had the same issue locally regarding the failed test https://github.com/getkirby/kirby/actions/runs/6429036429/job/17457332224?pr=5581 but wanted to make sure this is not related to my setup.

Changing the test case for the failing test would fix the issue. But not sure if this is the proper way to do this here.

public function testOptionsFromInvalidHost()
{
	$env = new Environment([
		'cli' => false, // <- this one here
		'allowed' => [
			'http://example.de'
		]
	], [
		'SERVER_NAME' => 'example.com'
	]);

	$this->assertSame([], $env->options($this->config));
}

@lukasbestle
Copy link
Member

lukasbestle commented Oct 6, 2023

@lukaskleinschmidt I think that would be the correct solution. The test assumes an HTTP request with the SERVER_NAME environment variable, so we need to fake CLI mode off for the test to make sense.

@lukasbestle lukasbestle removed the needs: tests 🧪 Requires missing tests label Oct 7, 2023
@lukasbestle lukasbestle modified the milestones: 4.0.0, 3.9.8 Oct 7, 2023
@bastianallgeier bastianallgeier merged commit b30822c into getkirby:develop Oct 9, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants