lorisleiva / laravel-actions

⚡️ Laravel components that take care of one specific task
https://laravelactions.com
MIT License
2.52k stars 124 forks source link

Add option to configure custom job decorator classes #225

Closed ksassnowski closed 1 year ago

ksassnowski commented 1 year ago

Summary

This PR adds the ability to define custom job decorator classes. These custom classes will then get used when using an action as a job. The default implementation is to use the existing JobDecorator and UniqueJobDecorator classes, so this should be a backwards compatible change.

Code changes

In order to change the decorator classes, two new methods were added to the ActionManager.

Apart from that, the only non-test code that needed be changed were the makeJob, makeUniqueJob, and assertPushed methods in the AsJob trait. These methods will now grab the decorator class from the ActionManager at runtime.

Tests

Since $jobDecoratorClass and $uniqueJobDecoratorClass are both static properties on the ActionManager, I decided to play it extra safe and add a global afterEach callback to the test suite which resets them back to their default values. This avoids having to remember to manually reset this in every test case that might change these default settings (as has happened to me when implementing this).

I also added two datasets to more easily check that a given test passes both for the default decorator classes, as well as custom decorator classes.

Additional Info

I have already tested these changes in a large real-world application and can confirm that they are sufficient to ship first-party integration with Laravel Actions inside Venture. Let me know what you think 😊

Closes #224

Wulfheart commented 1 year ago

I can’t say anything to the technicalities yet. But the communication style is 💅

ksassnowski commented 1 year ago

I'm glad it's appreciated. I spend a lot of time on these PR descriptions 😅

ksassnowski commented 1 year ago

The test failures seem to be unrelated to my changes as I get the same failures on main.

ksassnowski commented 1 year ago

Awesome, thanks for merging!

Side note for a different PR: do you think it would make sense to support custom decorators for other patterns like controllers, listeners, etc?

I think it wouldn't hurt, but I also honestly don't know how much value that would add. It feels like this was a very specific problem between the two packages and not really a general thing that was missing from the package. The maintenance overhead wouldn't be huge, but it would be non-zero because adds just that little bit of indirection to the codebase.

So, if it's not necessary, I'd say don't bother.