NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Notifier] Make`TransportTestCase` data providers static#49385

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

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:5.4fromalexandre-daubois:bc-notifier-static-data-providers

Feb 15, 2023

Merged

[Notifier] Make`TransportTestCase` data providers static#49385

nicolas-grekas merged 1 commit intosymfony:5.4fromalexandre-daubois:bc-notifier-static-data-providers

Feb 15, 2023

Conversation

Copy link

Member

alexandre-daubois commentedFeb 15, 2023•
edited by nicolas-grekas
Loading

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Part of#49368
License	MIT
Doc PR	-

ℹ️ Note: theTransportTestCase::createTransport() method also had to be passed to static.

cc@OskarStark

alexandre-daubois requested a review fromOskarStark as acode owner

February 15, 2023 07:52

carsonbot added Status: Needs Review Bug Deprecation Notifier labels

Feb 15, 2023

carsonbot added this to the5.4 milestone

Feb 15, 2023

alexandre-daubois force-pushed thebc-notifier-static-data-providers branch 3 times, most recently from471c943 to1509b1aCompare

February 15, 2023 08:04

nicolas-grekas changed the title~~[Notifier] MakeTransportTestCase data providers static~~[Notifier] [BC BREAK] MakeTransportTestCase data providers static

Feb 15, 2023

OskarStark reviewed

Feb 15, 2023

View reviewed changes

UPGRADE-5.4.md Outdated

		Notifier
		--------

		* The following data providers for`TransportTestCase` are now static:`toStringProvider()`,`supportedMessagesProvider()` and`unsupportedMessagesProvider()`

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is already done in#49368

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It was in case the main PR is not ready for the next patch version of 5.4. But I guess we'll be able to tackle everything before next release so I'll remove it! 🙂

nicolas-grekas approved these changes

Feb 15, 2023

View reviewed changes

Copy link

Member

nicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks, good to see it green. Can you please send a PR to 6.2 also? I will use it when merging up this branch. (+6.3 but the number of new bridges should be small so I could do it myself I suppose).

My reasoning for doing this in 5.4 is that while this is a BC BREAK, I consider phpunit as part of the PHP infrastructure. Exactly like we consider supporting new PHP versions as bug fixes, we should consider supporting new phpunit versions as bug fixes. The benefit of doing so is well known: enabling smooth migrations (this change won't break compact with phpunit < 10.)

About the BC BREAK itself, it cannot have prod impact since we're talking about test classes. Also, it would impact private notifier bridges. I suspect these to be quite rare.

👍

UPGRADE-5.4.md Outdated

		its default value will change to`true` in 6.0


		Notifier

Copy link

Member

nicolas-grekasFeb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

alpha order => after M* :)

Copy link

MemberAuthor

alexandre-dauboisFeb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Updated 🙂

UPGRADE-5.4.md OutdatedShow resolvedHide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Feb 15, 2023

carsonbot changed the title~~[Notifier] [BC BREAK] MakeTransportTestCase data providers static~~[Notifier] MakeTransportTestCase data providers static

Feb 15, 2023

OskarStark reviewed

Feb 15, 2023

View reviewed changes

UPGRADE-5.4.md OutdatedShow resolvedHide resolved

OskarStark approved these changes

Feb 15, 2023

View reviewed changes

nicolas-grekas added the BC Break label

Feb 15, 2023

[Notifier] MakeTransportTestCase data providers static

6a9f1d0

alexandre-daubois force-pushed thebc-notifier-static-data-providers branch from1509b1a to6a9f1d0Compare

February 15, 2023 12:02

Copy link

MemberAuthor

alexandre-daubois commentedFeb 15, 2023•
edited
Loading

About the 6.2 targeting PR, I'm on it! 👌

Edit: it's available here:#49389

alexandre-daubois mentioned this pull request

Feb 15, 2023

[Notifier] MakeTransportTestCase data providers static (6.2)#49389

Closed

nicolas-grekas merged commita9c2bce intosymfony:5.4

Feb 15, 2023

Copy link

Member

nicolas-grekas commentedFeb 15, 2023

thank you@alexandre-daubois

alexandre-daubois deleted the bc-notifier-static-data-providers branch

February 15, 2023 16:03

nicolas-grekas mentioned this pull request

Feb 15, 2023

[Notifier] Add Pusher Notifier Bridge#48203

Closed

bitgandtter added a commit to timgchile/symfony that referenced this pull request

Feb 15, 2023

chore: rebase forsymfony#49385

2be7303

Copy link

Member

nicolas-grekas commentedFeb 15, 2023

Arf sorry I realize only now that none of the classes in the Fixtures namespace should have been added.

MockHttpClient should be used instead of DummyHttpClient
*NullLogger for DummerLogger
DummyHub should be moved to the Mercure bridge
and there are already DummyMailer + DummyMessage implementations in Notifier that we should reuse

Can you please send a PR?

Copy link

MemberAuthor

alexandre-daubois commentedFeb 15, 2023

Of course, working on it 👍

bitgandtter added a commit to timgchile/symfony that referenced this pull request

Feb 15, 2023

chore: rebase forsymfony#49385

e17f292

alexandre-daubois mentioned this pull request

Feb 15, 2023

[Notifier] Replace tests dummy instances by already in place mocks#49398

Closed

bitgandtter added a commit to timgchile/symfony that referenced this pull request

Feb 15, 2023

chore: rebase forsymfony#49385

60ec41e

bitgandtter added a commit to timgchile/symfony that referenced this pull request

Feb 15, 2023

chore: apply comments fromsymfony#49385

a4c4401

nicolas-grekas added a commit that referenced this pull request

Feb 16, 2023

minor#49398[Notifier] Replace tests dummy instances by already in p…

703311e

…lace mocks (alexandre-daubois)This PR was merged into the 5.4 branch.Discussion----------[Notifier] Replace tests dummy instances by already in place mocks| Q             | A| ------------- | ---| Branch?       | 5.4| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       |Fix#49385 (comment)| License       | MIT| Doc PR        | _NA_Replacing dummy classes to use already existing mocks and stubs (`MockHttpClient`, `NullLogger` and so on).Commits-------83120cb [Notifier] Replace tests dummy instances by already in place mocks

This was referencedFeb 28, 2023

Release v5.4.21#49558

Merged

Release v6.2.7#49559

Merged

OskarStark pushed a commit to OskarStark/symfony that referenced this pull request

Dec 28, 2023