Skip to content

Upgrade to laravel 11 - #1050

Open
vchen-promptemr wants to merge 1 commit into
tenancy:5.xfrom
vchen-promptemr:vc/laravel-11
Open

Upgrade to laravel 11#1050
vchen-promptemr wants to merge 1 commit into
tenancy:5.xfrom
vchen-promptemr:vc/laravel-11

Conversation

@vchen-promptemr

Copy link
Copy Markdown

No description provided.

@pmhou

pmhou commented Jan 10, 2025

Copy link
Copy Markdown

@ArlonAntonius

@dazza-dev

Copy link
Copy Markdown

I went looking for why the extra make(CurrentHostname::class) in identifyHostname() is needed, since the comment next to it in #1051 says it is not clear. I think I can explain it, and the answer is that it is treating a symptom.

identifyHostname() does not identify anything. All it does is register a lazy singleton:

public function identifyHostname()
{
    $this->app->singleton(CurrentHostname::class, function () {
        $hostname = $this->dispatch(new HostnameIdentification());
        $this->tenant(optional($hostname)->website);
        return $hostname;
    });
}

Until something resolves that binding, no identification is dispatched and no tenant is set. So on its own, calling it is a no-op.

The reason it appears to work today is an accident of provider ordering:

  1. TenancyProvider::register() registers the Environment singleton inside a booted callback.
  2. HostnameProvider is a deferred provider whose boot() calls make(Environment::class), and that runs before booted callbacks.
  3. So the first resolution finds no singleton and builds an Environment through reflection that is then thrown away — but its constructor has already registered the binding and resolved it eagerly.
  4. The booted callback then registers the real singleton, and the next resolution builds a second Environment, which identifies all over again.

I put a backtrace in the constructor to check: two constructions per application. Registering the singleton in register() instead of in booted brings it down to one.

That second construction is what makes a later identifyHostname() appear to take effect, because resolving Environment again re-registers and re-resolves the binding. Remove the duplication and the method goes back to doing nothing visible — which I believe is the symptom the patch in both PRs is compensating for.

Worth knowing either way, since it is not free: per request it means installed() twice (a schema query), two identifications, and two Websites\Switched events, so any listener an application hangs off that event runs twice.

I have not sent a fix for this, because changing it moves identification to boot time, and whether the request's Host still decides the tenant then depends on the application's boot order. That is not something to change on a hunch in the code path that decides which tenant is active. Flagging it here so the finding is not lost, and so nobody adopts the extra make() believing it is the cause rather than a workaround.

For context: I maintain a fork of 5.9 at dazza-dev/hyn-multi-tenant and have been sending back what applies here — #1054 for the isolation and provisioning fixes, #1055 for moving the test harness to Orchestra Testbench.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants