Skip to content

Leverage trigger_deprecation() from symfony/deprecation-contracts #35550

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
Feb 8, 2020

Conversation

nicolas-grekas
Copy link
Member

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

Here is what it could mean to use deprecated() from #35526 in the code base.

@nicolas-grekas nicolas-grekas added this to the next milestone Feb 1, 2020
@nicolas-grekas nicolas-grekas changed the title Leverage deprecate() from symfony/deprecation-contracts Leverage deprecated() from symfony/deprecation-contracts Feb 1, 2020
@javiereguiluz
Copy link
Member

Given the amount of occurrences of this -> deprecated('', '', '... deprecation message...') in the PR, shouldn't we change the signature of this method like this?

// Before
deprecated($package, $version, $message)

// After
deprecated(string $message, string $package = '', string $version = '')

@nicolas-grekas
Copy link
Member Author

Given the amount of occurrences of this -> deprecated('', '', '... deprecation message...') in the PR, shouldn't we change the signature of this method like this?

actually no: we should improve the deprecation subsystem to allow specifying the package and version of it that introduced eg deprecating this service or this config option.

fabpot added a commit that referenced this pull request Feb 5, 2020
… convention to trigger deprecation notices (nicolas-grekas)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Contracts/Deprecation] Provide a generic function and convention to trigger deprecation notices

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

This PR results from a discussion we had yesterday with @alcaeus, @beberlei and others at the Symfony UG meetup in Berlin (thanks SensioLabs for bringing me there).

It provides a generic function and convention to trigger deprecation notices, which could be a better alternative to the `@trigger_error(..., E_USER_DEPRECATED)` calls that we use currently.

This proposed package would provide a single global function named `deprecated()`.
Its purpose is to trigger deprecations in a way that can be silenced on production environment
by using the `zend.assertions` ini setting and that can be caught during development to generate reports.

The function requires at least 3 arguments:
 - the name of the package that is triggering the deprecation
 - the version of the package that introduced the deprecation
 - the message of the deprecation
 - more arguments can be provided: they will be inserted in the message using `printf()` formatting

Example:
```php
deprecated('symfony/blockchain', 8.9, 'Using "%s" is deprecated, use "%s" instead.', 'bitcoin', 'fabcoin');
```

This will generate the following message:
`Since symfony/blockchain 8.9: Using "bitcoin" is deprecated, use "fabcoin" instead.`

Check #35550 to see how using this function could look like on Symfony itself.

Commits
-------

f0f41cb [Contracts/Deprecation] Provide a generic function and convention to trigger deprecation notices
@nicolas-grekas nicolas-grekas marked this pull request as ready for review February 5, 2020 19:03
@ro0NL
Copy link
Contributor

ro0NL commented Feb 5, 2020

Do we have a some plan for e.g. vendors to provide the package/version details? Currently the ecosystem includes those in the message i tend to believe, as the message is the only way currently.

Im talking about deprecated services, config nodes, option resolvers & co. Thus where the user provides deprecation details ...

The amount of deprecated('', '', ...) occurrences triggered me :)

@nicolas-grekas
Copy link
Member Author

We do, in https://github.com/orgs/symfony/projects/1#card-32681032, help wanted :)

Copy link
Contributor

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

Are we sure this should target Symfony 5.1 with new deprecation mechanism? This will break deprecation setup for everybody who doesn't have assertions enabled (pretty much everybody).

Lot of people will probably not notice they stopped getting deprecations, then they will notice in Symfony 5.4, but then it's too late, they meanwhile already mistakenly added lot of deprecations they were not alerted by phpunit-bridge.

In case they do notice it in Symfony 5.1 and do enable assertions, unrelated assertion in vendor package might trigger and they will get errors/warnings that most people will not know how to get rid of without going back to disabled assertions again, leaving their deprecation setup broken.

@nicolas-grekas
Copy link
Member Author

@ostrolucky the answer is definitely yes, this should be done on 5.1. If there should be any discussion about the exact behavior of the function itself, let's have one, but the goal has to be to merge this PR in 5.1.

@nicolas-grekas nicolas-grekas changed the title Leverage deprecated() from symfony/deprecation-contracts Leverage trigger_deprecation() from symfony/deprecation-contracts Feb 8, 2020
@fabpot
Copy link
Member

fabpot commented Feb 8, 2020

Thank you @nicolas-grekas.

fabpot added a commit that referenced this pull request Feb 8, 2020
…n-contracts (nicolas-grekas)

This PR was merged into the 5.1-dev branch.

Discussion
----------

Leverage trigger_deprecation() from symfony/deprecation-contracts

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

Here is what it could mean to use `deprecated()` from #35526 in the code base.

Commits
-------

3e35d8e Leverage trigger_deprecation() from symfony/deprecation-contracts
@fabpot fabpot merged commit 3e35d8e into symfony:master Feb 8, 2020
@nicolas-grekas nicolas-grekas deleted the use-deprecated branch February 8, 2020 16:51
@Tobion Tobion mentioned this pull request Feb 9, 2020
6 tasks
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
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.

9 participants