Skip to content

Handle missing mdwiki database configuration #190

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Handle missing mdwiki database configuration #190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 32 additions & 1 deletion src/backend/api_calls/mdwiki_sql.php
View file
Open in desktop
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,37 @@ 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");
$configPath = $this->home_dir . "/confs/db.ini";
$ts_mycnf = @parse_ini_file($configPath);
Copy link

@gemini-code-assist gemini-code-assist bot Nov 3, 2025

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 (@) is generally discouraged as it can hide important warnings (e.g., if the file is unreadable due to permissions) and make debugging more difficult. It's better practice to explicitly check for the file's existence and readability using is_readable() before attempting to parse it.


if ($ts_mycnf === false || !is_array($ts_mycnf)) {
Copy link

@gemini-code-assist gemini-code-assist bot Nov 3, 2025

Choose a reason for hiding this comment

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

medium

The !is_array($ts_mycnf) check is redundant. According to the PHP documentation, parse_ini_file returns false on any failure (including file not found when @ is used) and an array on success (which can be empty). A strict check for false is sufficient and makes the code cleaner.

        if ($ts_mycnf === false) {

error_log(sprintf('Database configuration file "%s" is missing or unreadable.', $configPath));
throw new RuntimeException('Database configuration error.');
}

$requiredKeys = ['user'];
$missingKeys = [];
foreach ($requiredKeys as $key) {
if (!isset($ts_mycnf[$key]) || $ts_mycnf[$key] === '') {
$missingKeys[] = $key;
}
}

if ($server_name !== 'localhost' && (!isset($ts_mycnf['password']) || $ts_mycnf['password'] === '')) {
$missingKeys[] = 'password';
}

if (!empty($missingKeys)) {
error_log(
sprintf(
'Database configuration file "%s" is missing required key(s): %s.',
$configPath,
implode(', ', array_unique($missingKeys))
)
);
throw new RuntimeException('Database configuration error.');
}

// ---
if ($server_name === 'localhost') {
$this->host = 'localhost:3306';
Expand Down
71 changes: 71 additions & 0 deletions tests/MdwikiSqlDatabaseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<?php

declare(strict_types=1);

use APICalls\MdwikiSql\Database;
use PHPUnit\Framework\TestCase;

final class MdwikiSqlDatabaseTest extends TestCase
{
private string $previousHome = '';

protected function setUp(): void
{
$home = getenv('HOME');
if ($home !== false) {
$this->previousHome = $home;
}
}

protected function tearDown(): void
{
if ($this->previousHome === '') {
putenv('HOME');
Copy link

@gemini-code-assist gemini-code-assist bot Nov 3, 2025

Choose a reason for hiding this comment

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

medium

To reliably unset an environment variable, you should pass an empty string as the value. The behavior of putenv() with only the variable name is not guaranteed across all systems and PHP versions for unsetting. The documented way is to use putenv('VAR=').

            putenv('HOME=');

} else {
putenv('HOME=' . $this->previousHome);
}
}

public function testMissingConfigThrowsRuntimeException(): void
{
$tmpDir = sys_get_temp_dir() . '/mdwiki_sql_test_' . uniqid();
mkdir($tmpDir, 0777, true);
putenv('HOME=' . $tmpDir);

$this->expectException(\RuntimeException::class);
$this->expectExceptionMessage('Database configuration error');

try {
new Database('localhost');
} finally {
$this->removeDir($tmpDir);
}
}

private function removeDir(string $dir): void
Copy link

@gemini-code-assist gemini-code-assist bot Nov 3, 2025

Choose a reason for hiding this comment

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

medium

The use of the error suppression operator (@) on lines 65 and 69 can hide issues during test cleanup (e.g., permission errors), potentially leaving artifacts that could interfere with other tests. It's generally better to avoid error suppression to make debugging easier and test environments cleaner.

{
if (!is_dir($dir)) {
return;
}

$items = scandir($dir);
if ($items === false) {
return;
}

foreach ($items as $item) {
if ($item === '.' || $item === '..') {
continue;
}

$path = $dir . DIRECTORY_SEPARATOR . $item;
if (is_dir($path)) {
$this->removeDir($path);
} else {
@unlink($path);
}
}

@rmdir($dir);
}
}