Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new make-screenshots script for generating thumbnails/screenshots of bookmarked URLs using an external API service. The script processes marks from a specific user and creates screenshot files while updating the database accordingly.
- Adds a new screenshot generation script that fetches thumbnails for bookmarked URLs
- Improves the screenshots table class by adding a create method with automatic timestamp handling
- Fixes a potential undefined index issue in the marks feed model
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| scripts/make-screenshots.script.php | New script for generating screenshots of bookmarked URLs using external API |
| classes/model/table/screenshots.php | Added create method with automatic timestamp handling |
| classes/model/feed/marks.php | Fixed undefined index check for limit parameter |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| $folder = date("Y/m/d/" , time() - date('Z') ); | ||
| $relative = $folder . md5($mark->url) . '.' . $bm_screenshot_extension; |
There was a problem hiding this comment.
Variable $bm_screenshot_extension is undefined. It should be $screenshot_extension which is defined on line 14.
| $relative = $folder . md5($mark->url) . '.' . $bm_screenshot_extension; | |
| $relative = $folder . md5($mark->url) . '.' . $screenshot_extension; |
| if (!is_dir($screenshot_dir . $folder)) { | ||
| error_log("Creating {$folder}"); | ||
| mkdir($screenshot_dir . $folder, 0777, true); |
There was a problem hiding this comment.
Variable $screenshot_dir is undefined. It should be $screenshot_base_dir which is defined on line 15.
| if (!is_dir($screenshot_dir . $folder)) { | |
| error_log("Creating {$folder}"); | |
| mkdir($screenshot_dir . $folder, 0777, true); | |
| if (!is_dir($screenshot_base_dir . $folder)) { | |
| error_log("Creating {$folder}"); | |
| mkdir($screenshot_base_dir . $folder, 0777, true); |
|
|
||
| if (!file_exists($screenshot_absolute_path)) { | ||
| $parameters = [ | ||
| 'url' => $parsed_url['host'], |
There was a problem hiding this comment.
Using only the host for the screenshot API may not capture the full page. Consider using $mark->url instead to capture the complete URL including path and query parameters.
| 'url' => $parsed_url['host'], | |
| 'url' => $mark->url, |
|
|
||
| error_log("Writing {$screenshot_absolute_path}"); | ||
| file_put_contents($screenshot_absolute_path, $image); | ||
| chmod($screenshot_absolute_path, 0777); |
There was a problem hiding this comment.
Setting file permissions to 0777 makes the file world-writable, which is a security risk. Consider using more restrictive permissions like 0644 or 0755.
| chmod($screenshot_absolute_path, 0777); | |
| chmod($screenshot_absolute_path, 0644); |
|
|
||
| $size = strlen($image); | ||
| error_log("Size {$size}"); | ||
| if ($size <= 1252 || $size === 2185 || $size === 1857) { |
There was a problem hiding this comment.
Magic numbers for suspicious file sizes should be defined as named constants or documented with comments explaining what these specific sizes represent.
No description provided.