Repository navigation
Enhancement: Build template parts system (header, footer, sidebar) - #198
Conversation
Implement reusable template parts that can be shared across multiple templates. Adds TemplatePart model with area-based filtering (header, footer, sidebar, custom), TemplatePartManager service with in-memory registry and database-backed CRUD, facade, helper functions, config, and default built-in parts registered via the service provider. Includes 40 tests covering model scopes, service operations, revision tracking, locking, duplication, seeding, and duplicate slug validation. Closes #161 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a template parts system: config, DB migration, Eloquent model, manager service + facade + helpers, service provider registration of defaults, and unit tests covering model and service behavior. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Helpers as Helpers
participant Facade as TemplatePartManager<br/>(Facade)
participant Service as TemplatePartManager<br/>(Service)
participant Registry as In-Memory<br/>Registry
participant DB as Database<br/>(TemplatePart Model)
participant Hooks as Action Hooks
App->>Helpers: veRegisterTemplatePart(slug, config)
Helpers->>Facade: register(slug, config)
Facade->>Service: register(slug, config)
Service->>Registry: store registered definition
App->>Helpers: veGetTemplatePartsForArea(area)
Helpers->>Facade: forArea(area)
Facade->>Service: forArea(area)
Service->>Registry: fetch registered for area
Service->>DB: query active parts for area
Service->>Service: merge DB (override) + registry, apply filters
Service-->>Facade: return merged array
Facade-->>Helpers: return to caller
Helpers-->>App: parts array
App->>Facade: create(data)
Facade->>Service: create(data)
Service->>DB: check slug uniqueness, validate area
Service->>DB: persist TemplatePart
Service->>Hooks: trigger ap.visualEditor.templatePartCreated
Service-->>Facade: return TemplatePart
Facade-->>App: created part
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/VisualEditorServiceProvider.php (1)
498-520: Consider honoring configured template-part areas when seeding defaults.Lines 498–520 always register
header,footer, andsidebar. If a project narrowsartisanpack.visual-editor.template_parts.areas, this can register parts outside its configured area set.💡 Proposed refactor
protected function registerDefaultTemplateParts(): void { $manager = $this->app->make( 'visual-editor.template-parts' ); + $enabledAreas = (array) config( + 'artisanpack.visual-editor.template_parts.areas', + [ 'header', 'footer', 'sidebar', 'custom' ] + ); - $manager->register( 'header', [ - 'name' => __( 'Header' ), - 'description' => __( 'A default header area for site-wide navigation and branding.' ), - 'area' => 'header', - 'content' => [], - 'is_custom' => false, - ] ); + if ( in_array( 'header', $enabledAreas, true ) ) { + $manager->register( 'header', [ + 'name' => __( 'Header' ), + 'description' => __( 'A default header area for site-wide navigation and branding.' ), + 'area' => 'header', + 'content' => [], + 'is_custom' => false, + ] ); + } @@ - $manager->register( 'footer', [ + if ( in_array( 'footer', $enabledAreas, true ) ) { + $manager->register( 'footer', [ 'name' => __( 'Footer' ), 'description' => __( 'A default footer area for site-wide links and information.' ), 'area' => 'footer', 'content' => [], 'is_custom' => false, - ] ); + ] ); + } @@ - $manager->register( 'sidebar', [ + if ( in_array( 'sidebar', $enabledAreas, true ) ) { + $manager->register( 'sidebar', [ 'name' => __( 'Sidebar' ), 'description' => __( 'A default sidebar area for supplementary content and widgets.' ), 'area' => 'sidebar', 'content' => [], 'is_custom' => false, - ] ); + ] ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/VisualEditorServiceProvider.php` around lines 498 - 520, The code always registers 'header', 'footer', and 'sidebar' via $manager->register which ignores the configured areas; update the seeding logic to read the configured array at config('artisanpack.visual-editor.template_parts.areas') and only call $manager->register for each default part whose 'area' exists in that configured array (use the existing 'header','footer','sidebar' definitions as templates), so registration honors the template_part areas configuration.src/Models/TemplatePart.php (1)
212-233: Consider extracting thetemplate_partdocument type into a constant.Line 215 and Line 232 repeat the same literal; a constant would reduce drift risk.
💡 Proposed refactor
class TemplatePart extends Model { + public const REVISION_DOCUMENT_TYPE = 'template_part'; @@ public function createRevision( ?int $userId = null ): Revision { return Revision::create( [ - 'document_type' => 'template_part', + 'document_type' => self::REVISION_DOCUMENT_TYPE, 'document_id' => $this->id, @@ public function revisions(): \Illuminate\Database\Eloquent\Collection { - return Revision::forDocument( 'template_part', $this->id ) + return Revision::forDocument( self::REVISION_DOCUMENT_TYPE, $this->id ) ->orderByDesc( 'created_at' ) ->get(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Models/TemplatePart.php` around lines 212 - 233, Extract the repeated literal 'template_part' into a class constant on TemplatePart (e.g. protected const DOCUMENT_TYPE = 'template_part') and replace both uses in createRevision() and revisions() with self::DOCUMENT_TYPE; update the Revision::create call and the Revision::forDocument(...) invocation to use that constant so the document type is defined in one place.src/Services/TemplatePartManager.php (2)
172-183: Consider consistent return type forresolve().Returning either
TemplatePart(model) orarray(from registry) requires callers to check the type before using the result. Consider converting to a consistent type (e.g., always return array via$dbPart->toArray(), or always wrap registered arrays in a TemplatePart-like structure).Option: Always return array for consistency
public function resolve( string $slug ): TemplatePart|array|null { $dbPart = TemplatePart::where( 'slug', $slug )->first(); if ( null !== $dbPart ) { - return $dbPart; + return $dbPart->toArray(); } return $this->registered[ $slug ] ?? null; }This would require updating the return type to
array|nulland adjusting callers that rely on model methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Services/TemplatePartManager.php` around lines 172 - 183, The resolve() method currently returns mixed types (TemplatePart model or array) which forces callers to branch; update resolve() to consistently return array|null by converting any TemplatePart model ($dbPart) to an array with $dbPart->toArray() and returning the registered entry ($this->registered[$slug]) as-is, and change the method signature/return type accordingly (TemplatePart->toArray(), resolve(): array|null) so all callers can expect an array or null and be updated to stop using model methods on the result.
297-304: Consider validatingnewSluguniqueness before delegating to model.The
duplicate()method delegates directly to$part->duplicate()without checking if$newSlugalready exists. While the model may handle this, validating upfront provides a clearer error message and consistent behavior withcreate().Proposed fix: Add slug existence check
public function duplicate( TemplatePart $part, string $newSlug, ?string $newName = null ): TemplatePart { + if ( $this->exists( $newSlug ) ) { + throw new RuntimeException( + __( 'A template part with the slug ":slug" already exists.', [ 'slug' => $newSlug ] ), + ); + } + $duplicate = $part->duplicate( $newSlug, $newName ); veDoAction( 'ap.visualEditor.templatePartDuplicated', $duplicate, $part ); return $duplicate; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Services/TemplatePartManager.php` around lines 297 - 304, Validate that the requested new slug is not already in use before calling $part->duplicate in TemplatePartManager::duplicate: perform a uniqueness check (e.g., via TemplatePart::where('slug', $newSlug)->exists() or an existing repository/method like TemplatePart::slugExists($newSlug)), throw or return a clear validation error if the slug exists, and only call $part->duplicate($newSlug, $newName) when the slug is unique to match the create() behavior and provide a clearer error message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@database/migrations/2024_01_01_000004_create_visual_editor_template_parts_table.php`:
- Around line 37-40: The migration hardcodes the foreign key to 'users', which
conflicts with the configurable user model used by TemplatePart; update the
migration (in the create_visual_editor_template_parts_table up method) to
resolve the user table dynamically by reading the configured user model
(config('artisanpack.visual-editor.user_model')), instantiate it (or use
forward_static_call to get its table) and use that table name in the foreign key
definition instead of 'users' (i.e., $userTable = (new $userClass)->getTable();
then ->references('id')->on($userTable)->nullOnDelete()); keep the rest of the
column/index logic unchanged.
In `@src/Services/TemplatePartManager.php`:
- Around line 99-110: The veApplyFilters call returns mixed which can violate
the declared array return type of TemplatePartManager::all(); update all(),
allActive(), and forArea() to validate/cast the filter result: call
veApplyFilters('ap.visualEditor.templateParts', $parts) (and the corresponding
filter keys), check is_array on the result and if not an array fall back to the
original $parts (or cast to (array) safely), then return that validated array so
the method's array<string,array<string,mixed>> contract is preserved.
- Around line 243-260: When updating a TemplatePart in update( TemplatePart
$part, array $data, ?int $userId = null ) ensure slug uniqueness: if
$data['slug'] is set and differs from $part->slug, check the registry and the
database for existing TemplatePart records (excluding the current $part ID) and
throw a RuntimeException if a duplicate exists; perform this check before
calling $part->update($data) (but after the existing is_locked/content revision
checks) so createRevision($userId) and veDoAction remain unchanged.
- Around line 212-225: The create method in TemplatePartManager currently checks
duplicates with TemplatePart::where(...)->exists(), which only queries the DB
and ignores programmatically registered parts; replace that check by calling
TemplatePart::exists($slug) (or the existing exists() helper) to ensure both DB
and in-memory registry are considered, and throw the same RuntimeException when
exists() returns true; additionally validate the incoming $data['area'] against
TemplatePart::AREAS (or the configured areas list) before calling
TemplatePart::create and throw a clear exception if the area is missing/invalid
to prevent persisting invalid area values.
---
Nitpick comments:
In `@src/Models/TemplatePart.php`:
- Around line 212-233: Extract the repeated literal 'template_part' into a class
constant on TemplatePart (e.g. protected const DOCUMENT_TYPE = 'template_part')
and replace both uses in createRevision() and revisions() with
self::DOCUMENT_TYPE; update the Revision::create call and the
Revision::forDocument(...) invocation to use that constant so the document type
is defined in one place.
In `@src/Services/TemplatePartManager.php`:
- Around line 172-183: The resolve() method currently returns mixed types
(TemplatePart model or array) which forces callers to branch; update resolve()
to consistently return array|null by converting any TemplatePart model ($dbPart)
to an array with $dbPart->toArray() and returning the registered entry
($this->registered[$slug]) as-is, and change the method signature/return type
accordingly (TemplatePart->toArray(), resolve(): array|null) so all callers can
expect an array or null and be updated to stop using model methods on the
result.
- Around line 297-304: Validate that the requested new slug is not already in
use before calling $part->duplicate in TemplatePartManager::duplicate: perform a
uniqueness check (e.g., via TemplatePart::where('slug', $newSlug)->exists() or
an existing repository/method like TemplatePart::slugExists($newSlug)), throw or
return a clear validation error if the slug exists, and only call
$part->duplicate($newSlug, $newName) when the slug is unique to match the
create() behavior and provide a clearer error message.
In `@src/VisualEditorServiceProvider.php`:
- Around line 498-520: The code always registers 'header', 'footer', and
'sidebar' via $manager->register which ignores the configured areas; update the
seeding logic to read the configured array at
config('artisanpack.visual-editor.template_parts.areas') and only call
$manager->register for each default part whose 'area' exists in that configured
array (use the existing 'header','footer','sidebar' definitions as templates),
so registration honors the template_part areas configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9da2ca94-9876-4a9e-9359-7b192f85a888
📒 Files selected for processing (9)
config/visual-editor.phpdatabase/migrations/2024_01_01_000004_create_visual_editor_template_parts_table.phpsrc/Facades/TemplatePartManager.phpsrc/Models/TemplatePart.phpsrc/Services/TemplatePartManager.phpsrc/VisualEditorServiceProvider.phpsrc/helpers.phptests/Unit/Models/TemplatePartTest.phptests/Unit/Services/TemplatePartManagerTest.php
| $table->unsignedBigInteger( 'user_id' )->nullable(); | ||
| $table->timestamps(); | ||
| $table->foreign( 'user_id' )->references( 'id' )->on( 'users' )->nullOnDelete(); | ||
| $table->index( [ 'area', 'status' ] ); |
There was a problem hiding this comment.
Hard-coded users FK conflicts with configurable user model table.
Line 39 pins the foreign key to users, but the model resolves the user class via config('artisanpack.visual-editor.user_model') (see src/Models/TemplatePart.php Line 198). In apps with a custom user table, this can break migration integrity.
💡 Proposed fix
Schema::create( 'visual_editor_template_parts', function ( Blueprint $table ): void {
$table->id();
@@
$table->boolean( 'is_custom' )->default( false );
$table->boolean( 'is_locked' )->default( false );
$table->unsignedBigInteger( 'user_id' )->nullable();
$table->timestamps();
- $table->foreign( 'user_id' )->references( 'id' )->on( 'users' )->nullOnDelete();
+ $userModelClass = config( 'artisanpack.visual-editor.user_model', 'App\\Models\\User' );
+ $userTable = class_exists( $userModelClass ) ? ( new $userModelClass() )->getTable() : 'users';
+ $table->foreign( 'user_id' )->references( 'id' )->on( $userTable )->nullOnDelete();
$table->index( [ 'area', 'status' ] );
} );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $table->unsignedBigInteger( 'user_id' )->nullable(); | |
| $table->timestamps(); | |
| $table->foreign( 'user_id' )->references( 'id' )->on( 'users' )->nullOnDelete(); | |
| $table->index( [ 'area', 'status' ] ); | |
| $table->unsignedBigInteger( 'user_id' )->nullable(); | |
| $table->timestamps(); | |
| $userModelClass = config( 'artisanpack.visual-editor.user_model', 'App\\Models\\User' ); | |
| $userTable = class_exists( $userModelClass ) ? ( new $userModelClass() )->getTable() : 'users'; | |
| $table->foreign( 'user_id' )->references( 'id' )->on( $userTable )->nullOnDelete(); | |
| $table->index( [ 'area', 'status' ] ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@database/migrations/2024_01_01_000004_create_visual_editor_template_parts_table.php`
around lines 37 - 40, The migration hardcodes the foreign key to 'users', which
conflicts with the configurable user model used by TemplatePart; update the
migration (in the create_visual_editor_template_parts_table up method) to
resolve the user table dynamically by reading the configured user model
(config('artisanpack.visual-editor.user_model')), instantiate it (or use
forward_static_call to get its table) and use that table name in the foreign key
definition instead of 'users' (i.e., $userTable = (new $userClass)->getTable();
then ->references('id')->on($userTable)->nullOnDelete()); keep the rest of the
column/index logic unchanged.
- create() now checks both DB and registry for duplicate slugs and validates area against configured areas list - update() validates slug uniqueness when changing slugs - duplicate() validates new slug uniqueness before duplicating - veApplyFilters return values are type-checked to preserve array contract - Default template part registration respects configured areas list - seedRegistered() bypasses registry check since it intentionally persists registered items to DB - Added 6 new tests for validation edge cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Services/TemplatePartManager.php (1)
386-412: Consider validating area before seeding to database.The
seedRegistered()method persists registered template parts to the database without validating theareafield against configured areas. If a programmatically registered part has an invalid area (e.g., via third-party code usingregister()), it would be persisted to the database, bypassing the validation thatcreate()enforces.This may be intentional if you trust programmatic registrations, but it creates an inconsistency with
create().🔧 Optional: Add area validation to seedRegistered()
public function seedRegistered(): array { $created = []; + $validAreas = (array) config( 'artisanpack.visual-editor.template_parts.areas', TemplatePart::AREAS ); foreach ( $this->registered as $slug => $config ) { if ( TemplatePart::where( 'slug', $slug )->exists() ) { continue; } + $area = $config['area'] ?? 'custom'; + if ( ! in_array( $area, $validAreas, true ) ) { + continue; // or throw, depending on desired strictness + } + $part = TemplatePart::create( [ 'name' => $config['name'], 'slug' => $slug, 'description' => $config['description'] ?? null, - 'area' => $config['area'] ?? 'custom', + 'area' => $area,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Services/TemplatePartManager.php` around lines 386 - 412, seedRegistered() persists registered parts without validating the 'area' value, causing inconsistency with the validation enforced by TemplatePart::create(); update seedRegistered() to validate each $config['area'] against the allowed areas (the same source used by create()/register()), and if invalid either skip the entry or normalize it to a safe default (e.g., 'custom') before calling TemplatePart::create() (or delegate to the create() helper) so persisted rows always respect the same area rules as runtime creations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Services/TemplatePartManager.php`:
- Around line 386-412: seedRegistered() persists registered parts without
validating the 'area' value, causing inconsistency with the validation enforced
by TemplatePart::create(); update seedRegistered() to validate each
$config['area'] against the allowed areas (the same source used by
create()/register()), and if invalid either skip the entry or normalize it to a
safe default (e.g., 'custom') before calling TemplatePart::create() (or delegate
to the create() helper) so persisted rows always respect the same area rules as
runtime creations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef33e667-d38b-4cca-bd59-ae1b0c3561f6
📒 Files selected for processing (3)
src/Services/TemplatePartManager.phpsrc/VisualEditorServiceProvider.phptests/Unit/Services/TemplatePartManagerTest.php
Description
Implements reusable template parts that can be shared across multiple templates. Template parts represent common layout sections (headers, footers, sidebars) that are edited once and reflected everywhere they're used.
Closes: #161
Type of Change
Related Issue
Issue: #161
Motivation and Context
Templates need reusable sections (headers, footers, sidebars) that can be shared across multiple templates. Without template parts, every template would need to duplicate common sections, making maintenance difficult. This mirrors Gutenberg's template parts concept.
Changes Made
TemplatePartmodel with area-based filtering (header, footer, sidebar, custom), revision support, duplication, and lockingTemplatePartManagerservice with in-memory registry + database-backed CRUD, matching the existingTemplateManagerpatternTemplatePartManagerfacade for static accessvisual_editor_template_partstableveRegisterTemplatePart(),veGetTemplatePart(),veGetTemplatePartsForArea(),veTemplatePartExists()template_partsconfiguration section withallow_custom_parts,locked_parts, andareassettingsap.visualEditor.templatePartsInithook for third-party registrationTemplatePartManager::create()How Has This Been Tested?
Testing Environment:
Tests Performed:
Accessibility Tests Run
Details: N/A — this is a backend-only change (model, service, migration, facade, helpers) with no UI components.
Tests Added
Test details:
tests/Unit/Models/TemplatePartTest.php— 17 tests covering casts, defaults, fillable attributes, unique slug, all scopes (forArea, active, draft, custom, builtIn, byUser), revisions, duplication, and AREAS constanttests/Unit/Services/TemplatePartManagerTest.php— 23 tests covering register/unregister, defaults, all/allActive/forArea queries, resolve priority (DB > registry), existence checks, CRUD, revision on content change, locked part protection, duplication, lock/unlock, seeding, and duplicate slug validationDocumentation
Documentation details: Full PHPDoc blocks on all classes, methods, and properties following existing package conventions.
Pre-Submission Checklist
Summary by CodeRabbit
New Features
Tests