[3.x] Add method to run a callback on the central application #512
Conversation
Signed-off-by: michael lundbøl <[email protected]>
Codecov Report
@@ Coverage Diff @@
## 3.x #512 +/- ##
============================================
+ Coverage 87.01% 87.10% +0.09%
- Complexity 370 373 +3
============================================
Files 103 103
Lines 1078 1086 +8
============================================
+ Hits 938 946 +8
Misses 140 140
Continue to review full report at Codecov.
|
tests/AutomaticModeTest.php
Outdated
| $this->assertSame('acme', app('tenancy_initialized_for_tenant')); | ||
|
|
||
| tenancy()->runGlobal(function () { | ||
| GlobalRun::$count = 1; |
There was a problem hiding this comment.
I think you could just assert that the connection was tenant before this call and is not tenant inside it.
There was a problem hiding this comment.
This is to make sure that the callback is indeed being called.
There was a problem hiding this comment.
Count is not really an intuitive way of testing things. I'd make assertions that:
- The connection was
tenantbut is not anymore, testing that we're in the central context - Setting a value in
app(), testing that the callback is being called
There was a problem hiding this comment.
Can you take a look at the changes to the tests and se if you like this better?
Signed-off-by: michael lundbøl <[email protected]>
Signed-off-by: michael lundbøl <[email protected]>
Signed-off-by: michael lundbøl <[email protected]>
|
Hey @stancl I believe your comments have been addressed now. Can you take a second look? |
morloderex
changed the title
| $runned = 0; | ||
|
|
||
| $this->assertFalse(tenancy()->initialized); | ||
| $this->assertFalse(app()->bound('tenancy_ended')); | ||
| $this->assertFalse(app()->bound('tenancy_initialized_for_tenant')); | ||
|
|
||
| tenancy()->central(function () use (&$runned) { | ||
| $this->assertFalse(app()->bound('tenancy_initialized_for_tenant')); | ||
| $this->assertFalse(app()->bound('tenancy_ended')); | ||
| $this->assertFalse(tenancy()->initialized); | ||
|
|
||
| $runned = 1; |
There was a problem hiding this comment.
What I meant is to:
- Check that tenancy was initialized by seeing that
config('database.default')wastenantand is not anymore. - Do
app()->instance('central_callback_executed', true)inside this closure.
I can just rewrite the whole thing if you want.
It should
- set it to false at the beginning of the test
- check that it's false right before the central() call
- set it to true inside the central call
- check that it's true after the central call
There was a problem hiding this comment.
@stancl We have 2 different tests here which tests different scenarios seperatly.
One where the tenant is already initialized and one where a tenant is not already initialized.
The test you are commenting on is the use case where a tenant has not been initialized so i want to make sure that the TenancyManager does not call the end method before running the callback.
However, am not quite sure why you don't see this logic as sufficient? I am not quite sure why we would need to go through the entire Laravel container class to check that a veriable is changed within a callback? Using a variable passed in by reference does the same thing here and the assertions called within the callback tests wether or not the bootstrapper provided has been executed. Can you ellabrorate on throughs here?
Co-authored-by: Samuel Štancl <[email protected]>
|
there are some other changes that I'd want here, but this is a small amount of code so it's literally easier to just write it than to explain the changes appreciate the effort, closing in favor of #526 |
Closes: #487
I found some free time to create a helper for when a tenant has been identified and you want to run a callback on the central application.
I'm not quite sure about the naming tho:
Other naming possiblities considered: