Skip to content

Commit fb97bce

Browse files
MrPunyapalMrPunyapal
committed
fix: validate commonmark options at the builder boundary
Follow-up to #14: - Validate commonMarkExtensions()/commonMarkConfig() in Builder so both PHP and CLI paths produce clear errors (previously CLI-only). - Include offending class-strings in extension validation messages. - Cache the CommonMarkRenderer instance so readme-index builds reuse one converter instead of constructing two. - Slim loadCommonMarkConfig() to file/manifest shape checks now that value validation lives in the builder. - Add failure-path CLI tests (missing file, non-array return, invalid extension) with a shared docsmithCliProcess() helper. - Fix usage.md example output dir for consistency (dist -> docs).
1 parent 19fbafb commit fb97bce

5 files changed

Lines changed: 155 additions & 28 deletions

File tree

‎bin/docsmith‎

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,9 @@ function boolOption(array $options, string $key): bool
254254
}
255255

256256
/**
257+
* Load a CLI CommonMark config file. Extension and config values are
258+
* validated by the builder when applied.
259+
*
257260
* @return array{extensions: list<ExtensionInterface>, config: array<string, mixed>}
258261
*/
259262
function loadCommonMarkConfig(string $path): array
@@ -275,22 +278,10 @@ function loadCommonMarkConfig(string $path): array
275278
throw new InvalidArgumentException('[extensions] must be a list of CommonMark extensions.');
276279
}
277280

278-
foreach ($extensions as $extension) {
279-
if (! $extension instanceof ExtensionInterface) {
280-
throw new InvalidArgumentException('[extensions] must contain only ExtensionInterface instances.');
281-
}
282-
}
283-
284281
if (! is_array($config)) {
285282
throw new InvalidArgumentException('[config] must be an array.');
286283
}
287284

