[Contracts/Deprecation] Provide a generic function and convention to trigger deprecation notices #35526
Conversation
b85243a to
51c09ec
Compare
|
is the
i.e. is it worth clarifying the difference when used y/n? |
|
In your example it looks like 8.9 is a float but it should be a string. |
|
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... |
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! |
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 Apart from this I really welcome this PR and I am 👍 |
|
@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? |
|
This reads more natural: - Deprecated: Since symfony/foo 12.3: lala lala
+ Deprecated since symfony/foo 12.3: lala lalaShall we do some string manipulation or keep the two |
|
There is no "Deprecated" prefix in my proposal because all the time, it's found in the message. |
There was a problem hiding this 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 🎉
There was a problem hiding this 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.
51c09ec to
f1593da
Compare
This is the exact final output, based on: |
|
PR updated, comments addressed.
nope, updated, operator moved inside the function.
I think that doesn't work in real life, with real messages.
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. |
|
@ostrolucky function deprecated($component, $version, $message)
{
} |
This explanation fits very well 👍 |
…trigger deprecation notices
0ed6a3a to
f0f41cb
Compare
|
Thank you @nicolas-grekas. |
… 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
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 |
There was a problem hiding this comment.
8.9 should be a string ?
|
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 I fear that attaching this side effect to the I mean, all deprecations are logged with a dedicated error level ( |
Float to string casts are locale-dependant in php! We really should not pass around version numbers as floats. https://3v4l.org/89Wu2 |
|
The reason we decided to use As for logging in production, I believe that we should never suggest people turn on |
Should we (symfony) provide such package? (just asking) |
Do we have any numbers on how bad the performance hit of Keep in mind that with this PR, we already have the function deprecated(…)
{
if ($_SERVER['SYMFONY_DISABLE_DEPRECATIONS'] ?? false) {
return;
}
@trigger_error(…);
}How much more expensive would this be, compared to the
My point exactly.
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.
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. |
I'm going to answer mine and your question: 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. |
|
Please have a look at #35648 |
…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
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.assertionsini setting and that can be caught during development to generate reports.The function requires at least 3 arguments:
printf()formattingExample:
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.