Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed check isCurrent from makeCurrent #115

Merged
merged 1 commit into from
Aug 21, 2020
Merged

Removed check isCurrent from makeCurrent #115

merged 1 commit into from
Aug 21, 2020

Conversation

masterix21
Copy link
Collaborator

It resolves a bug introduced by #111 PR.

Using a job to execute an Artisan command (Artisan::call(...), the Tenant is supplied, but Laravel resets the configuration applied by the tasks. To solve the problem, I removed the lines from 17 to 20 from Tenant model.

@masterix21 masterix21 merged commit a548263 into spatie:master Aug 21, 2020
@masterix21 masterix21 deleted the fix-artisan-cmd-from-jobs branch August 21, 2020 13:28
@telkins
Copy link
Contributor

telkins commented Aug 21, 2020

@masterix21 I'd like to know more about this problem that you described. How can it be reproduced?

I noted in the conversation in #111 that this PR now introduces issues with tests...not limited to tests that depend on jobs/queues (it seems).

I can't find a way around so maybe I can see if there's a different solution to the problem you described above...?

@masterix21
Copy link
Collaborator Author

masterix21 commented Aug 21, 2020

I’ve found the bug calling a chained queue:

FirstJob::withChain([
    new SecondJob(),
]);

If you try to call the migration command from SecondJob, you’ll get an error:

public function handle()
{
    Artisan::call(‘tenant:artisan “migrate --database=tenant --path=database/migrations/tenant” --tenant=1’);
}

@telkins
Copy link
Contributor

telkins commented Aug 21, 2020

I’ve found the bug calling a chained queue:

FirstJob::withChain([
    new SecondJob(),
]);

If you try to call the migration command from SecondJob, you’ll get an error:

public function handle()
{
    Artisan::call(‘tenant:artisan “migrate —database=tenant —path=database/migrations/tenant” —tenant=1’);
}

@masterix21 Thx. I'll try to look into it this weekend. In the meantime, any thoughts on my now-broken-with-the-latest-release tests...?

@telkins
Copy link
Contributor

telkins commented Aug 21, 2020

@masterix21 Some questions:

  • Are the jobs both implementing TenantAware...?
  • Not sure if you copied/pasted, but shouldn't all of the arguments in the artisan call be using double hyphens...? That is --tenant=1 instead of -tenant=1....could that be related?

In production I have a similar call from a job, which works, but it's not chained.

Alright....thx again. I'll post any findings if/when I get a chance to look at it. In the meantime, it's late and I've got to get my beauty sleep. ;-)

@masterix21
Copy link
Collaborator Author

masterix21 commented Aug 21, 2020

Yes, all jobs (and commands) are TenantAware.

About your second question, no, I'm using --, the iPad changed to -.

UPDATE: code updated ;)

@masterix21
Copy link
Collaborator Author

masterix21 commented Aug 24, 2020

I found the problem, and was an arrow function in my code: I solved changing it to a classic closure function. Check restored with 1.6.6 version, I'm sorry for the mistake.

@telkins
Copy link
Contributor

telkins commented Aug 24, 2020

Hey @masterix21 ....thx for the additional input/investigation. I had hoped to get time over the weekend to look into it some more....but didn't. :-(

That's fantastic that you found the problem. Great work...! 🤓

Anyway, I did sit down this morning and try to find a simple way to show what I believe to still be a problem....although not related, perhaps, to what we've been looking at. Please refer to #117 for a bit more information.

As I stated there (#117 ), I'm hoping someone can help see whether there's actually a problem or not and, if so, where it might be and what the solution might be.

Thx again...! 💪

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.

2 participants