Skip to content

[Mime] Removes deprecated tests #41404

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 25, 2021
Merged

[Mime] Removes deprecated tests #41404

merged 1 commit into from
May 25, 2021

Conversation

0x346e3730
Copy link
Contributor

@0x346e3730 0x346e3730 commented May 24, 2021 •
edited
Loading

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

Follow-up from #41398 to fix CI for 6.0.

@carsonbot carsonbot added this to the 5.2 milestone May 24, 2021
@0x346e3730 0x346e3730 mentioned this pull request May 24, 2021
Closed
@0x346e3730 0x346e3730 force-pushed the mime-remove-deprecated-tests branch from 276d354 to 679de60 Compare May 24, 2021 21:29
@nicolas-grekas
Copy link
Member

Sorry I wasn't clear enough. On legacy tests, calling deprecated code is expected! This PR is for 6.0:)

@0x346e3730 0x346e3730 force-pushed the mime-remove-deprecated-tests branch from 679de60 to cc358f4 Compare May 24, 2021 21:41
@0x346e3730 0x346e3730 changed the base branch from 5.2 to 6.0 May 24, 2021 21:41
@0x346e3730
Copy link
Contributor Author

My bad, I hesitated and went for it as it's late, should be good now :) Not sure why it auto asked for review tho, sorry for the unnecessary pings :(

@Nyholm Nyholm modified the milestones: 5.2, 6.0 May 25, 2021
Nyholm
Nyholm approved these changes May 25, 2021
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

I think this looks good.

Address::fromString() is removed in favour of Address::create(). These tests are no longer needed.

@0x346e3730
Copy link
Contributor Author

I just reviewed as I've sent the PR late yesterday, shouldn't I also delete fromStringProvider ?

@0x346e3730
Copy link
Contributor Author

I just reviewed as I've sent the PR late yesterday, shouldn't I also delete fromStringProvider ?

It's still used in testCreateWithString, I don't know if renaming the provider is a good idea or not.

@Nyholm
Copy link
Member

Nyholm commented May 25, 2021

Good observation.

Let's rename it. There is no backwards compatibility issue with files in /Tests/

@nicolas-grekas
Copy link
Member

The rename should happen on 5.3 then!

@nicolas-grekas
Copy link
Member

Or better: don't rename

@Nyholm
Copy link
Member

Nyholm commented May 25, 2021

Oh... let's not rename it then =)

Im happy with this PR in its current state.

@0x346e3730
Copy link
Contributor Author

I'm cool with not renaming it too.

@Nyholm
Copy link
Member

Nyholm commented May 25, 2021

Awesome. Congratulations to your first contribution!

@Nyholm Nyholm merged commit 8885c2d into symfony:6.0 May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@Nyholm Nyholm Nyholm approved these changes

Assignees
No one assigned
Labels
Bug Mime Status: Reviewed
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@0x346e3730 @nicolas-grekas @Nyholm @carsonbot