Exit as late as possible #26012

greg0ire · 2018-02-01T23:07:43Z

People might want to register other shutdown functions that should be
able to control the exit code themselves, without the deprecation error
handler taking over. The php manual says:

If you call exit() within one registered shutdown function, processing
will stop completely and no other registered shutdown functions will be
called.

See https://secure.php.net/manual/en/function.register-shutdown-function.php

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT

greg0ire · 2018-02-01T23:07:56Z

@alcaeus please review

nicolas-grekas · 2018-02-02T13:34:52Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

@@ -151,7 +151,9 @@ public static function register($mode = false)
                }

                if ('weak' !== $mode && ($deprecations['unsilenced'] || $deprecations['remaining'] || $deprecations['other'])) {
-                    exit(1);
+                    register_shutdown_function(function () {


what would be even better would be to defer the whole closure, so that deprecations triggered at shutdown time are also caught (and add a related test of course :) )

The downside being that if a shutdown function uses exit and the whole closure is deferred too, then you won't get any deprecations. But I think catching shutdown time deprecations might outweight this, what do you think? If we want to go crazy, we may even reset the state after the first, non deferred display and if any deprecations are caught while shutting down, then display them in the deferred closure.

as you want, both are fine to me :)

I'll see what I can manage

Ended up "going crazy", tell me what you think

greg0ire · 2018-02-03T12:29:44Z

Best reviewed when ignoring whitespace

@nicolas-grekas the second commit feels more like a feature than a bugfix to me. Would you be ok with moving it to a separate PR?

greg0ire · 2018-02-03T12:30:44Z

src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/shutdown_deprecations.phpt

+
+  1x: root deprecation
+
+Deprecations during shutdown


Please tell me if you want more emphasis here and how it should look like (ascii underline with = signs, maybe?), and if I should also add some header for the previous section.

not more than existing titles, so as is LGTM

greg0ire · 2018-02-03T13:07:26Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

                }
