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

feat: add command settings:publish #80

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 22 additions & 22 deletions phpstan.neon.dist
Original file line number Diff line number Diff line change
@@ -1,22 +1,22 @@
parameters:
tmpDir: build/phpstan
level: 5
paths:
- src/
- tests/
bootstrapFiles:
- vendor/codeigniter4/framework/system/Test/bootstrap.php
excludePaths:
- src/Config/Routes.php
- src/Views/*
ignoreErrors:
universalObjectCratesClasses:
- CodeIgniter\Entity
- CodeIgniter\Entity\Entity
- Faker\Generator
scanDirectories:
- vendor/codeigniter4/framework/system/Helpers
dynamicConstantNames:
- APP_NAMESPACE
- CI_DEBUG
- ENVIRONMENT
parameters:
Copy link
Member

Choose a reason for hiding this comment

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

What was changed here? Tabs to spaces?

Copy link
Member

Choose a reason for hiding this comment

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

@datamweb Please keep this file as close to the DevKit template as possible so updates are easier.

tmpDir: build/phpstan
level: 5
paths:
- src/
- tests/
bootstrapFiles:
- vendor/codeigniter4/framework/system/Test/bootstrap.php
excludePaths:
- src/Config/Routes.php
- src/Views/*
ignoreErrors:
universalObjectCratesClasses:
- CodeIgniter\Entity
- CodeIgniter\Entity\Entity
- Faker\Generator
scanDirectories:
- vendor/codeigniter4/framework/system/Helpers
dynamicConstantNames:
- APP_NAMESPACE
- CI_DEBUG
- ENVIRONMENT
91 changes: 91 additions & 0 deletions src/Commands/SettingsPublisher.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
<?php

namespace CodeIgniter\Settings\Commands;

use CodeIgniter\CLI\BaseCommand;
use CodeIgniter\CLI\CLI;
use CodeIgniter\Publisher\Publisher;
use Throwable;

/**
* Publish Settings config file into the current application.
*/
class SettingsPublisher extends BaseCommand
{
/**
* The group the command is lumped under
* when listing commands.
*
* @var string
*/
protected $group = 'Settings';

/**
* The Command's name
*
* @var string
*/
protected $name = 'settings:publish';

/**
* the Command's usage
*
* @var string
*/
protected $usage = <<<'EOL'
settings:publish [options]

Examples:
settings:publish -f
EOL;

/**
* the Command's short description
*
* @var string
*/
protected $description = 'Publish Settings config file into the current application.';

/**
* The Command's options
*
* @var array<string, string>
*/
protected $options = [
'-f' => 'Set to enable overwrites.',
];

/**
* @var bool `true` to enable overwrites file
*/
private bool $overwrites = false;

public function run(array $params): void
{
if (array_key_exists('f', $params)) {
$this->overwrites = true;
}

// Use the Autoloader to figure out the module path
$source = service('autoloader')->getNamespace('CodeIgniter\\Settings')[0];
$publisher = new Publisher($source, APPPATH);

try {
$publisher->addPath('Config/Settings.php')
->merge($this->overwrites);
Copy link
Member

Choose a reason for hiding this comment

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

If $this->overwrites is only needed in this particular place, then you don't need the property. You can simply use array_key_exists('f', $params) here or use an intermediate variable.

Comment on lines +74 to +75
Copy link
Member

Choose a reason for hiding this comment

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

Can you align ->addPath to the ->merge below?

Suggested change
$publisher->addPath('Config/Settings.php')
->merge($this->overwrites);
$publisher
->addPath('Config/Settings.php')
->merge($this->overwrites);

} catch (Throwable $e) {
$this->showError($e);
}

// If publication succeeded then update file
foreach ($publisher->getPublished() as $file) {
// Replace data in file
$contents = file_get_contents($file);
$contents = str_replace('namespace CodeIgniter\\Settings\\Config', 'namespace Config', $contents);
$contents = str_replace('use CodeIgniter\\Config\\BaseConfig', 'use CodeIgniter\\Settings\\Config\\Settings as SettingsConfig', $contents);
$contents = str_replace('class Settings extends BaseConfig', 'class Settings extends SettingsConfig', $contents);
file_put_contents($file, $contents);
CLI::write(CLI::color(' Published! ', 'green') . "You can customize the configuration by editing the \"{$file}\" file.");
}
}
}
83 changes: 83 additions & 0 deletions tests/Commands/SettingsPublisherTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
<?php

