-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[11.x] Improved class-string
types
#53657
Conversation
@@ -166,7 +166,7 @@ protected function prepareData() | |||
/** | |||
* Get the display name for the queued job. | |||
* | |||
* @return string | |||
* @return class-string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is incorrect, just because the default displayName is the class, name. This could be overwritten and return any string to be displayed. (or am I wrong here?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you give me a situation where this would not be a classname?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method currently returns the class string, but if you extend the class than as the method name suggests you can return any "Display name" for underlying queued job.
The Queue already has a mechanism in place which returns a different "displayName" instead of the default class.
framework/src/Illuminate/Queue/Queue.php
Lines 170 to 180 in e1db317
/** | |
* Get the display name for the given job. | |
* | |
* @param object $job | |
* @return string | |
*/ | |
protected function getDisplayName($job) | |
{ | |
return method_exists($job, 'displayName') | |
? $job->displayName() : get_class($job); | |
} |
Though this is mainly an internal class, it could also return a different "displayName" instead of the default class (I would never change the current behaviour and I am not sure, many people would do this. But I think a displayName in general is a string and not really a class-string just semantically).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the intention of this method is to always show the underlying class, as this is a wrapper class.
We can see examples of Taylor closing PRs related to custom display names: #30551
Although, I can also find an example where the opposite is true: #34620
I'm not going to open a PR to revert this, but I wouldn't be mad if you decided it was best to try a PR to revert it back to string
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taylor already reverted it back to string in this PR. So I will leave it as is. But I also completely understand your point why it could be a class-string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, nice. Didn't even realise haha.
Still, was good chatting :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for me too ;)
Making the types of these strings more explicit.
These listeners are consumed by user-land code, so having these types can improve end-user project static analysis / auto-complete.