Skip to content

Conversation

@pavoljurov
Copy link
Collaborator

@pavoljurov pavoljurov commented Nov 24, 2025

Originally, DiagramGenerator accepted legacy theme and piece name (e.g. 'green'/'neo'), and composed asset URL from provided keys.

This PR extends DiagramGenerator config to consume theme URLs directly, so it can be used with new themes service.
Legacy themes with hardcoded names and paths are still supported.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the DiagramGenerator to support theme URLs from a new themes service while maintaining backward compatibility with legacy hardcoded theme names and paths. The main addition is a themeUrls field in the Config class that allows consuming theme assets from dynamic URLs rather than static file paths.

  • Adds StorageNew class to handle theme URL-based image fetching and caching
  • Extends Config class with themeUrls property and related getter/setter/checker methods
  • Updates Board class to conditionally use StorageNew when theme URLs are configured
  • Adds comprehensive test coverage for the new theme URL functionality

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
src/DiagramGenerator/Config.php Adds themeUrls property with getter, setter, and hasThemeUrls() method to support dynamic theme URLs
src/DiagramGenerator/Board.php Conditionally instantiates StorageNew when theme URLs are configured, otherwise uses legacy Storage
src/DiagramGenerator/Image/Image.php Updates type hints to accept both Storage and StorageNew, adds logic to detect board textures from theme URLs
src/DiagramGenerator/Image/StorageNew.php New class implementing theme URL-based image fetching with URL-based caching strategy using MD5 hashes
tests/DiagramGenerator/Tests/ConfigTest.php Adds tests for theme URLs getter, setter, and validation methods
tests/DiagramGenerator/Tests/Image/StorageTest.php Comprehensive test suite for StorageNew including cache path generation and error handling
tests/DiagramGenerator/Tests/Image/ImageTest.php Tests verifying detection of board textures from theme URLs vs legacy mode

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}