declare(strict_types=1);

namespace Tests\Commands;

use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\Filters\CITestStreamFilter;

/**
* @internal
*/
final class SettingsPublisherTest extends CIUnitTestCase
{
protected function setUp(): void
{
parent::setUp();

CITestStreamFilter::registration();
CITestStreamFilter::addOutputFilter();
CITestStreamFilter::addErrorFilter();

}

protected function tearDown(): void
{
parent::tearDown();

CITestStreamFilter::removeOutputFilter();
CITestStreamFilter::removeErrorFilter();

}

public function testPublishConfigFile(): void
{
command('settings:publish');

$filepath = APPPATH . 'Config/Settings.php';
$this->assertFileExists($filepath);
$this->assertStringContainsString(' Published! ', CITestStreamFilter::$buffer);

$contents = $this->getFileContents($filepath);
$this->assertStringContainsString('namespace Config;', $contents);
$this->assertStringContainsString('use CodeIgniter\\Settings\\Config\\Settings as SettingsConfig;', $contents);
$this->assertStringContainsString('class Settings extends SettingsConfig', $contents);

if (is_file($filepath)) {
copy($filepath, APPPATH . 'Config/Settings.php.bak');
}
Comment on lines +46 to +49
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need this? Since there's no unlinking in the tearDown, this operation would unnecessarily pollute the next test and the filesystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulbalandan In fact, testPublishConfigFileWithForce() depends on this. I wanted to write in the following way, but apparently the notification will be removed in phpunit v10.1:

/**
* @depends testPublishConfigFile
*/
public function testPublishConfigFileWithForce(): void
{
// ...
}

phpunit v10.1:

#[Depends('testPublishConfigFile')]
public function testPublishConfigFileWithForce(): void
{
// ...
}

Copy link
Member

Choose a reason for hiding this comment

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

I think instead of depending on the other test, in the 2nd test you should run the command twice. First without the -f flag asserting the file exists. Then another command run with the -f flag.

}

/**
* @depends testPublishConfigFile
*/
public function testPublishConfigFileWithForce(): void
{
Copy link
Member

Choose a reason for hiding this comment

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

Remove extra whitespaces after { and before }.


$filepath = APPPATH . 'Config/Settings.php';

helper('filesystem');
write_file($filepath, 'fake text.');
$contents = $this->getFileContents($filepath);

$this->assertFileExists($filepath);
$this->assertStringContainsString('fake text.', $contents);

Comment on lines +58 to +66
Copy link
Member

Choose a reason for hiding this comment

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

This part basically tests write_file, not the command.

command('settings:publish -f');

$expectedConfigFile = APPPATH . 'Config/Settings.php.bak';
$this->assertFileEquals($expectedConfigFile, $filepath);

clearstatcache(true, $expectedConfigFile);
if (is_file($expectedConfigFile)) {
unlink($expectedConfigFile);
}
Comment on lines +72 to +75
Copy link
Member

Choose a reason for hiding this comment

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

cleanup should be done in teardown.

Copy link
Member

Choose a reason for hiding this comment

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

If the cleanup is only for the test method, I think it is okay to put it in the test method.
tearDown() is called after each test method.

Copy link
Member

Choose a reason for hiding this comment

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

The advantage of tearDown() is that it will always run regardless of test failures. Especially if there are multiple tests that might leave this file lingering I like the idea of an idempotent cleanup after every test.


}

private function getFileContents(string $filepath): string
{
return (string) @file_get_contents($filepath);
}
}