Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Notifier] Fix extensibility ofNotification class#46172
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Without using static it kills the extensibility of the `Notification` class as the `exception` and `exceptionAsString` properties are private and other methods such as `EmailMessage::fromNotification` use that to customize the message to send to channels.
| $parts =explode('\\',\get_class($exception)); | ||
| $notification =newself(sprintf('%s: %s',array_pop($parts),$exception->getMessage()),$channels); | ||
| $notification =newstatic(sprintf('%s: %s',array_pop($parts),$exception->getMessage()),$channels); |
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 change assumes that the child class has the same constructor signature as its parent which is not necessarily the case.
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.
Indeed, I missed that. Should we then just add a methodexception? Like this:
/** * @return $this */publicfunctionexception(\Throwable$exception):self {$parts =explode('\\',\get_class($exception));$this->subject =sprintf('%s: %s',array_pop($parts),$exception->getMessage());if (class_exists(FlattenException::class)) {$this->exception =$exceptioninstanceof FlattenException ?$exception : FlattenException::createFromThrowable($exception); }$this->exceptionAsString =$this->computeExceptionAsString($exception);return$this; }
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.
That sounds like a good idea. But we would need to target 6.2 with that.
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.
@andersonamuller Your suggestion seems good to me. Can you close this PR and open another one on 6.2 with the newexception() method?
Notification classClosing in favor of#47038 |
This PR was merged into the 6.2 branch.Discussion----------[Notifier] Add Notification::exception()| Q | A| ------------- | ---| Branch? | 6.2| Bug fix? | no| New feature? | yes <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets |Fix#46172 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->| License | MIT| Doc PR | -Commits-------2db50f6 [Notifier] Add Notification::exception()
Without using static it kills the extensibility of the
Notificationclass as theexceptionandexceptionAsStringproperties are private and other methods such asEmailMessage::fromNotificationuse that to customize the message to send to channels.