Skip to content
Open
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
18 changes: 17 additions & 1 deletion src/backend/api_calls/mdwiki_sql.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
//---
use PDO;
use PDOException;
use RuntimeException;

class Database
{
Expand Down Expand Up @@ -45,7 +46,22 @@ private function set_db($server_name)
// $ts_pw = posix_getpwuid(posix_getuid());
// $ts_mycnf = parse_ini_file($ts_pw['dir'] . "/confs/db.ini");
// ---
$ts_mycnf = parse_ini_file($this->home_dir . "/confs/db.ini");
$config_path = $this->home_dir . "/confs/db.ini";
$ts_mycnf = @parse_ini_file($config_path);
if ($ts_mycnf === false) {
throw new RuntimeException(sprintf('Database configuration file "%s" could not be read or parsed.', $config_path));
}
Comment on lines +50 to +53

Choose a reason for hiding this comment

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

medium

Using the error suppression operator (@) can hide issues and makes debugging harder. It's better to explicitly check for the file's readability first. This allows for more specific error messages about whether the file is unreadable versus being unparsable.

        if (!is_readable($config_path)) {
            throw new RuntimeException(sprintf('Database configuration file "%s" could not be read.', $config_path));
        }
        $ts_mycnf = parse_ini_file($config_path);
        if ($ts_mycnf === false) {
            throw new RuntimeException(sprintf('Database configuration file "%s" could not be parsed due to syntax errors.', $config_path));
        }


$required_keys = ['user'];
if ($server_name !== 'localhost') {
$required_keys[] = 'password';
}

foreach ($required_keys as $key) {
if (!array_key_exists($key, $ts_mycnf) || $ts_mycnf[$key] === '') {
throw new RuntimeException(sprintf('Database configuration is missing required "%s" entry in "%s".', $key, $config_path));
}
}
// ---
if ($server_name === 'localhost') {
$this->host = 'localhost:3306';
Expand Down
99 changes: 99 additions & 0 deletions tests/DatabaseConfigTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
<?php

declare(strict_types=1);

use APICalls\MdwikiSql\Database;
use FilesystemIterator;
use PHPUnit\Framework\TestCase;
use RecursiveDirectoryIterator;
use RecursiveIteratorIterator;
use RuntimeException;

final class DatabaseConfigTest extends TestCase
{
Comment on lines +12 to +13

Choose a reason for hiding this comment

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

medium

Great job on adding tests for the new error handling! To make the test suite even more comprehensive, consider adding a few more test cases:

  • A test for when the user key is missing from the config file.
  • A test for when a required key (like user or password) exists but its value is an empty string.

Here is an example for a missing user, which also incorporates the suggestion to move cleanup to tearDown():

public function testSetDbThrowsExceptionWhenUserMissing(): void
{
    $this->tempHome = sys_get_temp_dir() . '/dbconfig_test_incomplete_' . uniqid();
    $this->createTempHome($this->tempHome, ['password' => 'secret']); // No 'user' key

    $this->expectException(RuntimeException::class);
    $this->expectExceptionMessageMatches('/missing required "user" entry/');

    new Database('tools');
}

private ?string $originalHome = null;

protected function setUp(): void
{
$this->originalHome = getenv('HOME') !== false ? getenv('HOME') : null;
}

protected function tearDown(): void
{
if ($this->originalHome !== null) {
putenv('HOME=' . $this->originalHome);
} else {
putenv('HOME');
}
}

public function testSetDbThrowsExceptionWhenConfigMissing(): void
{
$tempHome = sys_get_temp_dir() . '/dbconfig_test_missing_' . uniqid();
$this->createTempHome($tempHome);

$this->expectException(RuntimeException::class);
$this->expectExceptionMessage('could not be read or parsed');

try {
new Database('localhost');
} finally {
$this->cleanupTempHome($tempHome);
}
Comment on lines +38 to +42

Choose a reason for hiding this comment

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

medium

The try...finally block for cleanup can be simplified by moving the cleanup logic into the tearDown() method. PHPUnit automatically calls tearDown() after each test, ensuring cleanup happens even if assertions fail. This makes the test methods cleaner and more focused on the test logic itself. You can add a property to the class to hold the temporary path to be cleaned up.

Example:

class DatabaseConfigTest extends TestCase
{
    // ...
    private ?string $tempHome = null;

    protected function tearDown(): void
    {
        // ... restore HOME env
        if ($this->tempHome !== null) {
            $this->cleanupTempHome($this->tempHome);
            $this->tempHome = null;
        }
    }

    public function testSetDbThrowsExceptionWhenConfigMissing(): void
    {
        $this->tempHome = sys_get_temp_dir() . '/dbconfig_test_missing_' . uniqid();
        $this->createTempHome($this->tempHome);

        $this->expectException(RuntimeException::class);
        $this->expectExceptionMessage('could not be read or parsed');

        new Database('localhost');
    }
    // ...
}

}

public function testSetDbThrowsExceptionWhenPasswordMissing(): void
{
$tempHome = sys_get_temp_dir() . '/dbconfig_test_incomplete_' . uniqid();
$this->createTempHome($tempHome, ['user' => 'exampleuser']);

$this->expectException(RuntimeException::class);
$this->expectExceptionMessage('password');

Choose a reason for hiding this comment

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

medium

The assertion for the exception message is very broad. A more specific assertion makes the test more robust and less likely to pass for the wrong reasons. You can use expectExceptionMessageMatches for a regex-based match to check for the key parts of the message.

        $this->expectExceptionMessageMatches('/missing required "password" entry/');


try {
new Database('tools');
} finally {
$this->cleanupTempHome($tempHome);
}
}

private function createTempHome(string $path, array $config = []): void
{
$configDir = $path . '/confs';
if (!is_dir($configDir)) {
mkdir($configDir, 0777, true);

Choose a reason for hiding this comment

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

medium

The file permission 0777 is overly permissive. Even for temporary directories, it's a good practice to use the most restrictive permissions possible to follow the principle of least privilege. 0700 would be more appropriate here, as it grants access only to the owner.

            mkdir($configDir, 0700, true);

}

putenv('HOME=' . $path);

if ($config !== []) {
$ini = '';
foreach ($config as $key => $value) {
$ini .= sprintf("%s = \"%s\"\n", $key, $value);
}
file_put_contents($configDir . '/db.ini', $ini);
}
}

private function cleanupTempHome(string $path): void
{
if (!is_dir($path)) {
return;
}

$items = new RecursiveIteratorIterator(
new RecursiveDirectoryIterator($path, FilesystemIterator::SKIP_DOTS),
RecursiveIteratorIterator::CHILD_FIRST
);

foreach ($items as $item) {
if ($item->isDir()) {
@rmdir($item->getPathname());
} else {
@unlink($item->getPathname());
}
}

@rmdir($path);
Comment on lines +89 to +97

Choose a reason for hiding this comment

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

medium

Using the error suppression operator (@) should be avoided as it can hide underlying problems. If cleanup fails, it might indicate an issue (like a file lock) that could affect other tests. It's better to let exceptions be thrown during cleanup. If a test fails due to a cleanup issue, it's a signal that something is wrong.

        foreach ($items as $item) {
            if ($item->isDir()) {
                rmdir($item->getPathname());
            } else {
                unlink($item->getPathname());
            }
        }

        rmdir($path);

}
}