Skip to content
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

account for empty array in notifications config #1880

Closed
wants to merge 1 commit into from
Closed

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented Jan 30, 2025

reported in #1879

Given how the configurations are built, array_replace_recursive won't allow to use an empty array as replacement.

I am not 100% happy with this solution as it requires to treat $notifications array differently from other configs. So feel free to close this and revert the other PR.

Anyway I added tests for this changes to ensure it's working as expected, I just don't like to have that "special" treatment of notifications array.

This wouldn't be an issue if instead of an empty array, the way to disable the notification was setting it to null/false, but at this moment it would be a breaking change.

@freekmurze

@freekmurze
Copy link
Member

I'm going to revert the original PR for now, so we don't need to have special cases in our code.

@freekmurze freekmurze closed this Jan 31, 2025
@pxpm
Copy link
Contributor Author

pxpm commented Jan 31, 2025

howww, missed it for a minute!

I've just updated this branch to remove the "special" case, and created a seamless way to merge the configs that should work no matter how deep they are. 😢

I opened the PR at: #1881

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants