driftingly / rector-laravel

Rector upgrades rules for Laravel
http://getrector.org
MIT License
469 stars 48 forks source link

Bugfix when queueing closures they were mistaken as non-queable jobs #206

Closed j3j5 closed 3 months ago

j3j5 commented 3 months ago

Fixes #205 and add a test for it.

j3j5 commented 3 months ago

Oh! I just realized that Bus::dispatch() does not in fact queue the closure, only using the helper the closure gets queued. I wonder if I should report this upstream to the laravel repo, wdty

j3j5 commented 3 months ago

I've just changed it for now to match the current behaviour on the framework, that is, using dispatch() with a Closure will get the closure queued, using Bus::dispatch() will get it executed. I'm going to open an issue upstream and see if it's something they overlooked or is expected behaviour. If they fix it, I can resubmit another PR later.

j3j5 commented 3 months ago

[Fry :eyes:] Not sure if not a bug or he stopped reading right after me opening with "I'm not 100% sure this is a bug" [/Fry :eyes:]

https://github.com/laravel/framework/issues/51150#issuecomment-2069026557

My fault probably :shrug:

GeniJaho commented 3 months ago

@j3j5 what a find :grin:. It looks like undocumented behavior, it may not be a bug but as a user I would expect them to behave the same way. In any case, if this changes later and the Bus call is updated to queue closures, this Rule will need to be updated as well. In that regard I think it would be better if we skipped the Bus facade altogether for now, so as not to break existing/future code. If the behavior changes, we would probably add a rule in a version constraint, like from Laravel 11 and up only. What do you think?

j3j5 commented 3 months ago

So, as of now, my PR makes the rule to change all Bus::dispatch() calls to Bus::dispatchSync() (the calls which contain something that's not implementing the ShouldQueue interface), which is exactly what's happening before my PR, right now it will only skip calls using the helper. The behaviour, as seen on the test is:

// Before
- Bus::dispatch(fn() => 'hello');
dispatch(fn() => 'hello');
---
// After using DispatchNonShouldQueueToDispatchSyncRector
+ Bus::dispatchSync(fn() => 'hello');
dispatch(fn() => 'hello');
j3j5 commented 3 months ago

Also, if you also think it's a bug, please, feel free to comment on the other issue, haha.

GeniJaho commented 3 months ago

Sounds good, let's merge it :v: