ZF-4286: PHP warning in Zend_Mail_Protocol_Abstract->_connect()
Description
The call to stream_socket_client() should be prepended with an @-sign to avoid warnings on connection timeout and probably also on other errors.
Kind regards, Thomas Gelf
The call to stream_socket_client() should be prepended with an @-sign to avoid warnings on connection timeout and probably also on other errors.
Kind regards, Thomas Gelf
Comments
Posted by Nico Edtinger (nico) on 2008-09-15T03:12:07.000+0000
Warnings are not enabled in production. Thus hiding a warning IMO only makes development harder.
Posted by Thomas Gelf (tgelf) on 2008-09-15T03:26:50.000+0000
You didn't even look at the source before closing this, did you?
The _connect() function checks whether opening the socket failed and throws an exception if so. Exceptions are fine and can be catched, native PHP warnings are absolutely useless here and would keep filling up my logs. Not every productional PHP code is a web application - I have a lot of code executed by various cronjobs and daemons, all of them running with E_ALL and E_STRICT enabled. If some warning appears our sysadmins immediately get a notification by mail.
Cheers, Thomas
Posted by Thomas Gelf (tgelf) on 2008-09-20T06:07:39.000+0000
As you've set this ticket to "unassigned" without any comment I took some time to reread "ZF coding standards", just to be sure. Regarding "Errors and Exceptions" they clearly state:
"The Zend Framework codebase must be E_STRICT compliant. Zend Framework code should not emit PHP warning (E_WARNING, E_USER_WARNING), notice (E_NOTICE, E_USER_NOTICE), or strict (E_STRICT) messages when error_reporting is set to E_ALL | E_STRICT."
Even if the "should not" is not enforcing it for me this point is pretty clear and no further discussion is needed.
I'll change this ticket to be reassigned automatically and also change priority and type - it's not an improvement, it's a coding standards violation.
Best regards, Thomas Gelf
Posted by Thomas Gelf (tgelf) on 2008-09-20T06:14:53.000+0000
Attached patch file for current SVN.
Posted by Matthew Weier O'Phinney (matthew) on 2008-10-14T12:40:25.000+0000
Probably the best situation here is to use error_get_last() when throwing the exception indicating that opening the stream failed. I think the patch Thomas has, combined with that information, would fit both criteria. (error_get_last() will get the last error message, even when error suppression occurred).
Posted by Thomas Gelf (tgelf) on 2008-10-15T03:23:55.000+0000
Thank you, Matthew! However: error_get_last() isn't required, stream_socket_client() is already writing errors to $errorStr, and this error is thrown as Zend_Mail_Protocol_Exception in case of an eventual error.
Posted by Nico Edtinger (nico) on 2008-11-07T16:04:56.000+0000
stream_socket_client() now got the shutup operator.
I also did take a look at the additional options. error_get_last() might give a message if there is a possibility for stream_socket_client() to fail without a warning. Would be better to add that to Zend_Exception. It would also help with all other suppressed warnings, as no component uses or returns that information yet.
Writing a test for this task would mean using a error_handler, as I couldn't find an other way to know if a warning was raised, but suppressed. Not worth it.
Posted by Wil Sinclair (wil) on 2008-11-13T14:10:17.000+0000
Changing issues in preparation for the 1.7.0 release.