Repository navigation
fix: resolve container bindings without a leading backslash - #710
Conversation
Laravel's container does not normalise FQCN keys, so `'\Foo\Bar'` and `Foo\Bar::class` are distinct entries. `EntitySearchController` built the template / template-part resolver class strings with a leading backslash, so `app()` missed any host- or test-registered binding and silently constructed a fresh resolver instead. `class_exists()` behaves identically either way, which hid the problem. Swept the package for the same pattern and corrected every site whose class string is resolved through the container: the query-runtime binding in the service provider, the Blade `BlockRenderer` lookups in SnippetBlock and DynamicLoopBlock, the RoleManager lookup in VisibilityEvaluator, and the ThemeManager lookup in MenuLocationsController. Sites that only feed `class_exists()` or static Eloquent calls are left alone — the backslash is inert there. Adds two regression tests that bind stand-in resolvers under the canonical key and assert the controller uses them; both fail against the old string. Closes #692 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 33 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 |
github-project-automation
Bot
moved this from In Review
to Done
in ArtisanPack UI Overview
Description
Laravel's container does not normalise FQCN keys, so
'\Foo\Bar'andFoo\Bar::classare two distinct entries.EntitySearchController::searchCmsFrameworkEntities()built the template / template-part resolver class strings with a leading backslash, soapp( $resolverClass )missed any host- or test-registered binding and silently constructed a fresh resolver instead.class_exists()behaves identically either way, which is why the guard above it hid the problem — the code "worked", just never with the override the host asked for.This drops the leading backslash there, and sweeps the rest of the package for the same pattern.
Closes: #692
Type of Change
Related Issue
Issue: #692
Motivation and Context
Host apps and test suites can't decorate or stub the site-editor resolvers behind the entity-search endpoint — their binding is registered under the canonical key and the controller looks up a different one. The failure is silent: results still come back, just from the wrong (freshly-constructed) instance.
Pre-existing; not introduced by #688. Found during review of #689 while fixing the identical bug in
TemplatePartInliner— that one surfaced only because a new test tried to inject a stub resolver and quietly got the real one instead. It was left out of #689 to keep that diff scoped.Changes Made
EntitySearchController— dropped the leading\from both resolver class strings, with a comment explaining why the canonical key matters.VisualEditorServiceProvider::registerQueryResolverBinding()—QueryRuntime(via$app->make())SnippetBlock::renderTree()—BlockRendererDynamicLoopBlock—BlockRendererVisibilityEvaluator::resolveRoles()—RoleManagerMenuLocationsController—ThemeManager(plus the siblingMenuLocationAssignmentstring in the same statement, for consistency)class_exists()or static Eloquent calls, where the leading backslash is inert:PatternInliner::PATTERN_MODEL,CustomFieldSource's$cmsField, andMenuLocationsController::cmsFrameworkAvailable(). Changing those would widen the diff without fixing anything.How Has This Been Tested?
Testing Environment:
release/1.6)Tests Performed:
composer test— 1741 passed, 1 skipped, 4570 assertions.EntitySearchControllerand confirmed the template override test fails, then confirmed it passes with the fix.Accessibility Tests Run
Details: Not applicable — this is a server-side container-resolution fix with no rendered output or markup changes. The picker's UI is unchanged.
Tests Added
Test details: Two regression tests in
tests/Feature/VisualEditor/EntitySearchControllerTest.phpbind stand-inTemplateResolver/TemplatePartResolverinstances under the canonical key and assert the controller uses them. The template test also pins the alphabetical sort; the template-part test exercises theq=needle filter. Both fail against the old string.The two pre-existing "cms-framework is not installed" tests were renamed to "when no theme resolves" — cms-framework is present in the Testbench environment, so the empty result comes from there being no active theme, not from a missing package. The file's header comment was corrected to match.
Documentation
Documentation details: Added an inline comment at the fix site explaining the container's lack of FQCN normalisation, so the missing backslash reads as deliberate rather than as a typo waiting to be "fixed" back. No public API changed, so no README/wiki updates are needed.
Screenshots
n/a — no visual change.
Pre-Submission Checklist
composer.jsonhas only atestscript); style was matched by hand against the surrounding code.