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

[5.7] Transactional jobs #26515

Closed
wants to merge 8 commits into from
Closed

Conversation

ed-fruty
Copy link
Contributor

@ed-fruty ed-fruty commented Nov 14, 2018

Added Transactional jobs.

With this PR we can easy to define that our job must execute in database transaction and it will automatically rollback if something will wrong.

Simple example:

class CreatePayoutOrderJob implements ShouldQueue, Transactional
{
   // ...

   public function __construct(User $user, Wallet $wallet, Money $amount)
   {
        // ...
   }

   public function handle()
   {
       // 1. Create payout order
       $payout = new Payout;
       $payout->wallet = $this->wallet;
       $payout->amount = $this->amount;
       $payout->user_id = $this->user;
       $payout->saveOrFail();  

      // 2. Remove amount from user balance.
      $user->decreaseBalanceFor($this->amount)->saveOrFail();

      // etc.
   }
}

And our data will still consistent event payout creation/balance changes will fail.

Probably, makes sense to add option like -t|--transactional to job generation command ?

@ed-fruty ed-fruty changed the base branch from master to 5.7 November 14, 2018 23:40
};

if ($this->instance instanceof Transactional) {
$handler = function () use ($handler) {
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH this style is freaking me out: "use"ing a variable and overwriting it in the same scope so the final call to it is the same? => That's a bit too much magic for my taste


if ($this->instance instanceof Transactional) {
$handler = function () use ($handler) {
$connection = method_exists($this->instance, 'dbConnection') && \is_callable([$this->instance, 'dbConnection'])
Copy link
Contributor

Choose a reason for hiding this comment

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

How can we, except with documentation, better indicate to developers that this dbConnection method is possible to use?

The interface doesn't indicate it.

Also I'd like to see the naming a bit improved. E.g. getConnectionForTransaction or something, otherwise it's ambiguous.


$payload = $payloadFactory->invoke(new SyncQueue(), $jobClassname, null, '');

unset($_SERVER['0(*_*)0']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do better then working with a global variable here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, job can use any payload, given from container mock to detect works.
But this is a simple test like Taylor have used here

It also can use real DB connection, real transactions. It's only base ones.

@driesvints driesvints changed the title [5.7] / [5.8?] Transactional jobs [5.7] Transactional jobs Nov 15, 2018
@taylorotwell
Copy link
Member

Honestly much simpler to just use DB::transaction(function () {}); within your jobs handle method.

@mfn
Copy link
Contributor

mfn commented Jun 17, 2020

Hello everyone,

based on an existing package and with permission of its original author, I've drafted a possible solution to tackle the "transactional events" problem.

At the moment it's a draft PR in my Laravel forked repo to first gather feedback from the community before approaching to create an official PR, to give things time to unfold / develop.

I invite everyone to participate in mfn#1 and share your feedback.

Thank you!

@driesvints
Copy link
Member

@mfn think it's best that you actually send this in so people can give feedback. I doubt anyone will see it otherwise.

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.

4 participants