[4.x] Correct DomainTenantResolver::isSubdomain() check#1425
[4.x] Correct DomainTenantResolver::isSubdomain() check#1425
DomainTenantResolver::isSubdomain() check#1425Conversation
Previously, the method returned `true` even for central domains, or tenant domains that ended with a central domain (e.g. tenant-app.test when app.test was central domain). This fix is basically the same as in #1423.
The test asserted that NotASubdomainException was thrown when visiting localhost/central-route. Because this exception is only thrown after the host is identified as a subdomain, while calling `makeSubdomain()`, NotASubdomainException is not thrown in this case anymore.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1425 +/- ##
============================================
+ Coverage 86.08% 86.10% +0.02%
- Complexity 1154 1157 +3
============================================
Files 184 184
Lines 3377 3383 +6
============================================
+ Hits 2907 2913 +6
Misses 470 470 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDomainTenantResolver::isSubdomain() was changed to normalize configured central domains into an array, explicitly exclude exact central-domain matches, and detect subdomains by checking string suffixes; tests were updated and a new unit test added to verify the behavior. Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Resolvers/DomainTenantResolver.php`:
- Around line 64-71: The loop in DomainTenantResolver that checks centralDomains
currently does suffix checks before ensuring longer/explicit central domains are
matched, causing order-dependent misclassification (e.g., 'test' vs 'app.test').
Fix by ensuring exact or more specific central domain matches are evaluated
first: either pre-check for any exact match of $domain against $centralDomains
(return false) before doing Str::endsWith checks, or sort $centralDomains by
descending length (so 'app.test' is checked before 'test') and then keep the
existing Str::endsWith logic; update the foreach over $centralDomains
accordingly in the resolver.
In `@tests/SubdomainTest.php`:
- Around line 112-117: Add assertions covering the exact central-domain and
overlapping central-domain cases: keep the existing
config(['tenancy.identification.central_domains' => ['app.test']]) test but also
assert DomainTenantResolver::isSubdomain('app.test') returns false (exact match
should not be treated as subdomain); then add a case with
config(['tenancy.identification.central_domains' => ['test','app.test']]) and
assert DomainTenantResolver::isSubdomain('foo.app.test') is true while
DomainTenantResolver::isSubdomain('app.test') remains false to ensure
overlapping central domains are handled correctly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3ca179b-6f2f-4716-a29e-2afdbd59a2a4
📒 Files selected for processing (3)
src/Resolvers/DomainTenantResolver.phptests/EarlyIdentificationTest.phptests/SubdomainTest.php
Added a failing test for determining if a host is a subdomain, then fixed
DomainTenantResolver::isSubdomain()(same fix as in #1423) and a related assertion.Summary by CodeRabbit