[5.8] Bug: Schema is aware of the connection that was default when it was first initialized, while it should be aware of the current connection

[stancl]

• Laravel Version: 5.8.0
• PHP Version: 7.2
• Database Driver & Version: Doesn't matter

Description:

Introduced in #25512.

This results in Schema using the connection that the DB manager uses when Schema is first called.

// change DB connection from default to foo
Schema::getConnection()->getName(); // => foo
// change the DB connection back
Schema::getConnection()->getName(); // => foo

Schema remembers the connection that was used when it was first initialized.

In 5.7 Schema used the current DB connection, which I believe is more correct.

My package's tests are broken because of this.

https://github.com/stancl/tenancy/blob/e1def355f96686556edd125975e76d1018d30d24/tests/CommandsTest.php#L30-L37

On line 34 Schema is first initialized and remembers that the sqlite connection is used and on line 36, when the DB manager uses the tenant connection, Schema still uses the sqlite connection.

Steps To Reproduce:

You can try to turn my pseudocode into code or you can play with my package:

cd /tmp
git clone https://github.com/stancl/tenancy/
cd tenancy
git checkout f9a819215f7350bc3aec869cf7da687cd53bfcbc
composer install
phpunit --filter migrate_command_works_with_tenants_option

and look at the migrate_command_works_with_tenants_option method in tests/CommandsTest.php

@taylorotwell said:

Hmm, I'm scared to change this in case I forgot some reason I did it this way. 👀

I think that this is the reason. This facade's accessor should be obtained on demand.

[stancl]

#25512
#25525
laravel/docs#4544

Should be reverted.

Possibly #25497 but only if it's causing problems.

[driesvints]

I can see this being a bug but I'm wondering if we can solve this differently in a way that we don't need to revert these changes. It seems to me that we need to find a way maybe make sure the concrete always loads the correct db connection.

[driesvints]

We've had a thorough look at this and decided to revert the changes. We released a fix for this. Thanks for reporting.

[stancl]

If Schema is the only facade that doesn't use a string accessor, perhaps it would be better to find a way to make Schema use the current connection instead of doing that logic in the getFacadeAccessor method?

[taylorotwell]

Why? What tangible, actual benefit would that provide to end-users of Laravel?

Also, Schema already does use the current connection. The facade has caching built-in for performance which was what was causing the bug.

[stancl]

No benefit to end-users, however since #25525 was merged, I assume such consistency is wanted.

Edit: Actually, the PR mentions this benefit:

In case you're wondering why this is necessary: As far as I can
tell, facade features like ::swap() did not work with these types
of facades (Blade and Schema), because those methods did not deal
with the possibility of objects being returned.

[driesvints]

I sent this in again but with keeping this issue in mind. I'd appreciate your review @stancl: #38017