[HttpFoundation] Use convention to allow throwing from __toString() #15077

nicolas-grekas · 2015-06-23T11:40:37Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

Corollary to #15076, works without it.

Tobion · 2015-06-23T11:59:35Z

src/Symfony/Component/HttpFoundation/Request.php

@@ -480,7 +480,7 @@ public function __toString()
        try {
            $content = $this->getContent();
        } catch (\LogicException $e) {
-            trigger_error($e->getMessage(), E_USER_ERROR);
+            return trigger_error($e, E_USER_ERROR);


trigger_error expects a string as first parameter and not an exception?! Also this changes the return value of the method. The method is supposed to return a string and not a boolean from trigger_error.

So this is a bc break.

The old return value is not returned anymore.

This is always the last executed line of the method, and return is bypassed (see #15077 (comment))

it depends on which error handler is used.

You may be did not notice that the very line (the original trigger_error($e->getMessage(), E_USER_ERROR);) has been introduced a few days ago. It is not in any tagged version of Symfony. And the original intent and behavior (even before this very line was introduced itself) was really to stop the execution flow at this line.

nicolas-grekas · 2015-06-23T12:08:51Z

In any case, trigger_error(..., E_USER_ERROR); is the last line of the method, because E_USER_ERROR either kills the process or throws (which kills the process with a fatal error). But when an error handler like the one in #15076 is in place, it is possible to recover, and it's the only way to do that.

About the argument type, $e is automatically casted to a string (__toString() is in the Throwable interface) and this is the most accurate error message: the generated string contains the type of the exception, its message + stack trace excerpt. They are all required to debug correctly. $e->getMessage() is not enought.

Tobion · 2015-06-23T12:16:07Z

It's not possible to gracefully handle an E_USER_ERROR and continue the programm?

nicolas-grekas · 2015-06-23T12:28:40Z

Technically it's possible with a custom error handler, but that would be wrong as it's not the semantic of E_USER_ERROR.

Tobion · 2015-06-23T12:44:06Z

The semantic of trigger_error is that it allows to continue the program flow of the method (which is the main difference to an exception). So return trigger_error breaks this contract and makes no sense to me.

nicolas-grekas · 2015-06-23T12:46:54Z

This is not the semantic for E_USER_ERROR: http://3v4l.org/6rDU4

nicolas-grekas · 2015-06-23T12:48:58Z

Which is also why E_USER_ERROR (and E_RECOVERABLE ERROR) can't be made to continue, but are forced to throw:

symfony/src/Symfony/Component/Debug/ErrorHandler.php

Line 254 in 27e6f48

    
           $this->thrownErrors = ($levels | E_RECOVERABLE_ERROR | E_USER_ERROR) & ~E_USER_DEPRECATED & ~E_DEPRECATED;

Tobion · 2015-06-23T14:01:32Z

This is not the semantic for E_USER_ERROR: http://3v4l.org/6rDU4

But this is only the default error handler. A custom one can do things differently. And for those the behavior changes. This is probably an edge case, but for me this code stand-alone makes no sense.
I'm not against it, but it's pretty much a hack which is bound to #15076. Without the debug error handler this change just creates an invalid toString() return value. That also means static code analysis will easily mark this as invalid.

So we can just as well provide a convenience method in the debug component for return trigger_error which is then used (which makes it a dependency).

nicolas-grekas · 2015-06-23T14:34:40Z

When looking for the perfect convention that would allow working around the restriction, I had in mind to not mandate a dependency on the debug component. I still think this is very important.
The convention I propose in #15076, which is applied here, is IMO the best: it allows for an enhanced experience when a non-compliant error handler is in use (native one or any other custom one), and opens for an even more enhanced experience when a compliant error handler is in use.

Tobion · 2015-06-23T14:47:08Z

It's ugly but a sensible workaround. 👍

nicolas-grekas · 2015-06-23T14:49:17Z

That's why I like this Debug component :)

…_toString() (nicolas-grekas) This PR was merged into the 2.3 branch. Discussion ---------- [HttpFoundation] Use convention to allow throwing from __toString() | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Corollary to #15076, works without it. Commits ------- 8982c32 [HttpFoundation] Use convention to allow throwing from __toString()

[HttpFoundation] Use convention to allow throwing from __toString()

8982c32

Tobion reviewed Jun 23, 2015
View reviewed changes

nicolas-grekas added the Ready label Jun 23, 2015

nicolas-grekas merged commit 8982c32 into symfony:2.3 Jun 25, 2015

nicolas-grekas deleted the tostring-throw branch June 25, 2015 09:06

Uh oh!

[HttpFoundation] Use convention to allow throwing from __toString() #15077

[HttpFoundation] Use convention to allow throwing from __toString() #15077

Uh oh!

Conversation

nicolas-grekas commented Jun 23, 2015

Uh oh!

Tobion Jun 23, 2015

Choose a reason for hiding this comment

Uh oh!

Tobion Jun 23, 2015

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jun 23, 2015

Choose a reason for hiding this comment

Uh oh!

Tobion Jun 23, 2015

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jun 23, 2015

Choose a reason for hiding this comment

Uh oh!

Tobion Jun 23, 2015

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jun 23, 2015

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jun 23, 2015

Uh oh!

Tobion commented Jun 23, 2015

Uh oh!

nicolas-grekas commented Jun 23, 2015

Uh oh!

Tobion commented Jun 23, 2015

Uh oh!

nicolas-grekas commented Jun 23, 2015

Uh oh!

nicolas-grekas commented Jun 23, 2015

Uh oh!

Tobion commented Jun 23, 2015

Uh oh!

nicolas-grekas commented Jun 23, 2015

Uh oh!

Tobion commented Jun 23, 2015

Uh oh!

nicolas-grekas commented Jun 23, 2015

Uh oh!

Uh oh!