Skip to content

laravel 12 support - #1051

Open
fabmade wants to merge 5 commits into
tenancy:5.xfrom
fabmade:5.x
Open

laravel 12 support#1051
fabmade wants to merge 5 commits into
tenancy:5.xfrom
fabmade:5.x

Conversation

@fabmade

@fabmade fabmade commented Mar 4, 2025

Copy link
Copy Markdown
Contributor

support for laravel 12

@fabmade fabmade changed the title laravel 12 support (under contstruction) laravel 12 support Mar 4, 2025
@fabmade

fabmade commented Mar 26, 2025

Copy link
Copy Markdown
Contributor Author

Can someone please check this?

@famendes-zs

Copy link
Copy Markdown

please...someone...

Comment thread src/Environment.php Outdated
@famendes-zs

Copy link
Copy Markdown

@fabmade could you kindly make the change @junamai2000 requested, please?

@famendes-zs

Copy link
Copy Markdown

Thank you @fabmade. Can this be merged now @junamai2000?

@janiskelemen

Copy link
Copy Markdown
Contributor

Hi @ArlonAntonius, I think this can be merged. Would you be able to take a look? Big thanks in advance!

@famendes-zs

Copy link
Copy Markdown

Hi! Any update on this merge? Thank you all.

@fabmade

fabmade commented Sep 11, 2025

Copy link
Copy Markdown
Contributor Author

?

@famendes-zs

Copy link
Copy Markdown

Please, someone...Can this, please, be merged? Thanks in advance...

@fernandocoronatomf

Copy link
Copy Markdown

Can we get this merged please?

@junamai2000

Copy link
Copy Markdown

Please merge this PR @ArlonAntonius @luceos

@famendes-zs

Copy link
Copy Markdown

Please...anyone...

@go-wolverine

Copy link
Copy Markdown

Please merge this PR @ArlonAntonius @luceos

@famendes-zs

Copy link
Copy Markdown

Another long shot to see if this message alert reaches anyone who can merge this...

@luceos luceos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about this. I looked at this before and noticed some things that need a change. The Dockerfile contains German doc, which should be English. And the resolving issue with CurrentHostname on Laravel 12 must be explained, simply resolving without reason doesn't really help with the reliability of the package.

Having said this, compatibility up to the latest Laravel should be seen as a highest priority, I'm trying to take care of this but the current maintainers have not been able to provide the permissions to take maintenance responsibilities onto myself.

@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.

@dazza-dev

Copy link
Copy Markdown

I ran the suite against Laravel 12, since that is the thing this PR needs and nobody seems to have measured. Results, in case they are useful here.

src/ needs no change. Not one line. Laravel 11 and 12 differ in nothing this package touches: the migration command constructors are identical, prependMiddleware(), FilesystemManager::set()/forgetDisk(), Router::setRoutes()/refreshNameLookups(), Migrator::usingConnection()/setConnection()/resolveConnection() and the Translation\Loader contract are all unchanged.

The generated configuration is byte for byte identical. I dumped what the package produces — all four division modes, the derived password and the connection names — on 11.56.1 and on 12.68.0, with the same APP_KEY and the same website, and diffed:

-     "framework": "11.56.1",
+     "framework": "12.68.0",

That is the only difference in the whole snapshot.

The isolation suite passes on 12. Whether one tenant can reach another's data through a connection, a queued job or an HTTP request, on MySQL 8, MariaDB 11 and PostgreSQL 14 and 16, across the database, prefix and schema division modes, on PHP 8.2, 8.3 and 8.4.

Two things worth knowing:

  1. Laravel 12 registers storage.local and storage.local.upload for serving the local disk. Nothing in the package depends on that, but a test of mine that counted the application's routes started failing — worth checking if you have one. The count was the wrong assertion anyway.
  2. Carbon 3 becomes mandatory. The package only uses Carbon in property annotations on Models\Website and Models\Hostname, and Carbon::now()->toDateString() in the logger, none of which is affected by the 2 → 3 differences. The risk is in applications, not here.

On the constraint. ^12.0 alone drops Laravel 11, which is a breaking change for anyone on it. Whether that matters depends on whether this project wants to keep an 11 line alive; ^11.0|^12.0 would avoid the question, and I did not find anything that would make supporting both awkward.

For context: I maintain a fork at dazza-dev/hyn-multi-tenant, where this is released as v2.0.0 for Laravel 12 and v1.0.0 for Laravel 11, and I have been sending back what applies to 5.9 — #1054 for isolation and provisioning, #1055 for the test harness, #1056 for the per-request identification that #1015 asks about. The measurements above are from that work; the useful part for this PR is that the framework jump itself costs nothing.

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.

9 participants