Conversation
📝 WalkthroughWalkthroughThis PR introduces a new "Backup Templates" plugin that allows per-server backup configuration management. It includes database migrations, Filament UI resources, Eloquent models, authorization policies, English and German translations, and service provider integration for template selection within backup forms. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Filament UI
participant Policy as Authorization<br/>Policy
participant Model as BackupTemplate<br/>Model
participant Backup as Backup<br/>Form
participant DB as Database
User->>UI: Access Backup Templates List
UI->>Policy: Check canAccess()
Policy->>UI: Verify SubuserPermission::BackupRead
UI->>DB: Load Templates for Server
DB->>UI: Return Templates
User->>Backup: Create/Edit Backup
Backup->>Model: Load Available Templates
Model->>DB: Query backupTemplates by server_id
DB->>Model: Return Templates
Model->>Backup: Display Template Select
User->>Backup: Select Template
Backup->>Model: Fetch Template Details
Model->>DB: Query Template.ignored
DB->>Model: Return ignored Paths
Backup->>Backup: Auto-populate Ignored Paths
User->>Backup: Submit Backup Config
Backup->>Policy: Check create Permission
Policy->>UI: Authorize via backupTemplates.create
UI->>DB: Save Backup with template_id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
backup-templates/plugin.json (1)
16-16: Changecomposer_packagesfrom empty array to null for consistency.The backup-templates plugin uses
[]while all other plugins in the codebase use eithernull(19 plugins) or an object with packages (4 plugins). Using an empty array is inconsistent with the established pattern and should be changed tonullto match plugins without dependencies.Proposed fix
- "composer_packages": [] + "composer_packages": null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backup-templates/plugin.json` at line 16, Update the backup-templates plugin manifest by changing the composer_packages value from an empty array to null so it matches the established pattern used by other plugins; locate the "composer_packages" key in the plugin.json for backup-templates and replace [] with null.backup-templates/src/Providers/BackupTemplatesPluginProvider.php (1)
79-93: Consider making the supported locales configurable or extensible.The translation loading hard-codes
['en', 'de']locales. If the host application uses other locales, users won't see translated permission strings.♻️ Suggested approach to support additional locales
- foreach (['en', 'de'] as $locale) { + $locales = array_filter( + ['en', 'de'], // supported by this plugin + fn ($locale) => is_dir("$pluginPath/lang/$locale") + ); + foreach ($locales as $locale) {Alternatively, dynamically scan the
langdirectory for available locales.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backup-templates/src/Providers/BackupTemplatesPluginProvider.php` around lines 79 - 93, The code currently hard-codes ['en','de'] when loading translations; change it to obtain supported locales from configuration (e.g. config('backup_templates.locales')) with a fallback to dynamically scanning the lang directory at "$pluginPath/lang" (list directories and use their names as locales) before the foreach; keep the rest of the logic (variables $pluginPath, $langFile, $coreLangFile, $coreTranslations, $pluginTranslations, $translations and Lang::addLines) the same so any locale found is processed exactly as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backup-templates/lang/de/server/user.php`:
- Around line 3-7: The permissions array uses flat dot-notation keys but should
be nested to match the policy checks; inside the 'permissions' array replace the
flat keys 'backupTemplates_desc' and 'backupTemplates_create' with a nested
'backupTemplates' array containing 'desc' and 'create' entries (i.e.,
'permissions' => ['backupTemplates' => ['desc' => ..., 'create' => ...]]),
keeping the existing German strings as values so authorization lookups that
expect 'backupTemplates.desc' and 'backupTemplates.create' resolve correctly.
In `@backup-templates/lang/en/server/user.php`:
- Around line 3-7: The translation keys use underscores (e.g.,
'backupTemplates_create') but the permission checks expect dot notation
('backupTemplates.create'), so update the array in user.php to use nested arrays
(e.g., 'backupTemplates' => ['create' => ... , 'desc' => ...]) so that
Arr::dot() (referenced in BackupTemplatesPluginProvider::87) produces
dot-notated keys; change the keys 'backupTemplates_desc' and
'backupTemplates_create' to nested entries under 'backupTemplates' to match the
policy lookups.
In
`@backup-templates/src/Filament/Server/Resources/BackupTemplates/BackupTemplateResource.php`:
- Around line 95-102: getEloquentQuery currently uses when($server, ...) so if
Filament::getTenant() returns null the query returns all records; change
getEloquentQuery (the method) so it always constrains results to a tenant — use
Filament::getTenant() into $server and, if $server is present apply
where('server_id', $server->id), otherwise force an empty result set (e.g. add a
fallback that restricts to no rows) to prevent accidental cross-tenant exposure
instead of leaving the query unfiltered.
- Around line 9-10: The imports for DeleteAction and EditAction are using the
wrong namespace; update the top of BackupTemplateResource.php to import
DeleteAction and EditAction from Filament\Tables\Actions (not Filament\Actions)
so the actions used in the recordActions() call (see recordActions method around
where DeleteAction and EditAction are referenced) resolve to the table record
action classes.
In `@README.md`:
- Line 48: Change the heading text "Third party plugins" in the README (the
string "_* Third party plugins_" from the diff) to use a hyphenated compound
adjective: replace it with "Third-party plugins" so the heading reads correctly.
---
Nitpick comments:
In `@backup-templates/plugin.json`:
- Line 16: Update the backup-templates plugin manifest by changing the
composer_packages value from an empty array to null so it matches the
established pattern used by other plugins; locate the "composer_packages" key in
the plugin.json for backup-templates and replace [] with null.
In `@backup-templates/src/Providers/BackupTemplatesPluginProvider.php`:
- Around line 79-93: The code currently hard-codes ['en','de'] when loading
translations; change it to obtain supported locales from configuration (e.g.
config('backup_templates.locales')) with a fallback to dynamically scanning the
lang directory at "$pluginPath/lang" (list directories and use their names as
locales) before the foreach; keep the rest of the logic (variables $pluginPath,
$langFile, $coreLangFile, $coreTranslations, $pluginTranslations, $translations
and Lang::addLines) the same so any locale found is processed exactly as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb93d5f0-d4f5-44f9-8bb3-2af5ffaccd8d
📒 Files selected for processing (15)
README.mdbackup-templates/LICENSEbackup-templates/README.mdbackup-templates/database/migrations/001_create_backup_templates_table.phpbackup-templates/lang/de/server/user.phpbackup-templates/lang/de/strings.phpbackup-templates/lang/en/server/user.phpbackup-templates/lang/en/strings.phpbackup-templates/plugin.jsonbackup-templates/src/BackupTemplatesPlugin.phpbackup-templates/src/Filament/Server/Resources/BackupTemplates/BackupTemplateResource.phpbackup-templates/src/Filament/Server/Resources/BackupTemplates/Pages/ListBackupTemplates.phpbackup-templates/src/Models/BackupTemplate.phpbackup-templates/src/Policies/BackupTemplatePolicy.phpbackup-templates/src/Providers/BackupTemplatesPluginProvider.php
📜 Review details
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~48-~48: Use a hyphen to join words.
Context: ...s to create their own servers * Third party plugins ## Themes - [Fluffy The...
(QB_NEW_EN_HYPHEN)
🪛 PHPMD (2.15.0)
backup-templates/src/BackupTemplatesPlugin.php
[warning] 22-22: Avoid unused parameters such as '$panel'. (undefined)
(UnusedFormalParameter)
backup-templates/src/Policies/BackupTemplatePolicy.php
[warning] 17-17: Avoid unused parameters such as '$model'. (undefined)
(UnusedFormalParameter)
[warning] 27-27: Avoid unused parameters such as '$model'. (undefined)
(UnusedFormalParameter)
[warning] 32-32: Avoid unused parameters such as '$model'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (10)
backup-templates/LICENSE (1)
1-674: LGTM!Standard GNU GPL v3 license text.
backup-templates/README.md (1)
1-12: LGTM!Clear and concise documentation describing the plugin's purpose and features.
backup-templates/lang/de/strings.php (1)
1-22: LGTM!German translations are well-structured with appropriate labels and help text.
backup-templates/src/Models/BackupTemplate.php (1)
16-28: LGTM!Model correctly defines the
BelongsTorelationship and fillable fields. The default table name (backup_templates) follows Laravel conventions and should match the migration.backup-templates/database/migrations/001_create_backup_templates_table.php (1)
1-30: LGTM!The migration follows the established pattern used by other plugins (billing, subdomains) for the
server_idforeign key. The composite unique index on(server_id, name)appropriately prevents duplicate template names per server.backup-templates/src/Policies/BackupTemplatePolicy.php (1)
10-36: LGTM!The tenant-based authorization pattern is consistent with other Filament resources. The unused
$modelparameters are acceptable since the policy relies onFilament::getTenant()for server context, and the resource'sgetEloquentQuery()already scopes records to the current server.backup-templates/lang/en/strings.php (1)
1-22: LGTM!The translation structure aligns with all usages in
BackupTemplateResource.phpandBackupTemplatesPluginProvider.php. Thetemplatekey correctly uses pipe syntax fortrans_choice()pluralization.backup-templates/src/BackupTemplatesPlugin.php (1)
1-23: LGTM!The implementation correctly follows the established plugin pattern used by the tickets plugin, with dynamic resource discovery based on panel ID. The empty
boot()method satisfies thePlugininterface contract.backup-templates/src/Filament/Server/Resources/BackupTemplates/Pages/ListBackupTemplates.php (1)
20-42: LGTM!The
CreateActioncorrectly authorizes using the custom permissionbackupTemplates.createagainst the tenant server, and properly injectsserver_idinto the form data to scope new templates to the current server.backup-templates/src/Providers/BackupTemplatesPluginProvider.php (1)
50-69: Document the requiredignoredfield name in BackupResource.The plugin assumes the host application's
BackupResourceform has a field namedignored. While the README describes the feature ("fills the ignored paths field"), it doesn't explicitly specify this field name. Add documentation clarifying that BackupResource must include anignoredform field for the template selection to populate the ignored paths correctly.
| use Filament\Actions\DeleteAction; | ||
| use Filament\Actions\EditAction; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Filament v3 table recordActions EditAction namespace
💡 Result:
In Filament v3, the table “Edit” row action class is:
use Filament\Tables\Actions\EditAction;Use it in your table like:
->actions([
EditAction::make(),
])(For non-table actions, the namespace is Filament\Actions\EditAction.) [1][2]
🏁 Script executed:
cat -n backup-templates/src/Filament/Server/Resources/BackupTemplates/BackupTemplateResource.phpRepository: pelican-dev/plugins
Length of output: 4017
Use correct namespace for table record actions.
Lines 9-10 import from Filament\Actions, but since these actions are used in ->recordActions() (line 82-85), they should import from Filament\Tables\Actions instead:
use Filament\Tables\Actions\DeleteAction;
use Filament\Tables\Actions\EditAction;The Filament\Actions namespace is for page-level header actions, not table record actions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backup-templates/src/Filament/Server/Resources/BackupTemplates/BackupTemplateResource.php`
around lines 9 - 10, The imports for DeleteAction and EditAction are using the
wrong namespace; update the top of BackupTemplateResource.php to import
DeleteAction and EditAction from Filament\Tables\Actions (not Filament\Actions)
so the actions used in the recordActions() call (see recordActions method around
where DeleteAction and EditAction are referenced) resolve to the table record
action classes.
| public static function getEloquentQuery(): Builder | ||
| { | ||
| /** @var Server|null $server */ | ||
| $server = Filament::getTenant(); | ||
|
|
||
| return parent::getEloquentQuery() | ||
| ->when($server, fn (Builder $query) => $query->where('server_id', $server->id)); | ||
| } |
There was a problem hiding this comment.
Potential data exposure if query executed without tenant.
The when($server, ...) conditional means all records are returned if $server is null. While canAccess() gates resource entry, if getEloquentQuery() is invoked through other code paths, it could expose templates from all servers.
🛡️ Proposed defensive fix
public static function getEloquentQuery(): Builder
{
/** `@var` Server|null $server */
$server = Filament::getTenant();
+ if (!$server) {
+ return parent::getEloquentQuery()->whereRaw('1 = 0');
+ }
+
return parent::getEloquentQuery()
- ->when($server, fn (Builder $query) => $query->where('server_id', $server->id));
+ ->where('server_id', $server->id);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backup-templates/src/Filament/Server/Resources/BackupTemplates/BackupTemplateResource.php`
around lines 95 - 102, getEloquentQuery currently uses when($server, ...) so if
Filament::getTenant() returns null the query returns all records; change
getEloquentQuery (the method) so it always constrains results to a tenant — use
Filament::getTenant() into $server and, if $server is present apply
where('server_id', $server->id), otherwise force an empty result set (e.g. add a
fallback that restricts to no rows) to prevent accidental cross-tenant exposure
instead of leaving the query unfiltered.
|
Thanks, but new third party plugins will not be accepted because of the upcoming hub marketplace. |
|
Ok, so i will have to host the PocketID Provider in my own repo too? |
|
Ideally, yes. The third party plugins in the repo will most likely be removed when the hub marketplace is goes live. |
2 participants
This plugin adds per-server backup templates that can be selected when creating a backup. Selecting a template automatically fills the ignored paths field with the saved preset.
Summary by CodeRabbit