[RFC] Fixing Deprecation Warnings #14900

reecefowell · 2015-06-07T06:45:19Z

This PR address an issue that causes Symfony to vomit E_DEPRECATED warnings everywhere.

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #14899
| License       | MIT

Fixes #14899

perk11 · 2015-06-07T06:51:12Z

👍 had numerous deprecated warnings after upgrading to 2.7 and this PR solved all of them.

xabbuh · 2015-06-07T08:12:50Z

The deprecation warnings are triggered intentionally to let you know what you need to change in your code to be forward compatible with Symfony 3.0.

You could silence them by changing your project's error_reporting level, but I'd rather update your code.

nicolas-grekas · 2015-06-07T08:16:41Z

This PR may be a good idea in fact don't you think?

elnur · 2015-06-07T08:22:53Z

The most profound PR I've ever seen. A must merge. 👍

xabbuh · 2015-06-07T08:29:10Z

@nicolas-grekas Not sure if I understand. Which issue would this change actually solve?

nicolas-grekas · 2015-06-07T08:31:25Z

It switches deprecation notices from opt-out to opt-in

xabbuh · 2015-06-07T08:34:04Z

Yeah, but is that really a good idea? I bet that many people then will never know about this and that they will be surprised when their apps break on Symfony 3.0. Not sure if that's a good experience either.

reecefowell · 2015-06-07T10:53:46Z

I think it is a good idea. @deprecated tags are sufficient, but calling trigger_error() is very shouty.

Googling most of the deprecated errors that come out result in most cases with the people discussing end suggesting and / or agreeing that muting E_DEPRECATED in php.ini is the best solution.

If the intent is to get people to prepare for 3.0 then, well people aren't. Instead they are just muting the warnings. Further more, for upgraders, seeing the messages for the first time, diagnosing / solving them the correct way is very difficult as google has little to offer in the way of answers.

Perhaps some of the deprecated calls don't actually have future ready implementations in place yet. I believe this was the case with the Form components OptionResolverInterface, which my IDE informs me was deprecated in (I think it was) 2.5 or so, but no alternative was provided until something like 2.6 (with OptionResolver).

nicolas-grekas · 2015-06-07T10:53:56Z

Still thinking about it, I really like the idea.
Upgrading to 3.0 is also an opt in, the upgrade path is clear.

I'm 👍
Reopening to see what other @symfony/deciders think

nicolas-grekas · 2015-06-07T11:00:40Z

@reecefowell can you please enhance the commit message?

nicolas-grekas · 2015-06-07T11:04:48Z

src/Symfony/Component/HttpFoundation/Tests/JsonResponseTest.php

@@ -224,7 +224,7 @@ class JsonSerializableObject implements \JsonSerializable
    {
        public function jsonSerialize()
        {
-            trigger_error('This error is expected', E_USER_WARNING);
+            @trigger_error('This error is expected', E_USER_WARNING);


This one is not a deprecation, please revert and ensure that you silence only deprecations

stof · 2015-06-07T12:25:57Z

@nicolas-grekas are these deprecation warnings still reported in the logs after being muted or no ?

And does the silencing add some overhead or no ?

nicolas-grekas · 2015-06-07T13:24:28Z

Yes, the debug handler ignores silencing for deprecations. The overhead is not significant at all.

fabpot · 2015-06-07T18:05:19Z

ping @symfony/deciders

If we agree on this one, I'd like to merge ASAP for at least two reasons:

I want to release 2.7.1 ASAP
Maintaining such a large patch is going to be difficult over time.

Tobion · 2015-06-07T20:05:39Z

So this only changes that the deprecation warnings are ignored in production, or?

nicolas-grekas · 2015-06-08T03:41:03Z

Yes, by default

reecefowell · 2015-06-08T05:31:07Z

src/Symfony/Component/Debug/Tests/DebugClassLoaderTest.php

@@ -170,7 +170,7 @@ public function testDeprecatedSuper($class, $super, $type)
    {
        set_error_handler('var_dump', 0);
        $e = error_reporting(0);
-        trigger_error('', E_USER_DEPRECATED);
+        @trigger_error('', E_USER_DEPRECATED);


Why is this even here? It doesn't have a message. Presumably the intention is to pass to the $e var to trigger_error?

It's a trick to reset the state of error_get_last() a few lines below. Silencing should be reverted here, as it's already done on line 172.

linaori · 2015-06-08T08:21:10Z

So how would I turn on deprecation notices now?

nicolas-grekas · 2015-06-08T08:51:42Z

By adding a user land error handler, like the one provided by e.g. the debug component

reecefowell · 2015-06-08T09:51:00Z

Comments addressed in code, also tests are now passing. Somehow though I seem to have broken fabbot.io.

The patch just gave me this

curl http://fabbot.io/patch/symfony/symfony/14900/eb261d45b038f9af47a92b4e6b602bcf8a853816/cs.diff | patch -p0
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8717  100  8717    0     0  20591      0 --:--:-- --:--:-- --:--:-- 20607
patch: **** Only garbage was found in the patch input.

So I applied the changes manually, but I guess maybe because I pushed another fix while fabbot.io was still running it broke it this time around.

Fabbot.io was talking about code I never touched though. So I guess those problems were already there.

Apparently the patch only contains garbage.... 😕

Other than that. Good to go.

weaverryan · 2015-06-08T19:16:46Z

To summarize a few questions people had (that I also had):

A) Do you still get deprecated warnings in the web debug toolbar? YES

B) Do deprecation warnings still show up in the logs? NO

Does this have any other side effects?

If there are no other side effects, I'm +1. We can add something to the "how to get rid of deprecation warnings" section of the docs to show people how to hook up an error handler to log the deprecation warnings.

fabpot · 2015-06-09T14:46:51Z

Ready to be merged?

phansys · 2015-06-09T16:15:55Z

Nice! Thanks for the clarification @nicolas-grekas.

Tobion · 2015-06-09T17:16:37Z

Would it not be better to change the production error handler instead to not log by default if that is what we want? Triggering deprecations and at the same time silence them seems like a hack.

nicolas-grekas · 2015-06-10T06:27:06Z

@Tobion this PR solves a real issue/pain users experiment with 2.7.: they must opt-out from deprecation notices. If we think this is required then we should not merge this PR.
If we think the deprecations should be opt-in, as would be migrating 3.0, then this PR is the right one.
Nobody but me voted here. Please do @symfony/deciders

nicolas-grekas · 2015-06-10T06:28:42Z

@reecefowell please rebase

lsmith77 · 2015-06-10T07:13:44Z

I agree that this should have been opt-in rather than opt-out. I do not know if there is a more elegant approach as I agree with @Tobion it seems hacky to use @trigger_error().

nicolas-grekas · 2015-06-10T07:53:52Z

I understand how you can see this as a hack. There is an other consideration that makes me think this is really the best: performance. We could wrap the implementation of triggering "opt-in deprecation notices" behind some more semantic interface. But that would add overhead to something that needs to be as fast as possible. This is not a micro-optim when this can be called thousands of times.
Thus this raw-bare-metal-php implementation that may look hacky, but is just the right implementation for our needs. At least that's my humble opinion :)

nicolas-grekas · 2015-06-10T07:55:10Z

UPGRADE-2.7.md

@@ -5,7 +5,7 @@ Global
 ------

 * `E_USER_DEPRECATED` warnings -
-   `trigger_error('... is deprecated ...', E_USER_DEPRECATED)` -
+   `@trigger_error('... is deprecated ...', E_USER_DEPRECATED)` -
   are now triggered when using all deprecated functionality.
   To avoid filling up error logs, you may need to add


This sentence looks wrong now, the UPGRADE needs an update

fabpot · 2015-06-10T14:00:53Z

I'm going to merge this today if nobody vote otherwise.

jakzal · 2015-06-10T14:43:55Z

👍 as I see people struggling with deprecation messages, especially in cli. However, we should provide clear instructions on how to enable them.

stof · 2015-06-10T14:47:06Z

👍

It also means we are much less intrusive for other libraries relying on Symfony 2.x

matthias-chlechowitz · 2015-06-10T14:48:36Z

👍

aitboudad · 2015-06-10T14:48:48Z

👍

xabbuh · 2015-06-10T15:27:50Z

I am still not fully convinced. How would people now see the stack trace for a triggered deprecation if it is not contained in the logs? The debug toolbar doesn't provide one.

This PR was merged into the 2.7 branch. Discussion ---------- [RFC] Fixing Deprecation Warnings This PR address an issue that causes Symfony to vomit E_DEPRECATED warnings everywhere. ```markdown | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14899 | License | MIT ``` Fixes #14899 ![http://cdn.meme.am/instances2/500x/125359.jpg](http://cdn.meme.am/instances2/500x/125359.jpg) Commits ------- 73bbaa6 Silence invasive deprecation warnings, opt-in for warnings

nicolas-grekas · 2015-06-10T15:36:29Z

Thank you @reecefowell

nicolas-grekas · 2015-06-10T15:37:42Z

@xabbuh we still have to work on better deprecations reporting.
But at least, we can do it quietly now, without hurting users.

reecefowell · 2015-06-10T15:51:20Z

Thanks everyone :)

craue · 2015-06-10T15:52:32Z

Are bundle developers encouraged to do this as well?

nicolas-grekas · 2015-06-10T15:52:57Z

@craue I'd say yes!

mmoreram · 2015-06-11T23:02:31Z

👍 Good move :)

xabbuh closed this Jun 7, 2015

nicolas-grekas reopened this Jun 7, 2015

nicolas-grekas reviewed Jun 7, 2015
View reviewed changes

reecefowell reviewed Jun 8, 2015
View reviewed changes

reecefowell force-pushed the 2.7 branch 2 times, most recently from eb261d4 to 00ac865 Compare June 8, 2015 09:26

Silence invasive deprecation warnings, opt-in for warnings

73bbaa6

reecefowell force-pushed the 2.7 branch from 00ac865 to 73bbaa6 Compare June 8, 2015 09:37

nicolas-grekas added the Ready label Jun 10, 2015

nicolas-grekas reviewed Jun 10, 2015
View reviewed changes

nicolas-grekas merged commit 73bbaa6 into symfony:2.7 Jun 10, 2015

reecefowell deleted the 2.7 branch June 10, 2015 16:46

smurfy mentioned this pull request Jun 12, 2015

Fix deprecated calls schmittjoh/JMSAopBundle#26

Closed

xabbuh mentioned this pull request Jun 15, 2015

[DependencyInjection] Deprecate scope concept #14984

Merged

linaori mentioned this pull request Jun 15, 2015

Do not trigger deprecation error in ResolveParameterPlaceHoldersPass #14988

Closed

xabbuh mentioned this pull request Jul 15, 2015

[Contributing][Code] deprecation triggers must be silenced by default symfony/symfony-docs#5525

Closed

nicoschoenmaker mentioned this pull request Feb 19, 2016

[DX] Logging deprecations #17858

Closed

nicolas-grekas mentioned this pull request Dec 6, 2017

Ensure column options map to an existing method doctrine/dbal#2846

Merged

alcaeus mentioned this pull request Apr 24, 2018

PHPLIB-346: Deprecate autoIndexId option for CreateCollection mongodb/mongo-php-library#533

Merged

[RFC] Fixing Deprecation Warnings #14900

[RFC] Fixing Deprecation Warnings #14900

Conversation

reecefowell commented Jun 7, 2015

perk11 commented Jun 7, 2015

xabbuh commented Jun 7, 2015

nicolas-grekas commented Jun 7, 2015

elnur commented Jun 7, 2015

xabbuh commented Jun 7, 2015

nicolas-grekas commented Jun 7, 2015

xabbuh commented Jun 7, 2015

reecefowell commented Jun 7, 2015

nicolas-grekas commented Jun 7, 2015

nicolas-grekas commented Jun 7, 2015

nicolas-grekas Jun 7, 2015

Choose a reason for hiding this comment

stof commented Jun 7, 2015

nicolas-grekas commented Jun 7, 2015 via email

fabpot commented Jun 7, 2015

Tobion commented Jun 7, 2015

nicolas-grekas commented Jun 8, 2015

reecefowell Jun 8, 2015

Choose a reason for hiding this comment

nicolas-grekas Jun 8, 2015

Choose a reason for hiding this comment

linaori commented Jun 8, 2015

nicolas-grekas commented Jun 8, 2015

reecefowell commented Jun 8, 2015

weaverryan commented Jun 8, 2015

fabpot commented Jun 9, 2015

phansys commented Jun 9, 2015

Tobion commented Jun 9, 2015

nicolas-grekas commented Jun 10, 2015

nicolas-grekas commented Jun 10, 2015

lsmith77 commented Jun 10, 2015

nicolas-grekas commented Jun 10, 2015

nicolas-grekas Jun 10, 2015

Choose a reason for hiding this comment

fabpot commented Jun 10, 2015

jakzal commented Jun 10, 2015

stof commented Jun 10, 2015

matthias-chlechowitz commented Jun 10, 2015

aitboudad commented Jun 10, 2015

xabbuh commented Jun 10, 2015

nicolas-grekas commented Jun 10, 2015

nicolas-grekas commented Jun 10, 2015

reecefowell commented Jun 10, 2015

craue commented Jun 10, 2015

nicolas-grekas commented Jun 10, 2015

mmoreram commented Jun 11, 2015