[9.x] Use string based accessor for Schema facade #38017
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
driesvints
changed the title
This was referenced Jul 15, 2021
I wonder if the change in logic (the removed As for my part of the review, I'd appreciate if you could clone https://github.com/archtechx/tenancy, change the Laravel dependencies to 9.x in composer.json (hopefully this won't create any issues with the other dependencies), and run: I mentioned this test in my original bug report. It basically runs |
That's why I sent this to master. I believe we shouldn't support that workaround anymore as it was originally intended to only return strings from it (see the return type on the parent class). The Schema facade was an odd one at best.
I can't... too much dependencies yet that can't be resolved 😞 |
|
Ah, that's unfortunate. Maybe you can replicate the test with something simple like: config(['database.default' => 'foo']);
app('db.manager')->setDefaultConnection('foo');
Schema::getConnection()->getName(); // => foo
config(['database.default' => 'bar']);
app('db.manager')->setDefaultConnection('bar');
Schema::getConnection()->getName(); // => bar(I believe it's In the broken version the second |
|
@stancl I added the same changes to a branch of 8.x but I get the following on both 8.x and my branch: PHPUnit output
|
|
Isn't this just going to make all facades slightly slower for no real benefit? |
|
@stancl Okay, I tried switching some connection stuff to connect to my local MySQL database and that tests passes on both 8.x and my branch.
No? This doesn't affects other facades? And the only change is that schema builder is now resolved from the container rather than directly on the facade. |
|
@driesvints Right, sorry I forgot to mention that the tests need And if the test passes and this is the current behavior, then there shouldn't be any issues with this change 👍🏻 |
|
@stancl are you in a position to re-try this PR now that we're closer to Laravel 9's release date? |
|
Hey @driesvints, do you mean testing the compatibility with my package? |
|
@stancl yeah |
|
Will check it this weekend and let you know 👍 |
71 tasks
|
@driesvints do you happen to know if there's a way to use testbench for Laravel 9? I've updated the dependencies in my control to use Laravel's |
|
@stancl sorry, forgot to get back to you. You should be able to pull |
|
@driesvints No worries. Okay, setting everything up locally was a bit painful with some breaking changes in dependencies, but I managed to get most of the tests working with the relevant one passing without issues: So the Schema stuff works great 👍🏻 |
|
Thanks @stancl ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a re-send of #25512 but one that keeps in mind the issues which originated from it with #27686
It moves the Schema facade to a string based accessor in the way facades were intended to work. This will make all facade accessors in Laravel string based which means we can remove any object checks. The way this PR solves the original issue from #27686 is by adding a new
$cachedcheck to the facade class. If caching is turned off, a fresh instance is always retrieved from the container, just like the facade is currently doing.At the same time this PR solves #37983. The facade mocking will disregard the caching check and will always register a mock with the facade and thus enable all capabilities like spying, etc. Swapping is also supported now on the Schema facade.
Basically this PR should resolve all prior issues. While technically this should be good to go to 8.x, because the facade root isn't returning an object anymore I opted to send this in to master for the next major release.
I'd very much appreciate if @stancl (tenancy) and @awjudd (OP of the spying issue) could try out this PR.