twilio / OpenVBX

OpenVBX is a web-based open source phone system for business.
http://openvbx.org
Other
699 stars 342 forks source link

PHP Errors in JSON Response #331

Closed andrewspode closed 9 years ago

andrewspode commented 9 years ago

I have PHP Error Output disabled in my PHP Configuration. Several things fail (adding new users etc.) but actually succeed.

Looking at the response in Chrome, I'm seeing a well formed JSON response (success) but above it some PHP errors being outputted.

Version 1.2.17. Example Output:

<div style="border:1px solid #990000;padding-left:20px;margin:0 0 10px 0;">

<h4>A PHP Error was encountered</h4>

<p>Severity: 8192</p>
<p>Message:  Non-static method PhoneNumber::validatePhoneNumber() should not be called statically, assuming $this from incompatible context</p>
<p>Filename: models/vbx_device.php</p>
<p>Line Number: 92</p>

</div>{"error":true,"message":"Number is not a US, Canadian or toll free phone number"}
andrewspode commented 9 years ago

I've checked all the config files and it's set NOT to output PHP errors.

In the end, I've edited Exceptions.php and done a "return false" at the top of the "show_php_error" function to repress the output.

Gipetto commented 9 years ago

Do you have a list of actions that fail for you? If you give me output from these actions I can most likely fix them up pretty easily. There are a lot of things in the OpenVBX code that PHP has since gotten more strict about.

Gipetto commented 9 years ago

Also, if you're interested, you can return Exceptions.php to its previous state and fix the phone number validation by altering a function signature. See https://github.com/twilio/OpenVBX/commit/5ebce66cc4b04ee37c67ca7141df517599836de0 for the fix.

andrewspode commented 9 years ago

From memory, it's mostly functions that need "static" added into their function declarations. However, these aren't fatal errors so don't necessarily need fixing. Validating a phone number was one that comes to mind. (note - just saw your fix, so you're way ahead of me on this one)

The bigger issue (IMO) is that according the Code Igniter configuration, PHP errors shouldn't be shown, but rather logged. So why are they? I would tackle that issue, before fixing non-fatal warnings and notifications.

Gipetto commented 9 years ago

CodeIgniter's configuration is separate from PHP's error reporting configuration. For example my PHP error_reporting ini value is 32767 but display_errors is Off. This will have more effect on what you want than CI's settings. CI is probably also doing what its told, but you should check your phpinfo() output for the error reporting configuration. In fact, the index.php file should override these values unless your server is set to disallow php ini overrides from scripts.

andrewspode commented 9 years ago

My PHP configuration is set not to output anything.

The errors I'm seeing outputted are not in the traditional PHP way, they are from the function in Exceptions.php i.e CI is echoing out the errors, not PHP.

This is definitely an issue with OpenVBX and/or CI, not a PHP setup.

Gipetto commented 9 years ago

I just looked at those config variables and they're not used at all. It appears as though someone intended to make error reporting and display configurable but never did. If you do a search for display_errors you'll see that the config item never gets used.

There appears to be a few duplicate ini_set declarations that are redundant since index.php sets them as one of the first things that happens.

If you look in system/codeigniter/Common.php on line 403 the system error reporting level is used to determine wether to show the error or not. The warning you got was a strict coding standard warning that was probably added to PHP after error reporting was set up in OpenVBX. Strict warnings aren't covered in the error level setting in index.php. I fear that different versions of PHP may also handle the E_STRICT warnings slightly differently which may be why you see it and I don't.

If you could try something for me that would be great: in index.php on line 60 if you could add ^ E_STRICT to the list of constants and let me know if that hides the error. If so I can just add that in as a default and we can sail right over this.

Gipetto commented 9 years ago

I shipped an update to the error handling configuration in 1.2.18. Closing this.