557 Show detail button for items in media library#438
Open
Conversation
Reviewer's GuideAdds a new "detail" column to the media library items datagrid that renders a context-specific “view” button linking to either YouTube, Vimeo, or a local media file, enabling users to open the media item directly from the list. Updated class diagram for MediaItemDataGrid with new detail column and getDetailUrlclassDiagram
class MediaItemDataGrid {
+__construct(Type type, int folderId, string searchQuery)
+setExtras(Type type, int folderId)
+addColumn(string name)
+setColumnFunction(callable callback, string[] parameters, string columnName, bool overwrite)
+getParameters(Type type, int folderId, string searchQuery) array
+getDetailUrl(string shardingFolderName, string url, string storageType) string
}
class DataGrid {
+addColumn(string name)
+setColumnFunction(callable callback, string[] parameters, string columnName, bool overwrite)
}
class Language {
+lbl(string key) string
}
MediaItemDataGrid --|> DataGrid
MediaItemDataGrid ..> Language : uses
note for MediaItemDataGrid "Constructor now adds detail column and binds getDetailUrl to render context specific view button"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The hardcoded media URLs in
getDetailUrl(YouTube, Vimeo, and the local/src/Frontend/Files/MediaLibrary/path) couple this grid to specific domains and paths; consider using configuration/constants or existing URL helpers so these links remain valid if the environment or routing changes. - The
storageTypestring checks ('youtube','vimeo') are brittle; if you already have an enum/Type/constant representation for storage types elsewhere in the media library domain, reusing that here would reduce the risk of mismatch. - Since
getDetailUrlbuilds HTML with interpolated values, it would be safer to ensure$linkand the label are properly escaped (e.g.htmlspecialchars) and to usesprintfor a small template helper to make the markup clearer and less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hardcoded media URLs in `getDetailUrl` (YouTube, Vimeo, and the local `/src/Frontend/Files/MediaLibrary/` path) couple this grid to specific domains and paths; consider using configuration/constants or existing URL helpers so these links remain valid if the environment or routing changes.
- The `storageType` string checks (`'youtube'`, `'vimeo'`) are brittle; if you already have an enum/Type/constant representation for storage types elsewhere in the media library domain, reusing that here would reduce the risk of mismatch.
- Since `getDetailUrl` builds HTML with interpolated values, it would be safer to ensure `$link` and the label are properly escaped (e.g. `htmlspecialchars`) and to use `sprintf` or a small template helper to make the markup clearer and less error-prone.
## Individual Comments
### Comment 1
<location path="src/Backend/Modules/MediaLibrary/Domain/MediaItem/MediaItemDataGrid.php" line_range="223-227" />
<code_context>
$this->setColumnFunction('htmlspecialchars', ['[title]'], 'title', false);
}
+
+ public static function getDetailUrl(string $shardingFolderName, string $url, string $storageType): string
+ {
+ if ($storageType === 'youtube') {
+ $link = 'https://youtube.com/watch?v=' . $url;
+ } elseif ($storageType === 'vimeo') {
+ $link = 'https://www.vimeo.com/' . $url;
+ } else {
</code_context>
<issue_to_address>
**suggestion:** Avoid hard-coded storage type strings and normalise external URLs.
Using raw strings like `'youtube'` and `'vimeo'` for `storageType` makes this fragile to typos and harder to change later; if there’s an existing enum/constant set for storage types, please reuse it here. Also, consider normalising the generated URLs to the conventional forms `https://www.youtube.com/watch?v=...` and `https://vimeo.com/...` rather than mixing domains/subdomains.
Suggested implementation:
```
public static function getDetailUrl(string $shardingFolderName, string $url, string $storageType): string
{
// map known storage types to their canonical external URL bases
$externalUrlBaseMap = [
'youtube' => 'https://www.youtube.com/watch?v=',
'vimeo' => 'https://vimeo.com/',
];
if (array_key_exists($storageType, $externalUrlBaseMap)) {
$link = $externalUrlBaseMap[$storageType] . $url;
} else {
$link = '/src/Frontend/Files/MediaLibrary/' . $shardingFolderName . '/' . $url;
}
$html = '<a href="' . $link . '" target="_blank" class="btn btn-default btn-xs pull-right">';
$html .= '<span class="fa fa-eye"></span>' . ucfirst(Language::lbl('View'));
$html .= '</a>';
return $html;
}
```
To fully address the concern about hard-coded storage type strings, you should:
1. Replace the `'youtube'` and `'vimeo'` keys in `$externalUrlBaseMap` with the appropriate enum/constant values already used in the MediaLibrary domain (for example, something like `MediaItem::STORAGE_TYPE_YOUTUBE` / `MediaItem::STORAGE_TYPE_VIMEO` or a dedicated `StorageType` enum).
2. Import that enum/constant provider at the top of this file with a `use` statement if needed.
3. Optionally, extract `$externalUrlBaseMap` into a dedicated constant or method so that storage type → URL mappings are defined in a single, reusable place.
</issue_to_address>
### Comment 2
<location path="src/Backend/Modules/MediaLibrary/Domain/MediaItem/MediaItemDataGrid.php" line_range="225-231" />
<code_context>
+ $link = 'https://youtube.com/watch?v=' . $url;
+ } elseif ($storageType === 'vimeo') {
+ $link = 'https://www.vimeo.com/' . $url;
+ } else {
+ $link = '/src/Frontend/Files/MediaLibrary/' . $shardingFolderName . '/' . $url;
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Consider using a generated/public-facing URL instead of a hard-coded path into `src/Frontend`.
Hard-coding `/src/Frontend/Files/MediaLibrary/` couples this link to the current directory structure and will likely break if the public path or deployment layout changes. Prefer using an existing helper/config value to build media URLs (or adding one if missing) instead of embedding the path directly.
```suggestion
if ($storageType === 'youtube') {
$link = 'https://youtube.com/watch?v=' . $url;
} elseif ($storageType === 'vimeo') {
$link = 'https://www.vimeo.com/' . $url;
} else {
$link = FRONTEND_FILES_URL . '/MediaLibrary/' . $shardingFolderName . '/' . $url;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+223
to
+227
| public static function getDetailUrl(string $shardingFolderName, string $url, string $storageType): string | ||
| { | ||
| if ($storageType === 'youtube') { | ||
| $link = 'https://youtube.com/watch?v=' . $url; | ||
| } elseif ($storageType === 'vimeo') { |
There was a problem hiding this comment.
suggestion: Avoid hard-coded storage type strings and normalise external URLs.
Using raw strings like 'youtube' and 'vimeo' for storageType makes this fragile to typos and harder to change later; if there’s an existing enum/constant set for storage types, please reuse it here. Also, consider normalising the generated URLs to the conventional forms https://www.youtube.com/watch?v=... and https://vimeo.com/... rather than mixing domains/subdomains.
Suggested implementation:
public static function getDetailUrl(string $shardingFolderName, string $url, string $storageType): string
{
// map known storage types to their canonical external URL bases
$externalUrlBaseMap = [
'youtube' => 'https://www.youtube.com/watch?v=',
'vimeo' => 'https://vimeo.com/',
];
if (array_key_exists($storageType, $externalUrlBaseMap)) {
$link = $externalUrlBaseMap[$storageType] . $url;
} else {
$link = '/src/Frontend/Files/MediaLibrary/' . $shardingFolderName . '/' . $url;
}
$html = '<a href="' . $link . '" target="_blank" class="btn btn-default btn-xs pull-right">';
$html .= '<span class="fa fa-eye"></span>' . ucfirst(Language::lbl('View'));
$html .= '</a>';
return $html;
}
To fully address the concern about hard-coded storage type strings, you should:
- Replace the
'youtube'and'vimeo'keys in$externalUrlBaseMapwith the appropriate enum/constant values already used in the MediaLibrary domain (for example, something likeMediaItem::STORAGE_TYPE_YOUTUBE/MediaItem::STORAGE_TYPE_VIMEOor a dedicatedStorageTypeenum). - Import that enum/constant provider at the top of this file with a
usestatement if needed. - Optionally, extract
$externalUrlBaseMapinto a dedicated constant or method so that storage type → URL mappings are defined in a single, reusable place.
Comment on lines
+225
to
+231
| if ($storageType === 'youtube') { | ||
| $link = 'https://youtube.com/watch?v=' . $url; | ||
| } elseif ($storageType === 'vimeo') { | ||
| $link = 'https://www.vimeo.com/' . $url; | ||
| } else { | ||
| $link = '/src/Frontend/Files/MediaLibrary/' . $shardingFolderName . '/' . $url; | ||
| } |
There was a problem hiding this comment.
suggestion: Consider using a generated/public-facing URL instead of a hard-coded path into src/Frontend.
Hard-coding /src/Frontend/Files/MediaLibrary/ couples this link to the current directory structure and will likely break if the public path or deployment layout changes. Prefer using an existing helper/config value to build media URLs (or adding one if missing) instead of embedding the path directly.
Suggested change
| if ($storageType === 'youtube') { | |
| $link = 'https://youtube.com/watch?v=' . $url; | |
| } elseif ($storageType === 'vimeo') { | |
| $link = 'https://www.vimeo.com/' . $url; | |
| } else { | |
| $link = '/src/Frontend/Files/MediaLibrary/' . $shardingFolderName . '/' . $url; | |
| } | |
| if ($storageType === 'youtube') { | |
| $link = 'https://youtube.com/watch?v=' . $url; | |
| } elseif ($storageType === 'vimeo') { | |
| $link = 'https://www.vimeo.com/' . $url; | |
| } else { | |
| $link = FRONTEND_FILES_URL . '/MediaLibrary/' . $shardingFolderName . '/' . $url; | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add a detail/view action to media library items in the backend grid.
New Features:
Enhancements: