NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

Exit as late as possible#26012

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:2.7fromgreg0ire:defer_exit

Feb 11, 2018

Merged

Exit as late as possible#26012

nicolas-grekas merged 1 commit intosymfony:2.7fromgreg0ire:defer_exit

Feb 11, 2018

Conversation

Copy link

Contributor

greg0ire commentedFeb 1, 2018

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.

Seehttps://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

carsonbot added Status: Needs Review Bug labels

Feb 1, 2018

Copy link

ContributorAuthor

nicolas-grekasFeb 2, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 :) )

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The downside being that if a shutdown function usesexit 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.

Copy link

Member

nicolas-grekasFeb 2, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

as you want, both are fine to me :)

Copy link

ContributorAuthor

greg0ireFeb 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'll see what I can manage

Copy link

ContributorAuthor

greg0ireFeb 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

chalasr added this to the2.7 milestone

Feb 2, 2018

greg0ire force-pushed thedefer_exit branch fromfc1e6b7 to79334efCompare

February 3, 2018 12:27

Copy link

ContributorAuthor

greg0ire commentedFeb 3, 2018

Best reviewed whenignoring 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 commented

Feb 3, 2018

View reviewed changes

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


		1x: root deprecation

		Deprecations during shutdown

Copy link

ContributorAuthor

greg0ireFeb 3, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Member

nicolas-grekasFeb 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

not more than existing titles, so as is LGTM

greg0ire commented

Feb 3, 2018

View reviewed changes

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php Outdated

		}

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

Copy link

ContributorAuthor

greg0ireFeb 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This calls for a data structure refactoring

Copy link

Member

nicolas-grekasFeb 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

greg0ireFeb 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

"fixed" :P

nicolas-grekas reviewed

Feb 4, 2018

View reviewed changes

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php Outdated

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

Copy link

Member

nicolas-grekasFeb 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

greg0ireFeb 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

done

Copy link

Contributor

alcaeus commentedFeb 4, 2018

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.

greg0ire force-pushed thedefer_exit branch 2 times, most recently from07fe27a tod0e85e5Compare

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

Copy link

Member

nicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

greg0ire commentedFeb 4, 2018•
edited
Loading

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

Copy link

ContributorAuthor

greg0ire commentedFeb 4, 2018•
edited
Loading

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 force-pushed thedefer_exit branch fromd0e85e5 to9f71764Compare

February 4, 2018 15:37

Copy link

ContributorAuthor

greg0ire commentedFeb 4, 2018•
edited
Loading

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

Copy link

ContributorAuthor

greg0ire commentedFeb 4, 2018

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

Copy link

ContributorAuthor

greg0ire commentedFeb 4, 2018

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

nicolas-grekas approved these changes

Feb 11, 2018

View reviewed changes

nicolas-grekas reviewed

Feb 11, 2018

View reviewed changes

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php Outdated

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

Copy link

Member

nicolas-grekasFeb 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

nicolas-grekas force-pushed thedefer_exit branch from9f71764 tobf78d74Compare

February 11, 2018 14:54

[Bridge\PhpUnit] Exit as late as possible

97370a3

People might want to register other shutdown functions that should beable to control the exit code themselves, without the deprecation errorhandler 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.Seehttps://secure.php.net/manual/en/function.register-shutdown-function.php

nicolas-grekas force-pushed thedefer_exit branch frombf78d74 to97370a3Compare

February 11, 2018 14:57

Copy link

Member

nicolas-grekas commentedFeb 11, 2018

Thank you@greg0ire.

nicolas-grekas merged commit97370a3 intosymfony:2.7

Feb 11, 2018

nicolas-grekas added a commit that referenced this pull request

Feb 11, 2018

bug#26012Exit as late as possible (greg0ire)

ec9a109

This PR was merged into the 2.7 branch.Discussion----------Exit as late as possiblePeople might want to register other shutdown functions that should beable to control the exit code themselves, without the deprecation errorhandler 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.Seehttps://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<!--- Bug fixes must be submitted against the lowest branch where they apply  (lowest branches are regularly merged to upper ones so they get the fixes too).- Features and deprecations must be submitted against the master branch.- Replace this comment by a description of what your PR is solving.-->Commits-------97370a3 [Bridge\PhpUnit] Exit as late as possible

greg0ire deleted the defer_exit branch

February 11, 2018 16:17

This was referencedFeb 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

Labels

Bug Status: Reviewed


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

Movatterモバイル変換

Uh oh!

Exit as late as possible#26012

Exit as late as possible#26012

Uh oh!

Conversation

greg0ire commentedFeb 1, 2018

Uh oh!

greg0ire commentedFeb 1, 2018

Uh oh!

nicolas-grekasFeb 2, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasFeb 2, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commentedFeb 3, 2018

Uh oh!

greg0ireFeb 3, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcaeus commentedFeb 4, 2018

Uh oh!

nicolas-grekas left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commentedFeb 4, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

greg0ire commentedFeb 4, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

greg0ire commentedFeb 4, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

greg0ire commentedFeb 4, 2018

Uh oh!

greg0ire commentedFeb 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedFeb 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nicolas-grekasFeb 2, 2018•
edited
Loading

nicolas-grekasFeb 2, 2018•
edited
Loading

greg0ireFeb 3, 2018•
edited
Loading

nicolas-grekas left a comment•
edited
Loading

greg0ire commentedFeb 4, 2018•
edited
Loading

greg0ire commentedFeb 4, 2018•
edited
Loading

greg0ire commentedFeb 4, 2018•
edited
Loading