-
-
Notifications
You must be signed in to change notification settings - Fork 14
Add extensibility methods to Session and Sanctum middleware #292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…-tenant applications
|
Hi @binaryfire, thank you for your contribution and for addressing a real pain point in multi-tenant applications! However, I have some concerns about this approach: Design Philosophy ConcernFramework components shouldn't need to know about the multi-tenancy concept. They should rely solely on their configuration as the single source of truth. The Consistency IssueIn Alternative ApproachI believe a better solution would be to implement a multi-tenancy-aware config instance at the configuration layer, rather than modifying individual framework components. This approach would:
What do you think? |
|
Hi @albertcht, thanks for the feedback. I want to make sure I understand the distinction here, because this approach is similar to the Regarding the config-layer solution - could you help me understand how this would work in practice? In my case (and I imagine many multi-tenant apps), tenant configuration is stored in the database with hundreds or thousands of tenants. The domain for a given request is determined by looking up the current tenant at runtime. It's dynamic. Wouldn't a config-layer solution be problematic because configs are persistent / stateful with Swoole? On the consistency point about |
|
Hi @albertcht. I realised I used multi tenant examples in the doc blocks and in the tests. The code itself is completely tenant agnostic, but the comments and test examples were confusing. I've removed all references to tenancy now. I also extracted the entire cookie config Just to clarify - the main thing I'm trying to solve in this PR is avoiding duplication and making maintenance easier. Currently the only way to create custom versions of these middleware classes is to duplicate all the logic:
Having extension points means we can extend them using just a few lines. And more importantly, there's no risk of our child classes breaking if the framework versions change. |
Replace getSessionCookieDomain with getSessionCookieConfig that returns all cookie settings, providing a single extension point for customization.
|
Hi @binaryfire, thanks for the clarifications and updates! My concern is more about the broader framework implications. If other components also face this scenario where different tenants need different config values, would we need to add similar overridable methods to all those components wherever they depend on config? I'm wondering if we could solve this pattern more generally with a tenant-aware config implementation. For example, when calling
This way, framework components that depend on config would naturally get different values per tenant without any modifications. However, this approach has its own limitation: components couldn't cache config values - they'd need to always fetch from the config instance dynamically, which isn't a universal solution either. So my main question is: Should framework components anticipate and accommodate multi-tenancy scenarios (or any dynamic config scenarios) by design? Again, this specific PR is well-implemented. I'm just thinking about whether this sets a precedent where we'd need to consider extensibility for dynamic configs across many other framework components in the future. What's your take on this? |
|
@albertcht That's a fair question about precedent. In my experience with multi-tenancy in Laravel, only a handful of things need to vary per-tenant: session domain, CORS origins, sanctum stateful domains. It's not a widespread problem across all framework components. A tenant-aware config system is an interesting idea, but as you noted it has limitations around caching. And personally, I think overriding the app configs at runtime is a bad idea and should be avoided. All the Laravel tenancy packages use custom middleware to handle dynamic tenant settings. These settings are stored independently of configs - eg. in the db (and cached on retrieval), set automatically per-request in There's only one other piece of the puzzle that's missing - Laravel's What do you think? |
Summary
This PR adds small extensibility methods to two middleware classes, allowing multi-tenant applications to extend them. Tenant middleware must dynamically determine session cookie domains and Sanctum stateful domains, and these hooks allow multi tenant apps to reuse framework middleware logic, which avoids duplication and makes maintenance easier.
Changes
StartSession middleware
getSessionCookieDomain(array $config): stringmethodEnsureFrontendRequestsAreStateful middleware
statefulDomains(): arraymethod (uses late static binding)Motivation
In multi-tenant applications where each tenant has a different domain (e.g., tenant-a.example.com, tenant-b.example.com, or custom domains like customsite.com), the session cookie domain and Sanctum stateful domains need to be determined dynamically per-request.
Normally, you’d create fully custom middleware for this. With these changes, custom middleware can extend the framework classes and take advantage of the existing logic:
Tests
Added tests for statefulDomains() method including override behavior verification.