Repository navigation
feat: C4 · Navigation: model, migration, API, menu locations - #365
Conversation
Lands the server-side backing for the wp_navigation entity the B1
core-data shim addresses at /visual-editor/api/navigation. Adds the
VisualEditorNavigation Eloquent model, migration, CRUD controller,
store/update form requests (with TemplateBlockTreeRule for the
{raw, blocks} envelope), NavigationResource, and policy gating.
Menu locations are config-driven for V1 per the plan doc §8: a new
navigation.locations section in config/artisanpack/visual-editor.php
names theme-exposed slugs, and MenuLocationResolver::forLocation($slug)
falls back to the first published nav in menu_order when a location
has no assignment, points at a missing record, or points at an empty
block tree. Returns null when the database has no published navs.
Round-trip coverage includes B2's nested.json fixture to exercise
core/navigation-submenu with arbitrary innerBlocks depth.
core/navigation stays on disabled_blocks until D4/E4.
Closes #360
📝 WalkthroughWalkthroughImplements the server-side backing for Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/Feature/VisualEditor/MenuLocationResolverTest.php (1)
47-48: Avoid repeated resolver calls within one assertion chain.Each repeated
forLocation()call can hit the DB again; storing the resolved value once makes these tests cheaper and easier to read.Pattern to apply in these tests
- expect( $resolver->forLocation( 'primary' ) )->not->toBeNull() - ->and( $resolver->forLocation( 'primary' )->id )->toBe( $primary->id ); + $resolved = $resolver->forLocation( 'primary' ); + expect( $resolved )->not->toBeNull() + ->and( $resolved->id )->toBe( $primary->id );Also applies to: 65-67, 82-84, 107-109, 139-141, 150-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/VisualEditor/MenuLocationResolverTest.php` around lines 47 - 48, The test repeatedly calls $resolver->forLocation('primary') (and similar locations) inside a single assertion chain causing extra DB queries; change each test to call $resolver->forLocation(...) once, store the result in a local variable (e.g., $resolved = $resolver->forLocation('primary')), then assert on $resolved (not null and ->id equals $primary->id). Update all occurrences referenced in the comment (lines around the chains with forLocation, including the other ranges) to use the single-call pattern with a local variable.database/migrations/2026_04_23_300000_create_visual_editor_navigations_table.php (1)
37-38: Consider a composite index for the published-fallback read path.
MenuLocationResolver::fallback()filters bystatusand orders bymenu_order; a composite index can reduce sort/filter cost as this table grows.Proposed migration tweak
- $table->string( 'status' )->default( 'publish' )->index(); - $table->unsignedInteger( 'menu_order' )->default( 0 )->index(); + $table->string( 'status' )->default( 'publish' ); + $table->unsignedInteger( 'menu_order' )->default( 0 ); + $table->index( [ 'status', 'menu_order' ], 'visual_editor_nav_status_menu_order_idx' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/2026_04_23_300000_create_visual_editor_navigations_table.php` around lines 37 - 38, Add a composite index on (status, menu_order) in the migration that creates the visual editor navigations table to optimize MenuLocationResolver::fallback() queries that filter by status and order by menu_order; in the migration class CreateVisualEditorNavigationsTable (inside the up() method where $table->string('status')->... and $table->unsignedInteger('menu_order')->... are defined) add a combined index definition for status and menu_order so the DB can use one index for both filtering and ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Http/Requests/StoreNavigationRequest.php`:
- Around line 77-82: The envelopeShapeRule() closure currently lets an empty
list pass because of the "&& [] !== $value" check; remove that exemption so any
list-shaped array (including empty lists like content: []) is rejected. Update
the condition in envelopeShapeRule() (the Closure returned by envelopeShapeRule)
from "is_array($value) && [] !== $value && array_is_list($value)" to simply
"is_array($value) && array_is_list($value)" (or add an explicit check to fail
when $value is an empty list) so the rule rejects bare lists including empty
arrays and calls $fail() accordingly.
- Around line 29-32: The request currently always returns true in
StoreNavigationRequest::authorize(), allowing expensive validation (e.g.,
TemplateBlockTreeRule) to run before the controller's policy check; change
authorize() to perform the permission check (for example using
$this->user()->can('create', Navigation::class) or Gate::allows('create',
Navigation::class)) and return that boolean so unauthorized requests are
rejected before validation runs, and remove/move the duplicate policy check from
the controller to rely on the request authorize() method.
In `@src/Http/Requests/UpdateNavigationRequest.php`:
- Around line 79-84: The envelopeShapeRule closure currently allows an empty
bare list (content: []) because it excludes [] from the failure condition;
update the closure in envelopeShapeRule so that any list-form array—including an
empty array detected by array_is_list($value)—triggers the $fail call. Locate
the envelopeShapeRule() method and remove the special-case exclusion (the [] !==
$value check) so the validation fails for both non-empty and empty bare lists
with the existing error message.
- Around line 29-32: The authorize() method in UpdateNavigationRequest currently
returns true unconditionally; move the authorization check into this method so
the policy is enforced before validation. Replace the unconditional return with
a call to Gate::authorize(...) or use $this->user()->can(...) inside
UpdateNavigationRequest::authorize() to run the same permission check used in
the controller (matching the Gate::authorize() call there) and return true only
when authorized—otherwise throw/return false so Laravel returns 403 prior to
validation.
---
Nitpick comments:
In
`@database/migrations/2026_04_23_300000_create_visual_editor_navigations_table.php`:
- Around line 37-38: Add a composite index on (status, menu_order) in the
migration that creates the visual editor navigations table to optimize
MenuLocationResolver::fallback() queries that filter by status and order by
menu_order; in the migration class CreateVisualEditorNavigationsTable (inside
the up() method where $table->string('status')->... and
$table->unsignedInteger('menu_order')->... are defined) add a combined index
definition for status and menu_order so the DB can use one index for both
filtering and ordering.
In `@tests/Feature/VisualEditor/MenuLocationResolverTest.php`:
- Around line 47-48: The test repeatedly calls $resolver->forLocation('primary')
(and similar locations) inside a single assertion chain causing extra DB
queries; change each test to call $resolver->forLocation(...) once, store the
result in a local variable (e.g., $resolved =
$resolver->forLocation('primary')), then assert on $resolved (not null and ->id
equals $primary->id). Update all occurrences referenced in the comment (lines
around the chains with forLocation, including the other ranges) to use the
single-call pattern with a local variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa5b4e2b-29c5-42d0-b2b7-e582afebf144
📒 Files selected for processing (13)
config/visual-editor.phpdatabase/migrations/2026_04_23_300000_create_visual_editor_navigations_table.phproutes/api.phpsrc/Http/Controllers/NavigationController.phpsrc/Http/Requests/StoreNavigationRequest.phpsrc/Http/Requests/UpdateNavigationRequest.phpsrc/Http/Resources/NavigationResource.phpsrc/Models/VisualEditorNavigation.phpsrc/Policies/VisualEditorNavigationPolicy.phpsrc/Services/MenuLocationResolver.phpsrc/VisualEditorServiceProvider.phptests/Feature/VisualEditor/MenuLocationResolverTest.phptests/Feature/VisualEditor/NavigationControllerTest.php
| public function authorize(): bool | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Laravel FormRequest, does authorize() run before validation and before the controller action?
💡 Result:
Yes, in Laravel FormRequest, the authorize method runs before validation and before the controller action. When a FormRequest is type-hinted in a controller method, Laravel automatically calls validateResolved on it (via FormRequestServiceProvider) before injecting it into the controller and executing the action. This happens after middleware but before the controller runs. In validateResolved from ValidatesWhenResolvedTrait: 1. prepareForValidation (optional hook) 2. passesAuthorization → calls authorize if defined 3. If authorized, getValidatorInstance → creates validator using rules 4. Runs validation 5. If passes, passedValidation (optional hook) If authorize returns false, it throws AuthorizationException (403) before any validation occurs. Validation only runs if authorization passes. The entire process (auth + validation) completes before the controller action executes, as confirmed by official docs: "The incoming form request is validated before the controller method is called".
Citations:
- 1: https://laravel.com/docs/11.x/validation
- 2: https://laravel.com/docs/master/validation
- 3: https://laravel.com/docs/13.x/validation
- 4: https://github.com/laravel/framework/blob/10.x/src/Illuminate/Foundation/Http/FormRequest.php
- 5: https://github.com/laravel/framework/blob/5.8/src/Illuminate/Foundation/Http/FormRequest.php
- 6: https://github.com/laravel/framework/blob/10.x/src/Illuminate/Foundation/Providers/FormRequestServiceProvider.php
- 7: https://github.com/laravel/framework/blob/11.x/src/Illuminate/Foundation/Http/FormRequest.php
- 8: https://github.com/laravel/framework/blob/10.x/src/Illuminate/Validation/ValidatesWhenResolvedTrait.php
🏁 Script executed:
# Find and examine the NavigationController around line 113
find . -name "NavigationController.php" -type fRepository: ArtisanPack-UI/visual-editor
Length of output: 119
🏁 Script executed:
# Find and examine StoreNavigationRequest to see the rules
find . -name "StoreNavigationRequest.php" -type fRepository: ArtisanPack-UI/visual-editor
Length of output: 118
🏁 Script executed:
# Search for VisualEditorNavigation class
fd -t f "VisualEditorNavigation" --type fRepository: ArtisanPack-UI/visual-editor
Length of output: 155
🏁 Script executed:
# Search for TemplateBlockTreeRule
fd -t f "TemplateBlockTreeRule" --type fRepository: ArtisanPack-UI/visual-editor
Length of output: 107
🏁 Script executed:
# Read NavigationController around line 113
cat -n ./src/Http/Controllers/NavigationController.php | sed -n '100,130p'Repository: ArtisanPack-UI/visual-editor
Length of output: 1188
🏁 Script executed:
# Read StoreNavigationRequest - focus on authorize and rules
cat -n ./src/Http/Requests/StoreNavigationRequest.phpRepository: ArtisanPack-UI/visual-editor
Length of output: 3063
🏁 Script executed:
# Check VisualEditorNavigationPolicy for the create method
cat -n ./src/Policies/VisualEditorNavigationPolicy.phpRepository: ArtisanPack-UI/visual-editor
Length of output: 2030
🏁 Script executed:
# Check TemplateBlockTreeRule to understand its cost
cat -n ./src/Rules/TemplateBlockTreeRule.php | head -50Repository: ArtisanPack-UI/visual-editor
Length of output: 1784
Move the authorization check to the authorize() method.
Line 31 always returns true, so the payload gets fully validated before the policy check in the controller (line 113). This means an authenticated caller can trigger expensive validation in TemplateBlockTreeRule even if they lack create permission. Policy checks should happen before validation runs.
Suggested change
public function authorize(): bool
{
- return true;
+ return $this->user()?->can( 'create', VisualEditorNavigation::class ) ?? false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Http/Requests/StoreNavigationRequest.php` around lines 29 - 32, The
request currently always returns true in StoreNavigationRequest::authorize(),
allowing expensive validation (e.g., TemplateBlockTreeRule) to run before the
controller's policy check; change authorize() to perform the permission check
(for example using $this->user()->can('create', Navigation::class) or
Gate::allows('create', Navigation::class)) and return that boolean so
unauthorized requests are rejected before validation runs, and remove/move the
duplicate policy check from the controller to rely on the request authorize()
method.
| protected function envelopeShapeRule(): Closure | ||
| { | ||
| return function ( string $attribute, mixed $value, Closure $fail ): void { | ||
| if ( is_array( $value ) && [] !== $value && array_is_list( $value ) ) { | ||
| $fail( 'The :attribute must be a { raw, blocks } envelope, not a bare list of blocks.' ); | ||
| } |
There was a problem hiding this comment.
Reject content: [] too.
Line 80 exempts the empty list case, so content: [] is accepted and then normalized into an empty { raw, blocks } envelope. That leaves a contract hole even though this rule says the payload must be object-shaped.
Suggested change
return function ( string $attribute, mixed $value, Closure $fail ): void {
- if ( is_array( $value ) && [] !== $value && array_is_list( $value ) ) {
+ if ( is_array( $value ) && array_is_list( $value ) ) {
$fail( 'The :attribute must be a { raw, blocks } envelope, not a bare list of blocks.' );
}
};📝 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.
| protected function envelopeShapeRule(): Closure | |
| { | |
| return function ( string $attribute, mixed $value, Closure $fail ): void { | |
| if ( is_array( $value ) && [] !== $value && array_is_list( $value ) ) { | |
| $fail( 'The :attribute must be a { raw, blocks } envelope, not a bare list of blocks.' ); | |
| } | |
| protected function envelopeShapeRule(): Closure | |
| { | |
| return function ( string $attribute, mixed $value, Closure $fail ): void { | |
| if ( is_array( $value ) && array_is_list( $value ) ) { | |
| $fail( 'The :attribute must be a { raw, blocks } envelope, not a bare list of blocks.' ); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 79-79: Avoid unused parameters such as '$attribute'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Http/Requests/StoreNavigationRequest.php` around lines 77 - 82, The
envelopeShapeRule() closure currently lets an empty list pass because of the "&&
[] !== $value" check; remove that exemption so any list-shaped array (including
empty lists like content: []) is rejected. Update the condition in
envelopeShapeRule() (the Closure returned by envelopeShapeRule) from
"is_array($value) && [] !== $value && array_is_list($value)" to simply
"is_array($value) && array_is_list($value)" (or add an explicit check to fail
when $value is an empty list) so the rule rejects bare lists including empty
arrays and calls $fail() accordingly.
| public function authorize(): bool | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Laravel FormRequest, does authorize() run before validation and before the controller action?
💡 Result:
Yes, in Laravel FormRequest, the authorize method runs before validation and before the controller action. The FormRequest implements ValidatesWhenResolved, which triggers validateResolved automatically when the request is resolved (type-hinted in controller). In validateResolved from ValidatesWhenResolvedTrait: 1. prepareForValidation 2. passesAuthorization which calls authorize 3. If authorization fails, failedAuthorization throws exception (controller not reached) 4. getValidatorInstance creates validator using rules 5. Validator runs (actual data validation) 6. If validation fails, failedValidation 7. If both pass, passedValidation, controller action executes This order is confirmed in Laravel 10.x source code and official docs stating FormRequest validation (including authorization) happens before controller method. If authorize returns false, 403 Forbidden before any validation or controller execution.
Citations:
- 1: https://laravel.com/docs/master/validation
- 2: https://laravel.com/docs/11.x/validation
- 3: https://laravel.com/docs/13.x/validation
- 4: https://github.com/laravel/framework/blob/10.x/src/Illuminate/Foundation/Http/FormRequest.php
- 5: https://github.com/laravel/framework/blob/5.8/src/Illuminate/Foundation/Http/FormRequest.php
- 6: https://github.com/laravel/framework/blob/10.x/src/Illuminate/Validation/ValidatesWhenResolvedTrait.php
- 7: https://api.laravel.com/docs/master/Illuminate/Validation/ValidatesWhenResolvedTrait.html
🏁 Script executed:
# Check the UpdateNavigationRequest.php file content
cat -n src/Http/Requests/UpdateNavigationRequest.phpRepository: ArtisanPack-UI/visual-editor
Length of output: 3114
🏁 Script executed:
# Check the controller to see the policy check at line 141
sed -n '130,150p' src/Http/Controllers/NavigationController.phpRepository: ArtisanPack-UI/visual-editor
Length of output: 543
🏁 Script executed:
# Check if VisualEditorNavigation class exists and is properly imported
rg "VisualEditorNavigation" --type php -B 2 -A 5 src/Http/Requests/Repository: ArtisanPack-UI/visual-editor
Length of output: 3295
🏁 Script executed:
# Check the StoreNavigationRequest to see if there's a similar pattern
cat -n src/Http/Requests/StoreNavigationRequest.php | head -50Repository: ArtisanPack-UI/visual-editor
Length of output: 1670
🏁 Script executed:
# Check the routes to understand the route parameters
rg "navigation" --type php src/routes/ | grep -i updateRepository: ArtisanPack-UI/visual-editor
Length of output: 123
Move the update authorization check to the authorize() method.
Currently, authorize() returns true unconditionally, allowing any authenticated user to pass request validation. The actual authorization check (Gate::authorize()) happens in the controller after validation, meaning unauthorized users can trigger validation errors and receive 422 responses instead of being rejected immediately with 403. In Laravel, authorize() runs before validation; implement the policy check here to prevent unauthorized access from running the validator.
Suggested change
public function authorize(): bool
{
- return true;
+ $navigation = $this->route( 'navigation' );
+
+ return $navigation instanceof VisualEditorNavigation
+ && ( $this->user()?->can( 'update', $navigation ) ?? false );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Http/Requests/UpdateNavigationRequest.php` around lines 29 - 32, The
authorize() method in UpdateNavigationRequest currently returns true
unconditionally; move the authorization check into this method so the policy is
enforced before validation. Replace the unconditional return with a call to
Gate::authorize(...) or use $this->user()->can(...) inside
UpdateNavigationRequest::authorize() to run the same permission check used in
the controller (matching the Gate::authorize() call there) and return true only
when authorized—otherwise throw/return false so Laravel returns 403 prior to
validation.
| protected function envelopeShapeRule(): Closure | ||
| { | ||
| return function ( string $attribute, mixed $value, Closure $fail ): void { | ||
| if ( is_array( $value ) && [] !== $value && array_is_list( $value ) ) { | ||
| $fail( 'The :attribute must be a { raw, blocks } envelope, not a bare list of blocks.' ); | ||
| } |
There was a problem hiding this comment.
Reject the empty bare-list payload on update as well.
Line 82 still allows content: [], which then gets normalized into an empty envelope. That makes the update path accept a payload shape this validator says should be rejected.
Suggested change
return function ( string $attribute, mixed $value, Closure $fail ): void {
- if ( is_array( $value ) && [] !== $value && array_is_list( $value ) ) {
+ if ( is_array( $value ) && array_is_list( $value ) ) {
$fail( 'The :attribute must be a { raw, blocks } envelope, not a bare list of blocks.' );
}
};📝 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.
| protected function envelopeShapeRule(): Closure | |
| { | |
| return function ( string $attribute, mixed $value, Closure $fail ): void { | |
| if ( is_array( $value ) && [] !== $value && array_is_list( $value ) ) { | |
| $fail( 'The :attribute must be a { raw, blocks } envelope, not a bare list of blocks.' ); | |
| } | |
| protected function envelopeShapeRule(): Closure | |
| { | |
| return function ( string $attribute, mixed $value, Closure $fail ): void { | |
| if ( is_array( $value ) && array_is_list( $value ) ) { | |
| $fail( 'The :attribute must be a { raw, blocks } envelope, not a bare list of blocks.' ); | |
| } | |
| }; | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 81-81: Avoid unused parameters such as '$attribute'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Http/Requests/UpdateNavigationRequest.php` around lines 79 - 84, The
envelopeShapeRule closure currently allows an empty bare list (content: [])
because it excludes [] from the failure condition; update the closure in
envelopeShapeRule so that any list-form array—including an empty array detected
by array_is_list($value)—triggers the $fail call. Locate the envelopeShapeRule()
method and remove the special-case exclusion (the [] !== $value check) so the
validation fails for both non-empty and empty bare lists with the existing error
message.
Description
Lands the server-side backing for the
wp_navigationentity the B1 core-data shim dispatches to/visual-editor/api/navigation. Adds the model, migration, CRUD REST controller, form-request validation, resource, policy, and the config-driven menu-locations surfacecore/navigationwill read in D4/E4.Closes: #360
Type of Change
Related Issue
Issue: #360
Motivation and Context
Per §4.3 of the V1 plan doc, navigation is the single biggest risk area in the expansion —
core/navigationpulls a disproportionate share of the core-data surface with it. B1 (#353) absorbed the client-side shim work and B2 (#354) shipped the fixtures; C4 lands the backend contract so D4 (nav editor UI) can re-enablecore/navigation.Menu locations for V1 are configuration-driven per §8 of the plan doc — a
navigation.locationsarray inconfig/artisanpack/visual-editor.phpnames the slugs the theme exposes and maps each to a nav record (or the fallback). No admin CRUD for locations in V1; that's 1.1+.Changes Made
VisualEditorNavigationEloquent model backingwp_navigationwith{raw, blocks}envelope helpers,published()scope, andisEmpty()check.visual_editor_navigations(id, unique slug, title, content JSON, status indexed, menu_order indexed, timestamps).NavigationControllerwith the five shim-compatible endpoints mounted under/visual-editor/api/navigation[/:id].StoreNavigationRequest/UpdateNavigationRequest— status enum,{raw, blocks}envelope shape,TemplateBlockTreeRulefor the block tree, unique slug (ignore-self on update).NavigationResourceshaping records into the B1wp_navigationenvelope (title.rendered,content.{raw,blocks},menu_order,type: wp_navigation).VisualEditorNavigationPolicy— auth-gated CRUD matching the Template policy default.navigation.locationsconfig section with a documented{slug, label, primary_id?}shape.MenuLocationResolver::forLocation($slug)service — returns configured primary when assigned + non-empty, else falls back to the first published nav bymenu_order, else null.routes/api.phpalongside the rest of the C-phase surface.How Has This Been Tested?
Testing Environment:
Tests Performed:
MenuLocationResolverbranch (configured hit, null primary_id, missing id, empty record, no published navs, unknown slug, malformed config).release/1.0— no findings.Accessibility Tests Run
Details: Backend-only PR. No templates, views, or frontend code shipped.
Tests Added
Test details:
tests/Feature/VisualEditor/NavigationControllerTest.php— 22 tests covering the full REST surface plus bare-list rejection, null-title rejection, slug-collision updates, and the B2 fixture round-trip.tests/Feature/VisualEditor/MenuLocationResolverTest.php— 10 tests covering configured-hit, fallback-hit for null/missing/empty primary, empty-DB null, draft exclusion, unknown-slug fallback, locations() normalization, malformed entry filtering.Documentation
Documentation details: PHPDoc blocks on every new class and public method. The
navigation.locationsconfig block is documented inline inconfig/visual-editor.php. No separate docs file needed —docs/core-data-shim.md§Navigation already locks the record shape.Pre-Submission Checklist
Summary by CodeRabbit
New Features
Tests