Skip to content

Conversation

nikic
Copy link
Member

@nikic nikic commented Apr 7, 2015

The fact that engine exceptions currently display as normal fatal errors is causing a lot of confusion, we will need to switch this to proper exception messages before the release. This PR cleans up the current exception messages to make it feasible to use them for engine exceptions as well.

Old message:

Fatal error: Uncaught exception 'UnexpectedValueException' with message 'Failed to open directory ""' in %s:%d
Stack trace:
#0 %s(%d): DirectoryIterator->__construct('\x00/abc')
#1 {main}
  thrown in %s on line %d

New message:

UnexpectedValueException: Failed to open directory "" in %s on line %d
Stack trace:
#0 %s(%d): DirectoryIterator->__construct('\x00/abc')
#1 {main}

Fallout: The error message for an "uncaught exception" is no longer based on __toString. Instead a canonical representation is always used.

Internal changes: The error callback now accepts two additional arguments: Firstly, the error type string (like Fatal error or UnexpectedValueException), which can now be changed independently of the error type. Secondly "additional information" which is to be displayed after the message, file and line information. This is used for exceptions to display the stack trace and "next" exceptions.

nikic added 3 commits April 7, 2015 17:30
Previously this was overriding Exception::__toString(). Now
setting with the fault code directly.
@Kubo2
Copy link
Contributor

Kubo2 commented Apr 11, 2015

@nikic 💯

@smalyshev
Copy link
Contributor

This will break any pre-7 tool that tried to look for PHP error messages, and also will make PHP exception messages non-machine-detectable as they won't have neither common prefix nor even common pattern. I don't think this is such a good idea. 👎

@hikari-no-yume
Copy link
Contributor

To make exceptions easily detectable, you could prepend Uncaught.

But I don't see why this would make looking for PHP errors harder, just look for Uncaught EngineException:. Tools can be updated, but humans come first.

@Danack
Copy link
Contributor

Danack commented Apr 25, 2015

also will make PHP exception messages non-machine-detectable as they won't have neither common prefix nor even common pattern.

That will only affect people who refuse to have a top level try/catch block.

And who also refuse to use the set_exception_handler to handle uncaught exceptions.

TazeTSchnitzel wrote:

Tools can be updated, but humans come first.

+1

@marcioAlmada
Copy link
Contributor

Even though this wasn't noted in advance during the RFC discussion, closing all "bugs" opened by people confused with the error message is not sustainable - not to mention bad for the users.

We should update the error messages and avoid confusion in the first place.

👍

@smalyshev
Copy link
Contributor

I don't see any reason why current message is unsuitable for humans it - reads just fine and the advantage in new way is minimal, while breakage of the tools is substantial. I think dismissing this breakage by saying "tools can be updated" is very nearsighted, and not providing any means for automatic detection of errors is even worse.

@hikari-no-yume
Copy link
Contributor

Depends which message you mean. Shortening the exception message may have less benefit, but given that errors produce EngineException now, they need to show up as such.

nikic added a commit to nikic/php-src that referenced this pull request May 17, 2015
This implements a reduced variant of php#1226 with just the following
change:

-Fatal error: Uncaught exception 'EngineException' with message 'Call to private method foo::bar() from context ''' in %s:%d
+Fatal error: Uncaught EngineException: Call to private method foo::bar() from context '' in %s:%d

The '' wrapper around messages is very weird if the exception
message itself contains ''. Futhermore having the message wrapped
in '' doesn't work for the "and defined" suffix of
TypeExceptions.
Kubo2 pushed a commit to Kubo2/php-src that referenced this pull request May 22, 2015
This implements a reduced variant of php#1226 with just the following
change:

-Fatal error: Uncaught exception 'EngineException' with message 'Call to private method foo::bar() from context ''' in %s:%d
+Fatal error: Uncaught EngineException: Call to private method foo::bar() from context '' in %s:%d

The '' wrapper around messages is very weird if the exception
message itself contains ''. Futhermore having the message wrapped
in '' doesn't work for the "and defined" suffix of
TypeExceptions.
@nikic
Copy link
Member Author

nikic commented May 25, 2015

Closing as I don't plan to further pursue this.

@nikic nikic closed this May 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants