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

nicolas-grekas · 2020-01-30T15:36:11Z

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.

src/Symfony/Contracts/Deprecation/deprecated.php

ro0NL · 2020-01-30T18:49:04Z

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 · 2020-01-30T20:52:08Z

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

OskarStark · 2020-01-30T21:04:21Z

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...

greg0ire

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

src/Symfony/Contracts/Deprecation/README.md

src/Symfony/Contracts/Deprecation/deprecated.php

nicolas-grekas · 2020-01-30T21:17:29Z

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 · 2020-01-30T21:19:52Z

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 · 2020-01-31T08:55:22Z

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 · 2020-01-31T09:40:25Z

@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?

src/Symfony/Contracts/Deprecation/deprecated.php

src/Symfony/Contracts/Deprecation/README.md

OskarStark · 2020-01-31T10:04:58Z

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 · 2020-01-31T10:10:12Z

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

alcaeus

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 🎉

ostrolucky

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.

OskarStark · 2020-01-31T13:40:40Z

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 · 2020-01-31T13:45:19Z

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 · 2020-01-31T13:54:43Z

@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)
{
}

OskarStark · 2020-02-05T13:35:41Z

it states that this code path is deprecated.

This explanation fits very well 👍

src/Symfony/Contracts/Deprecation/deprecated.php

…trigger deprecation notices

fabpot · 2020-02-05T18:14:24Z

Thank you @nicolas-grekas.

@alcaeus

… 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

@generated

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

cedriclombardot · 2020-02-06T20:26:04Z

src/Symfony/Contracts/Deprecation/README.md

+ - more arguments can be provided: they will be inserted in the message using `printf()` formatting
+
+Example:
+```php


8.9 should be a string ?

See #35526 (comment)

derrabus · 2020-02-07T09:42:47Z

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 · 2020-02-07T09:47:58Z

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 · 2020-02-07T10:08:55Z

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 · 2020-02-07T10:35:13Z

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 · 2020-02-07T11:10:58Z

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 · 2020-02-07T16:31:08Z

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 · 2020-02-08T13:03:49Z

Please have a look at #35648

@ostrolucky

…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()

…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

nicolas-grekas added this to the next milestone Jan 30, 2020

carsonbot added Status: Needs Review Feature labels Jan 30, 2020

nicolas-grekas commented Jan 30, 2020

View reviewed changes

src/Symfony/Contracts/Deprecation/deprecated.php Outdated Show resolved Hide resolved

nicolas-grekas force-pushed the deprecation branch from b85243a to 51c09ec Compare January 30, 2020 15:43

greg0ire reviewed Jan 30, 2020

View reviewed changes

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

fancyweb reviewed Jan 31, 2020

View reviewed changes

src/Symfony/Contracts/Deprecation/deprecated.php Show resolved Hide resolved

stof reviewed Jan 31, 2020

View reviewed changes

src/Symfony/Contracts/Deprecation/deprecated.php Outdated Show resolved Hide resolved

src/Symfony/Contracts/Deprecation/deprecated.php Outdated Show resolved Hide resolved

OskarStark reviewed Jan 31, 2020

View reviewed changes

src/Symfony/Contracts/Deprecation/README.md Show resolved Hide resolved

alcaeus approved these changes Jan 31, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 31, 2020

ostrolucky suggested changes Jan 31, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Jan 31, 2020

nicolas-grekas force-pushed the deprecation branch from 51c09ec to f1593da Compare January 31, 2020 13:40

nicolas-grekas added Status: Needs Review and removed Status: Needs Work labels Jan 31, 2020

nicolas-grekas added the Status: Needs Review label Feb 5, 2020

Tobion suggested changes Feb 5, 2020

View reviewed changes

src/Symfony/Contracts/Deprecation/deprecated.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 5, 2020

[Contracts/Deprecation] Provide a generic function and convention to …

f0f41cb

…trigger deprecation notices

nicolas-grekas force-pushed the deprecation branch from 0ed6a3a to f0f41cb Compare February 5, 2020 18:04

fabpot mentioned this pull request Feb 5, 2020

[Contracts/Deprecation] Provide a generic function and convention to trigger deprecations symfony/symfony-docs#13058

Closed

fabpot merged commit f0f41cb into symfony:master Feb 5, 2020

nicolas-grekas deleted the deprecation branch February 5, 2020 21:09

IonBazan mentioned this pull request Feb 6, 2020

Fix Contracts autoloader typo #35610

Merged

nicolas-grekas added Status: Reviewed and removed Status: Needs Work labels Feb 6, 2020

cedriclombardot reviewed Feb 6, 2020

View reviewed changes

nicolas-grekas mentioned this pull request Feb 8, 2020

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

Merged

nicolas-grekas modified the milestones: next, 5.1 May 4, 2020

fabpot mentioned this pull request May 5, 2020

Release v5.1.0-BETA1 #36713

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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

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

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment