Skip to content
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

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

Merged
merged 1 commit into from Feb 5, 2020

Conversation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 30, 2020

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:

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.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:deprecation branch from b85243a to 51c09ec Jan 30, 2020
@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Jan 30, 2020

is the @ operator required to meet the proposed purpose?

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

i.e. is it worth clarifying the difference when used y/n?

@OskarStark

This comment has been minimized.

Copy link
Contributor

OskarStark commented Jan 30, 2020

In your example it looks like 8.9 is a float but it should be a string.

@OskarStark

This comment has been minimized.

Copy link
Contributor

OskarStark commented Jan 30, 2020

Let’s say I will mute it via zend.assertions, I would also mute my follow code right?

assert(null !== $object);

If yes, this could lead to unexpected behavior...

Copy link
Contributor

greg0ire left a comment

I think this is a great move, especially the assert part! 👍

src/Symfony/Contracts/Deprecation/README.md Outdated Show resolved Hide resolved
src/Symfony/Contracts/Deprecation/README.md Outdated Show resolved Hide resolved
src/Symfony/Contracts/Deprecation/deprecated.php Outdated Show resolved Hide resolved
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Jan 30, 2020

In your example it looks like 8.9 is a float but it should be a string.

In PHP there is a really nice feature which is called type coercion. It's really nice because we can delegate these kind of details to the engine when they don't matter and let it deal with the concern for us!

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Jan 30, 2020

Let’s say I will mute it via zend.assertions, I would also mute my follow code right? assert(null !== $object); If yes, this could lead to unexpected behavior...

That's totally expected as that exactly what assert has been designed for. The opposite would be the unexpected behavior.

@OskarStark

This comment has been minimized.

Copy link
Contributor

OskarStark commented Jan 31, 2020

That's totally expected as that exactly what assert has been designed for. The opposite would be the unexpected behavior.

Let me be more clear on this topic, from a DX perspective. You explain, that one can mute this by setting setting X to Y but it is not clear (unless you check the method body itself), that the "muting" is done by deactivating the assert() function and not everybody knows (me too until now) that this ini settings exists.

Apart from this I really welcome this PR and I am 👍

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Jan 31, 2020

@OskarStark I'm not sure I understand what you want me to do, can you please tell me how you think I should update the PR?

@OskarStark

This comment has been minimized.

Copy link
Contributor

OskarStark commented Jan 31, 2020

This reads more natural:

- Deprecated: Since symfony/foo 12.3: lala lala
+ Deprecated since symfony/foo 12.3: lala lala

Shall we do some string manipulation or keep the two : in the sentence?

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Jan 31, 2020

There is no "Deprecated" prefix in my proposal because all the time, it's found in the message.

Copy link
Contributor

alcaeus left a comment

I agree with @stof about calling @trigger_error by default. We have noticed that using trigger_error without the suppression operator is a big no-no (see Symfony 2.7), and thus I think that we should not allow people that trigger deprecations to not make the method silent.

Other than that, I see this package as a very good evolution of the current deprecation logic in Symfony. It also allows people to provide different implementations for deprecated() by creating a package that uses replace in its composer.json. This gives people that want to log deprecations without the use of the PHP error handler mechanism to provide their own logic.

All in all, big 👍 from me for merging this and making this 1.0 of symfony/deprecation-contracts 🎉

Copy link
Contributor

ostrolucky left a comment

I was present at meetup, but we quickly changed topics so I didn't manage to voice my opinion. I don't agree that deprecations should be tied to zend.assertions. There is plenty people who log deprecations in production, but don't want to make normal assertions causing crashes in production, which is what would happen when these people are forced to enable assertions in order to log deprecations.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:deprecation branch from 51c09ec to f1593da Jan 31, 2020
@OskarStark

This comment has been minimized.

Copy link
Contributor

OskarStark commented Jan 31, 2020

There is no "Deprecated" prefix in my proposal because all the time, it's found in the message.

This is the exact final output, based on:
https://3v4l.org/CIZch

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Jan 31, 2020

PR updated, comments addressed.

is the @ operator required to meet the proposed purpose?

nope, updated, operator moved inside the function.

This is the exact final output, based on: 3v4l.org/CIZch

I think that doesn't work in real life, with real messages.

people who log deprecations in production, but don't want to make normal assertions causing crashes in production

Normal assertions can be configured to trigger a warning instead of an exception, and warnings can be turned to logs in prod (by default on Symfony). So I think this PR has a solution for every situations.

@beberlei

This comment has been minimized.

Copy link
Contributor

beberlei commented Jan 31, 2020

@ostrolucky trigger_error always returns true in this function, so the assertion is only a mechanism to make the AST assert optimization remove that code completly in production, making it essentially:

function deprecated($component, $version, $message)
{
}
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Feb 5, 2020

