-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Notifier] [BC BREAK] Fix return type #39549
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
Conversation
Thank you @OskarStark for this PR. We cannot merge this since it is a BC break. I had a better look where this is used, and I can see that nowhere do we handle the scenario where this method returns null. |
I switched it to |
c76d17d
to
9e0c0fc
Compare
I think the way to fix this is to deprecate the method and to add a new one with the correct syntax. But Im not sure it is worth the trouble. What is the reason you want to tighten this? |
9e0c0fc
to
9b8225e
Compare
As this is experimental, we can break BC in a minor release, thats why I now target |
75794f4
to
1de7ec1
Compare
Im not in my usual game today. I've been wrong twice in the same PR. I can admit that my mouse was even hovering the close button. Im currently extra happy that Im not the only reviewer. |
Thanks for your feedback Tobias 🌹 🌞 |
1de7ec1
to
84937b9
Compare
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.
In Notifier's composer.json, can you please adjust the conflict rules for the Infobip and Mattermost bridges? Their 5.2 branches will become incompatible with this PR.
see also #39558 for 5.2 |
Sure, like this?
|
…change. (derrabus) This PR was merged into the 5.2 branch. Discussion ---------- [Notifier] Prepare bridges for the upcoming return type change. | Q | A | ------------- | --- | Branch? | 5.2 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | N/A | License | MIT | Doc PR | N/A Preparation for #39549. Commits ------- b0c2c4e [Notifier] Prepare bridges for the upcoming return type change.
I've done this for all bridges now: #39559 |
Thank you |
84937b9
to
d508013
Compare
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 please rebase and fix the conflict?
d508013
to
d4f001a
Compare
I rebased this PR @jderusse 👍 |
Thank you Oskar. |
This can never return
null