/**
* Fetches remove file, and stores it locally
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

Typo in comment: "remove" should be "remote".

Suggested change
* Fetches remove file, and stores it locally
* Fetches remote file, and stores it locally

Copilot uses AI. Check for mistakes.
Comment on lines +288 to +305
protected function removeDirectory($dir)
{
if (!is_dir($dir)) {
return;
}

$files = array_diff(scandir($dir), array('.', '..'));
foreach ($files as $file) {
$path = $dir . '/' . $file;
if (is_dir($path)) {
$this->removeDirectory($path);
} else {
unlink($path);
}
}

rmdir($dir);
}
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The removeDirectory() helper method is duplicated across multiple test files (StorageTest.php and ImageTest.php). Consider extracting this to a shared test trait or base test class to reduce code duplication and improve maintainability.

Copilot uses AI. Check for mistakes.
Comment on lines +110 to +127
protected function removeDirectory($dir)
{
if (!is_dir($dir)) {
return;
}

$files = array_diff(scandir($dir), array('.', '..'));
foreach ($files as $file) {
$path = $dir . '/' . $file;
if (is_dir($path)) {
$this->removeDirectory($path);
} else {
unlink($path);
}
}

rmdir($dir);
}
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The removeDirectory() helper method is duplicated across multiple test files (StorageTest.php and ImageTest.php). Consider extracting this to a shared test trait or base test class to reduce code duplication and improve maintainability.

Copilot uses AI. Check for mistakes.
Comment on lines +228 to +261
protected function createConfigWithThemeUrls()
{
$config = new Config();
$config->setFen('rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR');
$config->setSizeIndex('100px');

$size = new Size();
$size->setCell(100);
$config->setSize($size);

$theme = new Theme();
$theme->setName('test');
$config->setTheme($theme);

$config->setThemeUrls([
'board' => 'https://example.com/board.png',
'wp' => 'https://example.com/wp.png',
]);

return $config;
}

protected function createConfigWithoutThemeUrls()
{
$config = new Config();
$config->setFen('rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR');
$config->setSizeIndex('100px');

$size = new Size();
$size->setCell(100);
$config->setSize($size);

return $config;
}
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The config creation helper methods (createConfigWithThemeUrls(), createConfigWithoutThemeUrls()) are duplicated between StorageTest.php and ImageTest.php. Consider extracting these to a shared test trait or base test class to improve maintainability and ensure consistency across tests.

Copilot uses AI. Check for mistakes.
];
}

foreach($handles as $key => $handle) {
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

Missing space after foreach keyword. This should be foreach ($handles (with a space) to maintain consistency with the coding style used elsewhere in the codebase.

Suggested change
foreach($handles as $key => $handle) {
foreach ($handles as $key => $handle) {

Copilot uses AI. Check for mistakes.
rename($fileHandle['tmpPath'], $fileHandle['realPath']);
}
}

Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

File handles opened with fopen() at line 180 are never closed. After the curl operations complete and files are renamed, the file handles should be closed with fclose() to prevent resource leaks. Add a loop after line 202 to close all file handles:

foreach ($fileHandles as $fileHandle) {
    if (is_resource($fileHandle['handle'])) {
        fclose($fileHandle['handle']);
    }
}
Suggested change
// Close all file handles to prevent resource leaks
foreach ($fileHandles as $fileHandle) {
if (is_resource($fileHandle['handle'])) {
fclose($fileHandle['handle']);
}
}

Copilot uses AI. Check for mistakes.
rename($fileHandle['tmpPath'], $fileHandle['realPath']);
}
}

Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The curl_init() handles created at line 178 are never individually closed with curl_close() before calling curl_multi_close(). While curl_multi_close() may clean up multi handle resources, the individual curl handles should be explicitly removed and closed. Add cleanup after line 196:

foreach ($handles as $handle) {
    curl_multi_remove_handle($multiHandle, $handle);
    curl_close($handle);
}
Suggested change
// Cleanup: remove and close each cURL handle
foreach ($handles as $handle) {
curl_multi_remove_handle($multiHandle, $handle);
curl_close($handle);
}

Copilot uses AI. Check for mistakes.
Comment on lines 179 to 180
$fileHandles[$pieceShortName] = [
'handle' => fopen($filePath . $uniqid, 'wb'),
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

If fopen() fails and returns false, it should be handled before attempting to use the file handle. Add error handling similar to the pattern in cacheImage() method (lines 261-263):

$handle = fopen($filePath . $uniqid, 'wb');
if (!$handle) {
    throw new RuntimeException(sprintf('Could not open temporary file: %s', $filePath . $uniqid));
}
$fileHandles[$pieceShortName] = [
    'handle' => $handle,
    // ...
];
Suggested change
$fileHandles[$pieceShortName] = [
'handle' => fopen($filePath . $uniqid, 'wb'),
$handle = fopen($filePath . $uniqid, 'wb');
if (!$handle) {
throw new RuntimeException(sprintf('Could not open temporary file: %s', $filePath . $uniqid));
}
$fileHandles[$pieceShortName] = [
'handle' => $handle,

Copilot uses AI. Check for mistakes.
@pavoljurov pavoljurov force-pushed the PJ/Add-support-for-themes-service branch from 58ae5c9 to dbdd4b9 Compare November 25, 2025 11:18
@pavoljurov pavoljurov force-pushed the PJ/Add-support-for-themes-service branch from 982db02 to be356d2 Compare November 25, 2025 12:59
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 13 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 61 to 73
public function testDrawBoardWithFiguresUsesLegacyTextureWhenThemeUrlsNotSet()
{
$config = $this->createConfigWithoutThemeUrls();
$storage = new Storage($this->cacheDirectory, $this->pieceThemeUrl, $this->boardTextureUrl);
$image = new Image($storage, $config);

// Verify that hasThemeUrls returns false
$this->assertFalse($config->hasThemeUrls());

// The actual drawing will fail due to missing images/colors, but we've verified
// the config setup is correct for legacy mode
$this->assertTrue(true);
}
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

Similar to the previous test, this only verifies config setup without testing actual behavior. Consider adding assertions that test the actual drawing logic or exception handling.

Copilot uses AI. Check for mistakes.

/**
* Gets background texture image from theme URL.
*
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

Missing @param Config $config parameter documentation in the PHPDoc comment.

Suggested change
*
*
* @param Config $config

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Useless, it's already in the code.

/**
* Finds max height of piece image
*
*
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

Missing parameter documentation. Should include @param Fen $fen and @param Config $config in the PHPDoc comment.

Suggested change
*
* @param Fen $fen
* @param Config $config

Copilot uses AI. Check for mistakes.

/**
* Fetches piece image from theme URL.
*
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

Missing parameter documentation. Should include @param Piece $piece and @param Config $config in the PHPDoc comment.

Suggested change
*
*
* @param Piece $piece
* @param Config $config

Copilot uses AI. Check for mistakes.
}

/**
* Downloads all piece images from theme URLs.
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

Missing parameter documentation. Should include @param Config $config in the PHPDoc comment.

Suggested change
* Downloads all piece images from theme URLs.
* Downloads all piece images from theme URLs.
*
* @param Config $config

Copilot uses AI. Check for mistakes.
Comment on lines +75 to +108
protected function createConfigWithThemeUrls()
{
$config = new Config();
$config->setFen('rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR');
$config->setSizeIndex('100px');

$size = new Size();
$size->setCell(100);
$config->setSize($size);

$theme = new Theme();
$theme->setName('test');
$config->setTheme($theme);

$config->setThemeUrls([
'board' => 'https://example.com/board.png',
'wp' => 'https://example.com/wp.png',
]);

return $config;
}

protected function createConfigWithoutThemeUrls()
{
$config = new Config();
$config->setFen('rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR');
$config->setSizeIndex('100px');

$size = new Size();
$size->setCell(100);
$config->setSize($size);

return $config;
}
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

The createConfigWithThemeUrls and createConfigWithoutThemeUrls helper methods have duplicated setup code. Consider extracting the common config setup into a base method to reduce duplication, similar to the issue in StorageTest.php.

Copilot uses AI. Check for mistakes.

/**
* Gets piece image from theme URLs.
*
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

The method documentation is incomplete. It should include @param Piece $piece and @param Config $config to document the parameters, following the PHPDoc standard used in the codebase.

Suggested change
*
*
* @param Piece $piece
* @param Config $config

Copilot uses AI. Check for mistakes.
Comment on lines +167 to +172
foreach($handles as $key => $handle) {
curl_setopt($handle, CURLOPT_FILE, $fileHandles[$key]['handle']);
curl_setopt($handle, CURLOPT_HEADER, 0);

curl_multi_add_handle($multiHandle, $handle);
}
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

The cURL handle operations in the loop don't check for errors. Consider checking if curl_init() returns false and handling that case, or at minimum checking for errors after curl_multi_exec() to ensure downloads completed successfully.

Copilot uses AI. Check for mistakes.
Comment on lines +179 to +183
foreach ($fileHandles as $fileHandle) {
if (isset($fileHandle['tmpPath']) && file_exists($fileHandle['tmpPath'])) {
rename($fileHandle['tmpPath'], $fileHandle['realPath']);
}
}
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

File handles should be closed before attempting to rename the files. On Windows systems, attempting to rename an open file may fail. The file handles in $fileHandles should be closed in this loop before the rename() call, not later in the cleanup section.

Copilot uses AI. Check for mistakes.
Comment on lines 46 to 59
public function testDrawBoardWithFiguresDetectsBoardTextureFromThemeUrls()
{
$config = $this->createConfigWithThemeUrls();
$storage = new StorageNew($this->cacheDirectory);
$image = new Image($storage, $config);

// Verify that hasThemeUrls works correctly
$this->assertTrue($config->hasThemeUrls());
$this->assertTrue(isset($config->getThemeUrls()['board']));

// The actual drawing will fail due to missing images/colors, but we've verified
// the config setup is correct for theme URLs
$this->assertTrue(true);
}
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

This test doesn't actually verify the behavior of drawBoardWithFigures. It only checks the config setup and then asserts true. Consider either calling the method and checking for expected behavior/exceptions, or making this an integration test that verifies the full drawing workflow.

Copilot uses AI. Check for mistakes.
Comment on lines 81 to 83
$storage = new StorageNew($this->cacheDir);
} else {
$storage = new Storage($this->cacheDir, $this->pieceThemeUrl, $this->boardTextureUrl);

Choose a reason for hiding this comment

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

What I usually prefer is to rename old class to StorageLegacy, because ultimately we would won't to delete the old class and new one should have the default (Storage) name. One less step during FF cleanup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will change it. I used New suffix to make review easier, as it is obvious that original file was not changed.

use Intervention\Image\ImageManagerStatic;
use RuntimeException;

class StorageNew implements StorageInterface

Choose a reason for hiding this comment

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

Just curious, is this implementation AI-generated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it is. As a Frontend dev I wouldn't do this PR by normal means. I started with Cursor implementation, but then I checked the code and tried to steer it to whatever looked as a good code to me. I tested code manually afterwards, but deep review by PHP senior is more then welcome.

@nikolaposa
Copy link

@pavoljurov Do you know who are original Diagram generator authors? I think we should consult with them about these changes, add as reviewers for sure.

@pavoljurov
Copy link
Collaborator Author

@pavoljurov Do you know who are original Diagram generator authors? I think we should consult with them about these changes, add as reviewers for sure.

It is very old codebase, but I see @umpirsky and @ikonic89 did some work 10 years ago. You guys have definitely good overview about this repository, would you mind to see this PR and review it? 🙇‍♂️

@nikolaposa
Copy link

nikolaposa commented Nov 27, 2025

What project are these changes being made for? Are there any BE developers on that project, is it a top 10 project?

Some changes being made here seems fishy, e.g. the use of mkdir and local filesystem in general, which could be problematic in the distributed world our infra runs in.

Also using curl_* instead of some HTTP client, most notably Guzzle that we use as a de-facto standard.

I think these are quite serious, impactful changes which need to be thought out much better. I kinda doubt that Cursor you used for this has all these things in mind.

@pavoljurov
Copy link
Collaborator Author

What project are these changes being made for? Are there any BE developers on that project, is it a top 10 project?

This library is used by Symfony (main chess repo). I am not sure about other projects which use it. There are no BE devs, no top 10 project, no planning - just this PR. I was teased in Slack thread here and here, so I decided to get the s**t done.

Some changes being made here seems fishy, e.g. the use of mkdir ... Also using curl_* instead of some HTTP client... I kinda doubt that Cursor you used for this has all these things in mind

Cursor used exactly same tech which is used in already existing Storage class.

@umpirsky
Copy link
Contributor

I defer to the original author @ikonic89.

Copy link
Member

@ikonic89 ikonic89 left a comment

Choose a reason for hiding this comment

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

I will approve, but:

I defer to the original author @ikonic89.

Original author @ikonic89 is actually just someone who ported this over from Qcodo, hahahah. There wasn't much original code here. It's all over 12 years old.

Is there a way to DL these changes?

$destHeight,
$srcWidth,
$srcHeight
);
Copy link
Member

Choose a reason for hiding this comment

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

if backgroundTexture image doesn't have perfect dimensions, this could result in asymetric image generated, right?

Of course, if background is always perfect, then nothing to worry about.

Copy link
Contributor

@umpirsky umpirsky left a comment

Choose a reason for hiding this comment

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

I reviewed the code, and the most concerning part is StorageNew. Name aside, which should be changed, the class looks like it needs some polishing. As @nikolaposa mentioned, it uses file system and network extensively, and does not follow our project patterns.

Maybe the way forward is exposing Storage and let clients implement their own storage. So the monolith can do $board->setStorage(), what do you think @nikolaposa ?

Then in main repo we can use project config, guzzle, caching etc. Not sure what is the future of this repo?


public function __construct(Storage $storage, Config $config)
/**
* @param StorageInterface $storage
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this dockblock and add type hints.

* @param StorageInterface $storage
* @param Config $config
*/
public function __construct($storage, Config $config)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason type hint is removed from storage?

Suggested change
public function __construct($storage, Config $config)
public function __construct(StorageInterface $storage, Config $config)

@pavoljurov
Copy link
Collaborator Author

Not sure what is the future of this repo?

It is very old repository and I would like to avoid massive improvements there. I mimicked the code which existed already. Goal is to enable new themes easily. If it is not aligned with more recent project patterns and those patterns are must, I don't think this PR should be continued (at least not by me).

@umpirsky
Copy link
Contributor

As I said, maybe the way forward is exposing Storage and let clients implement their own storage.

@nikolaposa
Copy link

Maybe the way forward is exposing Storage and let clients implement their own storage. So the monolith can do $board->setStorage(), what do you think @nikolaposa

You mean like keep the StorageInterface idea from this PR, but implement new storage implementation in user land (chess repo)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants