Skip to content

[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

Merged
merged 1 commit into from
Dec 26, 2020

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Dec 18, 2020

Q A
Branch? 5.x
Bug fix? yes
New feature? yes
Deprecations? no
Tickets ---
License MIT
Doc PR ---

This can never return null

Nyholm
Nyholm previously approved these changes Dec 18, 2020
@Nyholm
Copy link
Member

Nyholm commented Dec 18, 2020

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.

@OskarStark OskarStark changed the base branch from 5.1 to 5.x December 18, 2020 10:54
@OskarStark
Copy link
Contributor Author

I switched it to 5.x 👍

@Nyholm
Copy link
Member

Nyholm commented Dec 18, 2020

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?

@Nyholm Nyholm dismissed their stale review December 18, 2020 10:57

I was wrong.

@OskarStark
Copy link
Contributor Author

As this is experimental, we can break BC in a minor release, thats why I now target 5.x

@nicolas-grekas nicolas-grekas modified the milestones: 5.1, 5.x Dec 18, 2020
@Nyholm
Copy link
Member

Nyholm commented Dec 18, 2020

As this is experimental, we can break BC in a minor release, thats why I now target 5.x

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.
Thank you again Oskar for your work.

@OskarStark
Copy link
Contributor Author

Thanks for your feedback Tobias 🌹 🌞

@OskarStark OskarStark changed the title [Notifier] Fix return type [Notifier] [BC BREAK] Fix return type Dec 18, 2020
Copy link
Member

@derrabus derrabus left a 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.

@derrabus
Copy link
Member

see also #39558 for 5.2

@OskarStark
Copy link
Contributor Author

Sure, like this?

"conflict": {
    ...
    "symfony/infobib-notifier": "<5.3",
    "symfony/mattermost-notifier": "<5.3",

nicolas-grekas added a commit that referenced this pull request Dec 18, 2020
…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.
@derrabus
Copy link
Member

I've done this for all bridges now: #39559

@OskarStark
Copy link
Contributor Author

Thank you

Copy link
Member

@jderusse jderusse left a 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?

@OskarStark
Copy link
Contributor Author

I rebased this PR @jderusse 👍

@derrabus
Copy link
Member

Thank you Oskar.

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

Successfully merging this pull request may close these issues.

6 participants