+
+                register_shutdown_function(function () use (&$deprecations, $isFailing, $displayDeprecations) {
+                    if (array_sum(array_filter($deprecations, function ($key) {


This calls for a data structure refactoring

let's do a code-style fix for now :) the "if" should be on one line

nicolas-grekas · 2018-02-04T10:25:29Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+                        echo "Deprecations during shutdown\n";
+                    }
+                    $displayDeprecations($deprecations);
+                    if ($isFailing) {


if there are shutdown-time deprecations, shouldn't this fail also?

alcaeus · 2018-02-04T11:31:23Z

the second commit feels more like a feature than a bugfix to me

TBH, I'd still consider it a bugfix: the original bug was that any existing deprecations cause other shutdown handlers to be ignored. This is fixed in your first commit. Fixing that bug exposes another bug, namely that any deprecations happening in other shutdown handlers don't reported. This is fixed in your second commit.

nicolas-grekas

(failures should be fixed once merged up to master, isn't it?)

greg0ire · 2018-02-04T15:03:39Z

Correct, but let me try to test it on 5.4 to be 100% sure.

greg0ire · 2018-02-04T15:24:09Z

Don't merge, I get this, which seems to be a bug since I used the same version as in Travis:

Fatal error: Class 'PHPUnit_Util_ErrorHandler' not found in /tmp/sf/src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php on line 54

Then, with a var_dump($msg) on the offending line, this:

"Use of undefined constant ARRAY_FILTER_USE_KEY - assumed 'ARRAY_FILTER_USE_KEY'"

This can't work for php < 5.6

greg0ire · 2018-02-04T15:37:39Z

I fixed it however I could, it should be good to merge now (although Travis is still red)

greg0ire · 2018-02-04T15:39:20Z

Why exactly do we have those Count indices, BTW? I suppose the answer is performance, somehow?

greg0ire · 2018-02-04T16:25:35Z

Tested with docker run -it -v $PWD:/tmp/sf squallcx/docker-php54-apache /bin/sh

nicolas-grekas · 2018-02-11T11:35:56Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+                register_shutdown_function(function () use (&$deprecations, $isFailing, $displayDeprecations, $mode) {
+                    foreach ($deprecations as $group => $arrayOrInt) {
+                        if (is_int($arrayOrInt) && $arrayOrInt > 0 || is_array($arrayOrInt) && count($arrayOrInt) > 0) {
+                            echo "Deprecations during shutdown\n";


Small tweak: s/Deprecations during shutdown/Shutdown-time deprecations:/

People might want to register other shutdown functions that should be able to control the exit code themselves, without the deprecation error handler taking over. The php manual says: > If you call exit() within one registered shutdown function, processing > will stop completely and no other registered shutdown functions will be > called. See https://secure.php.net/manual/en/function.register-shutdown-function.php

nicolas-grekas · 2018-02-11T15:00:23Z

Thank you @greg0ire.

This PR was merged into the 2.7 branch. Discussion ---------- Exit as late as possible People might want to register other shutdown functions that should be able to control the exit code themselves, without the deprecation error handler taking over. The php manual says: > If you call exit() within one registered shutdown function, processing > will stop completely and no other registered shutdown functions will be > called. See https://secure.php.net/manual/en/function.register-shutdown-function.php | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT  Commits ------- 97370a3 [Bridge\PhpUnit] Exit as late as possible

carsonbot added Status: Needs Review Bug labels Feb 1, 2018

nicolas-grekas reviewed Feb 2, 2018

View reviewed changes

chalasr added this to the 2.7 milestone Feb 2, 2018

greg0ire force-pushed the defer_exit branch from fc1e6b7 to 79334ef Compare February 3, 2018 12:27

greg0ire commented Feb 3, 2018

View reviewed changes

nicolas-grekas reviewed Feb 4, 2018

View reviewed changes

greg0ire force-pushed the defer_exit branch 2 times, most recently from 07fe27a to d0e85e5 Compare February 4, 2018 14:02

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 4, 2018

nicolas-grekas approved these changes Feb 4, 2018

View reviewed changes

greg0ire force-pushed the defer_exit branch from d0e85e5 to 9f71764 Compare February 4, 2018 15:37

nicolas-grekas approved these changes Feb 11, 2018

View reviewed changes

nicolas-grekas reviewed Feb 11, 2018

View reviewed changes

nicolas-grekas force-pushed the defer_exit branch from 9f71764 to bf78d74 Compare February 11, 2018 14:54

nicolas-grekas force-pushed the defer_exit branch from bf78d74 to 97370a3 Compare February 11, 2018 14:57

nicolas-grekas merged commit 97370a3 into symfony:2.7 Feb 11, 2018

greg0ire deleted the defer_exit branch February 11, 2018 16:17

This was referenced Feb 28, 2018

Release v2.7.42 #26345

Closed

Release v2.7.42 #26346

Merged

Release v2.8.35 #26361

Merged

Release v3.4.5 #26362

Merged

Release v4.0.5 #26363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exit as late as possible #26012

Exit as late as possible #26012

greg0ire commented Feb 1, 2018

greg0ire commented Feb 1, 2018

nicolas-grekas Feb 2, 2018 •

edited

Loading

greg0ire Feb 2, 2018

nicolas-grekas Feb 2, 2018 •

edited

Loading

greg0ire Feb 2, 2018

greg0ire Feb 3, 2018

greg0ire commented Feb 3, 2018

greg0ire Feb 3, 2018 •

edited

Loading

nicolas-grekas Feb 4, 2018

greg0ire Feb 3, 2018

nicolas-grekas Feb 4, 2018

greg0ire Feb 4, 2018

nicolas-grekas Feb 4, 2018

greg0ire Feb 4, 2018

alcaeus commented Feb 4, 2018

nicolas-grekas left a comment •

edited

Loading

greg0ire commented Feb 4, 2018 •

edited

Loading

greg0ire commented Feb 4, 2018 •

edited

Loading

greg0ire commented Feb 4, 2018 •

edited

Loading

greg0ire commented Feb 4, 2018

greg0ire commented Feb 4, 2018

nicolas-grekas Feb 11, 2018

nicolas-grekas commented Feb 11, 2018

Exit as late as possible #26012

Exit as late as possible #26012

Conversation

greg0ire commented Feb 1, 2018

greg0ire commented Feb 1, 2018

nicolas-grekas Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Feb 3, 2018

greg0ire Feb 3, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus commented Feb 4, 2018

nicolas-grekas left a comment • edited Loading

Choose a reason for hiding this comment

greg0ire commented Feb 4, 2018 • edited Loading

greg0ire commented Feb 4, 2018 • edited Loading

greg0ire commented Feb 4, 2018 • edited Loading

greg0ire commented Feb 4, 2018

greg0ire commented Feb 4, 2018

Choose a reason for hiding this comment

nicolas-grekas commented Feb 11, 2018

nicolas-grekas Feb 2, 2018 •

edited

Loading

nicolas-grekas Feb 2, 2018 •

edited

Loading

greg0ire Feb 3, 2018 •

edited

Loading

nicolas-grekas left a comment •

edited

Loading

greg0ire commented Feb 4, 2018 •

edited

Loading

greg0ire commented Feb 4, 2018 •

edited

Loading

greg0ire commented Feb 4, 2018 •

edited

Loading