Skip to content

Use symfony/deprecation-contracts instead of trigger_error #40643

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
Apr 1, 2021

Conversation

andrew-demb
Copy link
Contributor

@andrew-demb andrew-demb commented Mar 30, 2021

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

As symfony/deprecation-contracts added to requirements with PR #40468, we can update trigger_error call in favor reusing deprecation abstractions

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.

Awesome. Thank you.

There are a few other places where we use trigger_error for deprecation.

Could you verify and update them too?

@andrew-demb
Copy link
Contributor Author

@Nyholm yes, I will.

One of the links relates to symfony/mailer, which doesn't have a dependency on symfony/deprecation-contracts.
Should I add it for this component?

@Nyholm
Copy link
Member

Nyholm commented Mar 31, 2021

Yes please

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.

Thank you. Just fix this typo

@andrew-demb andrew-demb force-pushed the patch-1 branch 2 times, most recently from 74faf7c to 1d1c145 Compare March 31, 2021 07:37
@andrew-demb
Copy link
Contributor Author

Sorry for so many force pushes. It was hard to drop "committed by" from commits.

All comments addressed. Thanks for the review.

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.

No worries about the many pushes.

Thank you for fixing this.

Btw, Fabbot is a false negative. We should not apply the CS fixes it suggests.

@xabbuh
Copy link
Member

xabbuh commented Mar 31, 2021

To be merged into 5.2?

@Nyholm
Copy link
Member

Nyholm commented Mar 31, 2021

I dont know if this is considered a bugfix or just a minor improvement. Im happy with 5.x and 5.2.

@nicolas-grekas
Copy link
Member

Thank you @andrew-demb.

@nicolas-grekas nicolas-grekas merged commit 11db9cd into symfony:5.x Apr 1, 2021
nicolas-grekas added a commit that referenced this pull request Apr 1, 2021
…sCommand (nicolas-grekas)

This PR was merged into the 5.3-dev branch.

Discussion
----------

[Messenger] fix deprecation layer in AbstractFailedMessagesCommand

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

Added in #38468, spotted in #40643

Commits
-------

079cb31 [Messenger] fix deprecation layer in AbstractFailedMessagesCommand
derrabus added a commit that referenced this pull request May 23, 2021
This PR was merged into the 5.3 branch.

Discussion
----------

[Mailer] Remove deprecation dependency

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

Introduced here (5.3) #40643
But removed was not needed because reverted here (5.2) #41380

Commits
-------

dc5c28d Remove deprecation dependency
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.

8 participants