-
Notifications
You must be signed in to change notification settings - Fork 11.6k
[12.x] Use displayName() for custom job identification #57499
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
base: 12.x
Are you sure you want to change the base?
Conversation
|
I think your idea has merit, but I personally don't like seeing things like this for BC: method_exists($job, 'jobTypeIdentifier') |
|
Just to clarify your BC concern:
Concern 1: Concern 2: While collision risk is small but not zero, an interface would avoid it entirely: interface HasJobTypeIdentifier
{
public function jobTypeIdentifier(): string;
}
$jobType = $job instanceof HasJobTypeIdentifier
? $job->jobTypeIdentifier()
: get_class($job);This requires explicit opt-in, which I'm fine with. Alternatively, |
|
I think you can just use the existing Mark as ready for review when the changes are made. 🫡 |
|
Updated to check for Why
|
|
I think what @taylorotwell was trying to say is that it should look something like this, which would be BC and use the frameworks existing API: return 'laravel_unique_job:'.$job->resolveName().':'.$uniqueId;Then the package would simply implement the |
|
Thank you for the suggestion. However, return 'laravel_unique_job:'.$job->job->resolveName().':'.$uniqueId;This approach presents issues where 1. Pre-dispatch context 2. Jobs without InteractsWithQueue Relying on the queue wrapper being available would break in above scenarios. The |
|
@taylorotwell I'd like to bring up one additional consideration regarding The method name suggests non-technical usage, as if it's purely for visual representation of jobs. However, this implementation gives it technical implications for lock/cache key generation, which could lead to unexpected behavior. In theory, developers could assign the same A potential solution would be to concatenate both the actual class name and the $jobName = method_exists($job, 'displayName')
? get_class($job).':'.$job->displayName()
: get_class($job);
return 'laravel_unique_job:'.$jobName.':'.$uniqueId;This approach would:
For now, I've kept the PR using |
Problem
The queue system uses
get_class($job)to identify jobs for:UniqueLock::getKey())ThrottlesExceptions::getKey())WithoutOverlapping::getLockKey())This doesn't support job wrapper patterns where multiple actions use the same wrapper class. This affects popular packages:
Since all actions share the same wrapper class name, they incorrectly share the same lock/cache keys.
Solution
Add an optional
jobTypeIdentifier()method that jobs can implement to provide a custom identifier as an alternative toget_class().Before:
After:
Wrapper packages can then implement:
Changes
Applied the same pattern to all three identification points:
src/Illuminate/Bus/UniqueLock.phpsrc/Illuminate/Queue/Middleware/ThrottlesExceptions.phpsrc/Illuminate/Queue/Middleware/WithoutOverlapping.phpTests added for both default (
get_class()) and custom identifier behavior.Backward Compatibility
✅ Fully backward compatible, existing jobs continue using
get_class()as before.