I think that the function name should be a verb, like trigger_deprecation() instead of deprecated(). That would also be more consistent with built-in PHP functions.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Feb 5, 2020

I think that the function name should be a verb, like trigger_deprecation() instead of deprecated().

I'm not sold on this one, because the function is explicitly designed to not trigger a deprecation in prod. Any verb would kinda lie to the reader.
In this regard, while this is a function call, this is closer to an annotation to me: it states that this code path is deprecated. What happens when this code path is run is configuration dependent. The caller doesn't need nor want to care. Said another way, that's a declarative statement.

@OskarStark

This comment has been minimized.

Copy link
Contributor

OskarStark commented Feb 5, 2020

it states that this code path is deprecated.

This explanation fits very well 👍

…trigger deprecation notices
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:deprecation branch from 0ed6a3a to f0f41cb Feb 5, 2020
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Feb 5, 2020

Thank you @nicolas-grekas.

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
@fabpot fabpot merged commit f0f41cb into symfony:master Feb 5, 2020
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:deprecation branch Feb 5, 2020
nicolas-grekas added a commit that referenced this pull request Feb 6, 2020
This PR was merged into the 5.1-dev branch.

Discussion
----------

Fix Contracts autoloader typo

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #35526
| License       | MIT

Autoloader does not include new Deprecation component when installing `symfony/contracts` due to [typo](https://getcomposer.org/doc/04-schema.md#files).

Steps to reproduce:
```
composer dump-autoload -a
cat vendor/composer/autoload_files.php
```
Produces:
```
<?php

// autoload_files.php @generated by Composer

$vendorDir = dirname(dirname(__FILE__));
$baseDir = dirname($vendorDir);

return array(
    '0e6d7bf4a5811bfa5cf40c5ccd6fae6a' => $vendorDir . '/symfony/polyfill-mbstring/bootstrap.php',
    '25072dd6e2470089de65ae7bf11d3109' => $vendorDir . '/symfony/polyfill-php72/bootstrap.php',
    'f598d06aa772fa33d905e87be6398fb1' => $vendorDir . '/symfony/polyfill-intl-idn/bootstrap.php',
);
```

Commits
-------

d5bcaff Fix Contracts autoloader typo
- more arguments can be provided: they will be inserted in the message using `printf()` formatting

Example:
```php

This comment has been minimized.

Copy link
@cedriclombardot

cedriclombardot Feb 6, 2020

Contributor

8.9 should be a string ?

This comment has been minimized.

Copy link
@greg0ire

greg0ire Feb 6, 2020

Contributor
@derrabus

This comment has been minimized.

Copy link
Contributor

derrabus commented Feb 7, 2020

Hey. I'm a bit late to the party here, sorry for that. I wasn't feeling well at the UG and had to leave early. Seems like I've missed an important discussion. 😃

I really like that we're now getting a unified way to express deprecations instead of the boilerplate that we've used before. This will also make it a lot easier to make use of that mechanism in other libraries and even userland code. This is a good thing.

However, I'm not really convinced that wrapping the deprecation into an assert() is the way to go, mainly following @ostrolucky's arguments. To me, zend.assertions is a setting that is totally unrelated to deprecations. Also, I want to disable assertions on prod, but I might want to log deprecations there.

I fear that attaching this side effect to the zend.assertions setting encourages people to chose ill-advised production settings for their php environment. I realize that this decision has already been made. Nevertheless, I'd like to have a discussion about alternatives to the assert() wrapping.

I mean, all deprecations are logged with a dedicated error level (E_USER_DEPRECATED). Is there really no better way to not log them in production? And if that way is too complicated, what can we do to make it easier?

@derrabus

This comment has been minimized.

Copy link
Contributor

derrabus commented Feb 7, 2020

In your example it looks like 8.9 is a float but it should be a string.

In PHP there is a really nice feature which is called type coercion. It's really nice because we can delegate these kind of details to the engine when they don't matter and let it deal with the concern for us!

Float to string casts are locale-dependant in php! We really should not pass around version numbers as floats. https://3v4l.org/89Wu2

@alcaeus

This comment has been minimized.

Copy link
Contributor

alcaeus commented Feb 7, 2020

The reason we decided to use assert is that in a production environment, it has no impact whatsoever since the parser won't generate an Opcode for the call. This is to remove concerns about @trigger_error having side effects and potential performance implications.

As for logging in production, I believe that we should never suggest people turn on zend.assertions in production, regardless for what use case without completely understanding the implications. If the only goal is to log deprecations, this should be done by creating a separate package that replaces the deprecation contract and provides its own implantation of the deprecated method: this method can then handle the deprecation using Monolog or any other mechanism that the user chooses to use.

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Feb 7, 2020

As for logging in production, I believe that we should never suggest people turn on zend.assertions in production, regardless for what use case without completely understanding the implications. If the only goal is to log deprecations, this should be done by creating a separate package that replaces the deprecation contract and provides its own implantation of the deprecated method: this method can then handle the deprecation using Monolog or any other mechanism that the user chooses to use.

Should we (symfony) provide such package? (just asking)

@derrabus

This comment has been minimized.

Copy link
Contributor

derrabus commented Feb 7, 2020

The reason we decided to use assert is that in a production environment, it has no impact whatsoever since the parser won't generate an Opcode for the call. This is to remove concerns about @trigger_error having side effects and potential performance implications.

Do we have any numbers on how bad the performance hit of trigger_error currently really is? That being said, deprecations are designed to be avoidable, meaning you should always be able to able to transform your application to a state where no deprecations are triggered at all. So even if there is a measurable performance penalty because of deprecations, that penalty should only be temporary.

Keep in mind that with this PR, we already have the deprecated() function call. That call will be compiled and executed, no matter if assertions are enabled or not. Let's say we would introduce a env variable to control this behavior:

function deprecated(…)
{
    if ($_SERVER['SYMFONY_DISABLE_DEPRECATIONS'] ?? false) {
        return;
    }

    @trigger_error(…);
}

How much more expensive would this be, compared to the assert() solution with assertions disabled?

As for logging in production, I believe that we should never suggest people turn on zend.assertions in production, regardless for what use case without completely understanding the implications.

My point exactly.

If the only goal is to log deprecations, this should be done by creating a separate package that replaces the deprecation contract and provides its own implantation of the deprecated method:

This sounds way too complicated to me. My prediction: Unless we provide an obvious and easy way to switch this behavior, people will enable assertions in production if they want deprecations to be logged.

this method can then handle the deprecation using Monolog or any other mechanism that the user chooses to use.

That's beside the point. Logging errors already works. Symfony's error handler does a pretty good job delegating logged errors to Monolog. And other error handler have similar features for logging errors.

@ostrolucky

This comment has been minimized.

Copy link
Contributor

ostrolucky commented Feb 7, 2020

How much more expensive would this be, compared to the assert() solution with assertions disabled?

I'm going to answer mine and your question:

>>>ini_set('error_reporting', E_ERROR);
=> "32767"
>>>ini_set('log_errors', 0);
=> "1"
>>>ini_set('zend.assertions', 0);
=> "1"
>>>function deprecated() { assert(@trigger_error('foo', E_USER_DEPRECATED)); }
>>>timeit -n1000 @trigger_error('foo', E_USER_DEPRECATED);
=> true
Command took 0.000007 seconds on average (0.000007 median; 0.007471 total) to complete.
>>>timeit -n1000 deprecated()
=> null
Command took 0.000003 seconds on average (0.000003 median; 0.002710 total) to complete.
>>>ini_set('zend.assertions', 1);
=> "0"
>>>timeit -n1000 deprecated()
=> null
Command took 0.000009 seconds on average (0.000008 median; 0.009156 total) to complete.
>>>

As for me, I'm most worried about side effects. Enabling assertions (which will now be encouraged in dev env) causes assertions in vendor/package{0..n} to execute extra code, not just vendor/packageWithDeprecations. And those packages don't expect most users having deprecations enabled, because symfony suddenly decided to hog zend.assertions for deprecations. Difficulty with logging deprecations while having disabled assertions also belongs to this category, because I agree people will likely jump into just enabling assertions in production, which will cause these side effects there as well.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Feb 8, 2020

Please have a look at #35648

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

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Contracts/Deprecation] don't use assert(), rename to trigger_deprecation()

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

This PR is a follow up the discussion that happened in #35526.

I applied all the suggestions I received so far on this previous PR so that we can discuss them all together.

Here are the changes:
- the most important change is to *not* use `assert()` anymore. This fixes the objection from @ostrolucky and @derrabus that deprecations and assert() should not be bound to each other. Instead, in order to still make the function a no-op when wanted, it is suggested that apps declare an empty function. This will be more flexible in practice I believe and keeps all the benefits we envisioned by using assert();
- as suggested by @fabpot, the function is renamed `trigger_deprecation()` instead of `deprecated()`. Because the implementation is now not conditional anymore (from the package pov, since assert() is not used), my arguments to keep the previous name don't stand as strong as before to me so I'm fine renaming.
- type hints are added back (requiring PHP 7.1 to use `void`). As mentioned in the 1st point, ppl that really want deprecations to be no-ops will redeclare the function as empty, thus not using any types, thus reclaiming all the perf diff. And for ppl that do want to catch deprecations, the overhead of types is negligible compared to any processing of the deprecations themselves.

WDYT?

All points can be discussed separately if needed of course. I'm personally fine with merging the PR as is, with all 3 changes.

Commits
-------

0032b2a [Contracts/Deprecation] don't use assert(), rename to trigger_deprecation()
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.