-
Notifications
You must be signed in to change notification settings - Fork 7
Add support for themes service data #43
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.
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
8add3fd
c9d132f
0341de3
dbdd4b9
be356d2
d9fcc7a
555ce84
eba2acb
572d7da
549d150
e09176f
d794183
47f764c
a6895c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,273 @@ | ||||||||||||||||||||||||||||||||
| <?php | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| namespace DiagramGenerator\Image; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| use DiagramGenerator\Config; | ||||||||||||||||||||||||||||||||
| use DiagramGenerator\Config\Texture; | ||||||||||||||||||||||||||||||||
| use DiagramGenerator\Fen; | ||||||||||||||||||||||||||||||||
| use DiagramGenerator\Fen\Piece; | ||||||||||||||||||||||||||||||||
| use Intervention\Image\Exception\NotReadableException; | ||||||||||||||||||||||||||||||||
| use Intervention\Image\Image; | ||||||||||||||||||||||||||||||||
| use Intervention\Image\ImageManagerStatic; | ||||||||||||||||||||||||||||||||
| use RuntimeException; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| class StorageNew | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| protected $pieces = []; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /** @var string */ | ||||||||||||||||||||||||||||||||
| protected $cacheDirectory; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /** @var string */ | ||||||||||||||||||||||||||||||||
| protected $pieceThemeUrl; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /** @var string */ | ||||||||||||||||||||||||||||||||
| protected $boardTextureUrl; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||
| * @param string $cacheDirectory | ||||||||||||||||||||||||||||||||
| * @param string $pieceThemeUrl | ||||||||||||||||||||||||||||||||
| * @param string $boardTextureUrl | ||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||
| public function __construct($cacheDirectory, $pieceThemeUrl, $boardTextureUrl) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| $this->cacheDirectory = $cacheDirectory; | ||||||||||||||||||||||||||||||||
| $this->pieceThemeUrl = $pieceThemeUrl; | ||||||||||||||||||||||||||||||||
| $this->boardTextureUrl = $boardTextureUrl; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| * | |
| * | |
| * @param Piece $piece | |
| * @param Config $config |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
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.
| * | |
| * | |
| * @param Config $config |
There was a problem hiding this comment.
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.
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
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.
| * | |
| * @param Fen $fen | |
| * @param Config $config |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
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.
| * | |
| * | |
| * @param Piece $piece | |
| * @param Config $config |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
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.
| * Downloads all piece images from theme URLs. | |
| * Downloads all piece images from theme URLs. | |
| * | |
| * @param Config $config |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
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,
// ...
];| $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
AI
Nov 24, 2025
There was a problem hiding this comment.
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.
| foreach($handles as $key => $handle) { | |
| foreach ($handles as $key => $handle) { |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
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
AI
Nov 27, 2025
There was a problem hiding this comment.
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
AI
Nov 24, 2025
There was a problem hiding this comment.
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']);
}
}| // Close all file handles to prevent resource leaks | |
| foreach ($fileHandles as $fileHandle) { | |
| if (is_resource($fileHandle['handle'])) { | |
| fclose($fileHandle['handle']); | |
| } | |
| } |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
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);
}| // Cleanup: remove and close each cURL handle | |
| foreach ($handles as $handle) { | |
| curl_multi_remove_handle($multiHandle, $handle); | |
| curl_close($handle); | |
| } |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
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".
| * Fetches remove file, and stores it locally | |
| * Fetches remote file, and stores it locally |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in the comment: "remove" should be "remote".
| * Fetches remove file, and stores it locally | |
| * Fetches remote file, and stores it locally |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cURL execution result is not checked. If curl_exec() fails, it returns false, but this is not validated. Consider checking the result and throwing an exception if the download fails, similar to how file handle creation is handled above.
There was a problem hiding this comment.
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?