-
Notifications
You must be signed in to change notification settings - Fork 13
fix(ExAppNotifier): First check if any exapps enabled #614
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: main
Are you sure you want to change the base?
Conversation
To avoid unnecessary work and logs Signed-off-by: Marcel Klehr <[email protected]>
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.
Pull Request Overview
This PR adds an early guard clause in the prepare
method to avoid unnecessary work and logs when no external apps are enabled.
- Introduces a check for an empty ExApps list and throws
UnknownNotificationException
if none are configured. - Retains existing behavior of throwing the same exception when a specific app is unknown.
Comments suppressed due to low confidence (1)
lib/Notifications/ExAppNotifier.php:39
- New behavior for an empty ExApps list should be covered by a unit test to ensure the exception is thrown as expected.
if (count($this->service->getExAppsList()) === 0) {
more performant Signed-off-by: Marcel Klehr <[email protected]>
Is it just to hide the Because |
Yes |
another option - maybe we just remove that debug line? |
As you wish. I was trying to keep the behavioural change minimal :) |
Of course, I prefer to remove this debug line (it's a less significant change), but I am happy with the current approach. |
To avoid unnecessary work and logs