Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove error suppression from mssqlnative (and more?) driver #433

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

maglnet
Copy link

@maglnet maglnet commented Jul 9, 2018

The @-Operator suppresses ALL errors and resulted in a script that exited/aborted without any error message because the error (memory limit exceeded) happened within the sqlsrv_fetch_array function.

I did not get the advantage of suppressing the errors and I currently don't see any problems within our application with this.

Any information why the error suppression was introduced?
If you wish, I could remove the error suppression from all other drivers as well.

else
$this->fields = @sqlsrv_fetch_array($this->_queryID,SQLSRV_FETCH_ASSOC);
$this->fields = sqlsrv_fetch_array($this->_queryID,SQLSRV_FETCH_ASSOC);
Copy link

Choose a reason for hiding this comment

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

$this->fields = sqlsrv_fetch_array($this->_queryID, ($this->fetchMode & ADODB_FETCH_NUM ? SQLSRV_FETCH_BOTH : SQLSRV_FETCH_ASSOC));

Copy link
Author

Choose a reason for hiding this comment

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

Well, you're right, but my focus is on the @-Operator and any ideas/problems with this.
I may remove the if/else condition later.

@mnewnham
Copy link
Contributor

The philosophy of suppressing all errors has been part of the project from day 1, Although I cannot speak to John Lim's decision as to why this is ,

  • I would certainly argue that in production environments, we should not expose database connection information on web pages.
  • Many of us have complex error trapping methodologies in place in our applications, that rely on the suppression of PHP level errors, and that changing this behavior should not be undertaken lightly.

What should happen is that the $db->debug setting should expose the error to you, and that if it does not, that should be addressed.

I think that before removing the error suppression from drivers, You should get a consensus of approval from the community

@maglnet
Copy link
Author

maglnet commented Jul 11, 2018

Thank you for your answer, but I don't think that suppressing errors is a good idea (there may be exceptions, but not in this case ) :-) .

Displaying Errors In Production:
You're right, displaying errors in prod environments should never happen but logging. This should be done by setting display_errors=off and log_errors=on within your production php.ini as stated here: http://php.net/manual/en/errorfunc.configuration.php#ini.display-errors
There's no need to suppress errors within the application, that's what the PHP configuration should handle for you.

Complex Error Trapping:
This is imho impossible if you encounter Fatal Errors within applications that use php < 7.0 as the execution stops immediately and no error handler can catch fatals. Also $db->debug will not be able to catch fatal errors.

@jk
Copy link

jk commented May 22, 2019

@dregad Do you mind considering this PR? Or giving directions what to do next?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants