Skip to content

[BUGFIX] Improve PlantumlRenderer robustness and temp file cleanup #1281

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
CybotTM wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[BUGFIX] Improve PlantumlRenderer robustness and temp file cleanup #1281

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
88d4222
fix: Improve PlantumlRenderer robustness and cleanup
CybotTM Jan 4, 2026
9258580
test: Improve test cleanup safety with multiple guards
CybotTM Jan 7, 2026
9cf7aa2
refactor: Extract ensureDirectoryExists helper method
CybotTM Jan 8, 2026
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
26 changes: 23 additions & 3 deletions packages/guides-graphs/src/Graphs/Renderer/PlantumlRenderer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use function mkdir;
use function sys_get_temp_dir;
use function tempnam;
use function unlink;

final class PlantumlRenderer implements DiagramRenderer
{
Expand Down Expand Up @@ -53,11 +54,25 @@ public function render(RenderContext $renderContext, string $diagram): string|nu
@enduml
PUML;

if (!is_dir($this->tempDirectory)) {
mkdir($this->tempDirectory, 0o755, true);
if (!is_dir($this->tempDirectory) && !@mkdir($this->tempDirectory, 0o755, true) && !is_dir($this->tempDirectory)) {
Copy link
Member

@jaapio jaapio Jan 7, 2026

Choose a reason for hiding this comment

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

I think it's better to split the conditions here. And not to perform operations in an condition like mkdir those are very hard to detect, people do not expect them in there.
Also the !is_dir($this->tempDirectory) is performed twice, that seems to be invalid.

Copy link
Contributor Author

@CybotTM CybotTM Jan 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

That's a simple trinary operation, not so uncommon/unknown, and the duplicated is_dir() is intentionally, that's the whole point of it.

if (!is_dir($this->tempDirectory)) {

    // 2. Attempt Creation: Try to create it with 0755 permissions.
    // The '@' suppresses a PHP warning if it fails.
    // 'true' allows recursive creation of parent folders.
    $creationSuccessful = @mkdir($this->tempDirectory, 0755, true);

    if (!$creationSuccessful) {
        
        // 3. Final Verification (Race Condition Check):
        // If mkdir failed, it might be because another process 
        // created it between step 1 and step 2.
        if (!is_dir($this->tempDirectory)) {
            
            // IF IT STILL DOESN'T EXIST: Handle the real failure.
            // This is where you would place your error logic or throw an exception.
            throw new \Exception("Failed to create directory: " . $this->tempDirectory);
        }
    }
}

But this is already explained in the PR description.

Copy link
Member

@jaapio jaapio Jan 8, 2026

Choose a reason for hiding this comment

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

I get it's common to do this, but I do see it as a bad practice to combine things like this. It makes the code hard to understand.

Copy link
Member

@jaapio jaapio Jan 8, 2026

Choose a reason for hiding this comment

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

I get it's common to do this, but I do see it as a bad practice to combine things like this. It makes the code hard to understand.

Copy link
Contributor Author

@CybotTM CybotTM Jan 8, 2026

Choose a reason for hiding this comment

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

I already changed it. You may notice.

$this->logger->error(
'Failed to create temp directory: ' . $this->tempDirectory,
$renderContext->getLoggerInformation(),
);

return null;
}

$pumlFileLocation = tempnam($this->tempDirectory, 'pu_');
if ($pumlFileLocation === false) {
$this->logger->error(
'Failed to create temporary file for diagram',
$renderContext->getLoggerInformation(),
);

return null;
}

file_put_contents($pumlFileLocation, $output);
try {
$process = new Process([$this->plantUmlBinaryPath, '-tsvg', $pumlFileLocation], __DIR__, null, null, 600.0);
Expand Down Expand Up @@ -86,6 +101,11 @@ public function render(RenderContext $renderContext, string $diagram): string|nu
return null;
}

return file_get_contents($pumlFileLocation . '.svg') ?: null;
$svg = file_get_contents($pumlFileLocation . '.svg') ?: null;

@unlink($pumlFileLocation);
@unlink($pumlFileLocation . '.svg');

return $svg;
}
}
74 changes: 66 additions & 8 deletions packages/guides-graphs/tests/unit/Renderer/PlantumlRendererTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,35 +13,93 @@

namespace phpDocumentor\Guides\Graphs\Renderer;

use FilesystemIterator;
use phpDocumentor\Guides\RenderContext;
use PHPUnit\Framework\TestCase;
use Psr\Log\NullLogger;
use RecursiveDirectoryIterator;
use RecursiveIteratorIterator;
use SplFileInfo;
use Throwable;

use function assert;
use function basename;
use function is_dir;
use function realpath;
use function rmdir;
use function sys_get_temp_dir;
use function uniqid;
use function unlink;

use const DIRECTORY_SEPARATOR;

final class PlantumlRendererTest extends TestCase
{
private const TEMP_DIR_PREFIX = 'plantuml-test-';

public function testRenderCreatesTempDirectoryWhenMissing(): void
{
$tempDir = sys_get_temp_dir() . '/plantuml-test-' . uniqid();
$tempDir = sys_get_temp_dir() . '/' . self::TEMP_DIR_PREFIX . uniqid('', true);

// Ensure the directory does not exist
self::assertDirectoryDoesNotExist($tempDir);

// Use a non-existent binary path - the render will fail but directory should be created first
$renderer = new PlantumlRenderer(new NullLogger(), '/non/existent/plantuml', $tempDir);

$renderContext = $this->createMock(RenderContext::class);
$renderContext->method('getLoggerInformation')->willReturn([]);

// The render will fail due to missing binary, but the temp directory should be created
$renderer->render($renderContext, 'A -> B');
try {
// Render will fail (returns null) or throw due to non-existent plantuml binary.
// We only care about verifying that the temp directory was created.
$renderer->render($renderContext, 'A -> B');
} catch (Throwable) {
// Expected: plantuml binary doesn't exist
}

self::assertDirectoryExists($tempDir, 'Temp directory should have been created');

$this->removeTempDirSafely($tempDir);
}

private function removeTempDirSafely(string $dir): void
{
if ($dir === '' || !is_dir($dir)) {
return;
}

$realDir = realpath($dir);
if ($realDir === false) {
return;
}

$realSysTmp = realpath(sys_get_temp_dir());
if ($realSysTmp === false) {
return;
}

// Safety: must be under system temp and have our prefix
self::assertStringStartsWith(
$realSysTmp . DIRECTORY_SEPARATOR,
$realDir . DIRECTORY_SEPARATOR,
'Refusing to delete directory outside system temp',
);
self::assertStringContainsString(
self::TEMP_DIR_PREFIX,
basename($realDir),
'Refusing to delete directory without expected prefix',
);

/** @var RecursiveIteratorIterator<RecursiveDirectoryIterator> $iterator */
$iterator = new RecursiveIteratorIterator(
new RecursiveDirectoryIterator($realDir, FilesystemIterator::SKIP_DOTS),
RecursiveIteratorIterator::CHILD_FIRST,
);

self::assertDirectoryExists($tempDir);
foreach ($iterator as $file) {
assert($file instanceof SplFileInfo);
$file->isDir() ? @rmdir($file->getPathname()) : @unlink($file->getPathname());
}

// Clean up
@rmdir($tempDir);
@rmdir($realDir);
}
}