Issues

ZF-4286: PHP warning in Zend_Mail_Protocol_Abstract->_connect()

Issue Type: Coding Standards Violation Created: 2008-09-14T06:17:39.000+0000 Last Updated: 2008-11-13T14:10:17.000+0000 Status: Resolved Fix version(s): - 1.7.0 (17/Nov/08)

Reporter: Thomas Gelf (tgelf) Assignee: Nico Edtinger (nico) Tags: - Zend_Mail

Related issues: Attachments: - zend_mail_protocol_abstract-warning.patch

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

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.

Have you found an issue?

See the Overview section for more details.

Copyright

© 2006-2016 by Zend, a Rogue Wave Company. Made with by awesome contributors.

This website is built using zend-expressive and it runs on PHP 7.

Contacts