288-
foreach (array_keys($config) as $key) {
289-
if (! is_string($key)) {
290-
throw new InvalidArgumentException('[config] must use string keys.');
291-
}
292-
}
293-
294285
return [
295286
'extensions' => $extensions,
296287
'config' => $config,

‎md/usage.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ use League\CommonMark\Extension\DescriptionList\DescriptionListExtension;
4343

4444
Docsmith::make()
4545
->source(__DIR__ . '/md')
46-
->output(__DIR__ . '/dist')
46+
->output(__DIR__ . '/docs')
4747
->commonMarkExtensions([
4848
new DescriptionListExtension(),
4949
])

‎src/Builder/Builder.php‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use Docsmith\Markdown\CommonMarkRenderer;
1313
use Docsmith\Render\OgImageGenerator;
1414
use Docsmith\Render\SiteBuilder;
15+
use InvalidArgumentException;
1516
use League\CommonMark\Extension\ExtensionInterface;
1617
use LogicException;
1718
use RecursiveDirectoryIterator;
@@ -74,6 +75,8 @@ final class Builder
7475
/** @var array<string, mixed> */
7576
private array $commonMarkConfig = [];
7677

78+
private ?CommonMarkRenderer $commonMarkRenderer = null;
79+
7780
/** @var list<string> */
7881
private array $navigationOrder = [];
7982

@@ -440,9 +443,15 @@ public function rightSidebar(bool $rightSidebar = true): self
440443
* Register additional League CommonMark extensions for Markdown rendering.
441444
*
442445
* @param list<ExtensionInterface> $extensions
446+
*
447+
* @throws InvalidArgumentException When an extension does not implement ExtensionInterface.
443448
*/
444449
public function commonMarkExtensions(array $extensions): self
445450
{
451+
foreach ($extensions as $extension) {
452+
$this->assertCommonMarkExtension($extension);
453+
}
454+
446455
$this->commonMarkExtensions = $extensions;
447456

448457
return $this;
@@ -455,14 +464,41 @@ public function commonMarkExtensions(array $extensions): self
455464
* configuration.
456465
*
457466
* @param array<string, mixed> $config
467+
*
468+
* @throws InvalidArgumentException When a config key is not a string.
458469
*/
459470
public function commonMarkConfig(array $config): self
460471
{
472+
foreach (array_keys($config) as $key) {
473+
$this->assertCommonMarkConfigKey($key);
474+
}
475+
461476
$this->commonMarkConfig = $config;
462477

463478
return $this;
464479
}
465480

481+
private function assertCommonMarkExtension(mixed $extension): void
482+
{
483+
if (! $extension instanceof ExtensionInterface) {
484+
throw new InvalidArgumentException(sprintf(
485+
'CommonMark extensions must implement %s, [%s] given.',
486+
ExtensionInterface::class,
487+
is_string($extension) ? $extension : get_debug_type($extension),
488+
));
489+
}
490+
}
491+
492+
private function assertCommonMarkConfigKey(mixed $key): void
493+
{
494+
if (! is_string($key)) {
495+
throw new InvalidArgumentException(sprintf(
496+
'CommonMark config must use string keys, [%s] given.',
497+
get_debug_type($key),
498+
));
499+
}
500+
}
501+
466502
/** @param list<string> $order */
467503
public function navigationOrder(array $order): self
468504
{
@@ -783,7 +819,7 @@ private function generateOgImages(BuildConfig $config, ?array $documents = null,
783819

784820
private function commonMarkRenderer(): CommonMarkRenderer
785821
{
786-
return new CommonMarkRenderer($this->commonMarkExtensions, $this->commonMarkConfig);
822+
return $this->commonMarkRenderer ??= new CommonMarkRenderer($this->commonMarkExtensions, $this->commonMarkConfig);
787823
}
788824

789825
private function buildDocs(): void

‎tests/Feature/BuildSiteTest.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
declare(strict_types=1);
44

5+
use Docsmith\Builder\Builder;
56
use Docsmith\Docsmith;
67
use League\CommonMark\Extension\DescriptionList\DescriptionListExtension;
78

@@ -41,6 +42,18 @@
4142
->and(str_contains($html, 'removed'))->toBeFalse();
4243
});
4344

45+
it('rejects commonmark extension values that are not extension instances', function (): void {
46+
// @phpstan-ignore-next-line Deliberately passing an invalid value.
47+
expect(fn (): Builder => Docsmith::make()->commonMarkExtensions([DescriptionListExtension::class]))
48+
->toThrow(InvalidArgumentException::class, DescriptionListExtension::class);
49+
});
50+
51+
it('rejects commonmark config keys that are not strings', function (): void {
52+
// @phpstan-ignore-next-line Deliberately passing an invalid value.
53+
expect(fn (): Builder => Docsmith::make()->commonMarkConfig(['html_input']))
54+
->toThrow(InvalidArgumentException::class, 'string keys');
55+
});
56+
4457
it('rewrites markdown links between pages into built page urls', function (): void {
4558
$sourcePath = sys_get_temp_dir() . '/docsmith-md-links-' . uniqid();
4659
mkdir($sourcePath . '/guides', 0777, true);

‎tests/Feature/CommonMarkCliTest.php‎

Lines changed: 101 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
declare(strict_types=1);
44

5+
use League\CommonMark\Extension\DescriptionList\DescriptionListExtension;
6+
57
it('uses commonmark extensions and environment config in cli builds', function (): void {
68
$projectPath = sys_get_temp_dir() . '/docsmith-commonmark-cli-' . uniqid();
79
$sourcePath = $projectPath . '/md';
@@ -31,13 +33,103 @@
3133
];
3234
PHP);
3335

34-
$command = [
35-
PHP_BINARY,
36-
dirname(__DIR__, 2) . '/bin/docsmith',
36+
$result = docsmithCliProcess([
3737
'build',
3838
'--source=' . $sourcePath,
3939
'--output=' . $outputPath,
4040
'--commonmark-config=' . $commonMarkConfigPath,
41+
]);
42+
43+
expect($result['exitCode'])->toBe(0, $result['stderr'])
44+
->and($result['stdout'])->toContain('Built docs');
45+
46+
$html = (string) file_get_contents($outputPath . '/index.html');
47+
48+
expect($html)->toContain('<dl>')
49+
->toContain('<dt>Term</dt>')
50+
->toContain('<dd>Definition</dd>')
51+
->and(str_contains($html, '<div>'))->toBeFalse()
52+
->and(str_contains($html, 'removed'))->toBeFalse();
53+
});
54+
55+
it('fails clearly when the commonmark config file does not exist', function (): void {
56+
$projectPath = sys_get_temp_dir() . '/docsmith-commonmark-cli-missing-' . uniqid();
57+
$sourcePath = $projectPath . '/md';
58+
mkdir($sourcePath, 0777, true);
59+
file_put_contents($sourcePath . '/index.md', '# Hello');
60+
61+
$result = docsmithCliProcess([
62+
'build',
63+
'--source=' . $sourcePath,
64+
'--output=' . $projectPath . '/docs',
65+
'--commonmark-config=' . $projectPath . '/missing.php',
66+
]);
67+
68+
expect($result['exitCode'])->toBe(1)
69+
->and($result['stderr'])->toContain('[Docsmith] Invalid CommonMark config')
70+
->and($result['stderr'])->toContain('File does not exist');
71+
});
72+
73+
it('fails clearly when the commonmark config does not return an array', function (): void {
74+
$projectPath = sys_get_temp_dir() . '/docsmith-commonmark-cli-nonarray-' . uniqid();
75+
$sourcePath = $projectPath . '/md';
76+
mkdir($sourcePath, 0777, true);
77+
file_put_contents($sourcePath . '/index.md', '# Hello');
78+
79+
$configPath = $projectPath . '/commonmark.php';
80+
file_put_contents($configPath, "<?php\n\nreturn 'not-an-array';\n");
81+
82+
$result = docsmithCliProcess([
83+
'build',
84+
'--source=' . $sourcePath,
85+
'--output=' . $projectPath . '/docs',
86+
'--commonmark-config=' . $configPath,
87+
]);
88+
89+
expect($result['exitCode'])->toBe(1)
90+
->and($result['stderr'])->toContain('The file must return an array.');
91+
});
92+
93+
it('fails clearly when the commonmark config has invalid extensions', function (): void {
94+
$projectPath = sys_get_temp_dir() . '/docsmith-commonmark-cli-badext-' . uniqid();
95+
$sourcePath = $projectPath . '/md';
96+
mkdir($sourcePath, 0777, true);
97+
file_put_contents($sourcePath . '/index.md', '# Hello');
98+
99+
$configPath = $projectPath . '/commonmark.php';
100+
file_put_contents($configPath, <<<'PHP'
101+
<?php
102+
103+
return [
104+
'extensions' => [\League\CommonMark\Extension\DescriptionList\DescriptionListExtension::class],
105+
];
106+
PHP);
107+
108+
$result = docsmithCliProcess([
109+
'build',
110+
'--source=' . $sourcePath,
111+
'--output=' . $projectPath . '/docs',
112+
'--commonmark-config=' . $configPath,
113+
]);
114+
115+
expect($result['exitCode'])->toBe(1)
116+
->and($result['stderr'])->toContain(DescriptionListExtension::class)
117+
->and($result['stderr'])->toContain('must implement');
118+
});
119+
120+
/**
121+
* Run the Docsmith binary in a subprocess and capture its output.
122+
*
123+
* @param list<string> $arguments
124+
*
125+
* @return array{exitCode: int, stdout: string, stderr: string}
126+
*/
127+
function docsmithCliProcess(array $arguments): array
128+
{
129+
$command = [
130+
PHP_BINARY,
131+
dirname(__DIR__, 2) . '/bin/docsmith',
132+
...$arguments,
41133
];
42134
$pipes = [];
43135
$process = proc_open($command, [
@@ -55,14 +147,9 @@
55147
fclose($pipes[2]);
56148
$exitCode = proc_close($process);
57149

58-
expect($exitCode)->toBe(0, $stderr)
59-
->and($stdout)->toContain('Built docs');
60-
61-
$html = (string) file_get_contents($outputPath . '/index.html');
62-
63-
expect($html)->toContain('<dl>')
64-
->toContain('<dt>Term</dt>')
65-
->toContain('<dd>Definition</dd>')
66-
->and(str_contains($html, '<div>'))->toBeFalse()
67-
->and(str_contains($html, 'removed'))->toBeFalse();
68-
});
150+
return [
151+
'exitCode' => $exitCode,
152+
'stdout' => (string) $stdout,
153+
'stderr' => (string) $stderr,
154+
];
155+
}

0 commit comments

Comments
 (0)