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?

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

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.

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 theassert() 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 👍

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

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

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

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

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.

@ostroluckytrigger_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:

functiondeprecated($component,$version,$message){}

it states that this code path is deprecated.

This explanation fits very well 👍

Thank you@nicolas-grekas.

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 anassert() 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 thezend.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 theassert() 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?

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

The reason we decided to useassert 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 onzend.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 thedeprecated method: this method can then handle the deprecation using Monolog or any other mechanism that the user chooses to use.

As for logging in production, I believe that we should never suggest people turn onzend.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 thedeprecated 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)

The reason we decided to useassert 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 oftrigger_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 evenif 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 thedeprecated() 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:

functiondeprecated(…){if ($_SERVER['SYMFONY_DISABLE_DEPRECATIONS'] ??false) {return;    }    @trigger_error(…);}

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

As for logging in production, I believe that we should never suggest people turn onzend.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 thedeprecated 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.

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);=> trueCommand took 0.000007 seconds on average (0.000007 median; 0.007471 total) to complete.>>>timeit -n1000 deprecated()=> nullCommand took 0.000003 seconds on average (0.000003 median; 0.002710 total) to complete.>>>ini_set('zend.assertions', 1);=> "0">>>timeit -n1000 deprecated()=> nullCommand 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.

Please have a look at#35648