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

Add DebugOutput setter function #26

Merged
merged 1 commit into from
Nov 5, 2017
Merged

Add DebugOutput setter function #26

merged 1 commit into from
Nov 5, 2017

Conversation

Bankde
Copy link
Contributor

@Bankde Bankde commented Nov 5, 2017

Having DebugOutput setter will give developers ability to create their own error handle function which can be very useful.

@ivantcholakov
Copy link
Owner

Hello, @Bankde ,

Please, see this line at PHPMailer:
https://github.com/PHPMailer/PHPMailer/blob/v5.2.26/class.phpmailer.php#L663
By default PHPMailer detects CLI and sets the option accordingly.

CodeIgniter detects CLI this way: https://github.com/bcit-ci/CodeIgniter/blob/3.1-stable/system/core/Common.php#L381

Therefore, for not breaking backward compatibility I would prefer the default configuration option to have this value:

$config['debug_output']     = (PHP_SAPI === 'cli' OR defined('STDIN')) ? 'echo' : 'html'; // PHPMailer's SMTP debug output: 'html', 'echo', 'error_log' or user defined function with parameter $str and $level.

Do you agree? If yes, you can add this change to this PR, or I can accept this PR as it is and make the change later.

@ivantcholakov
Copy link
Owner

@Bankde

Since there is a newer version of PHPMailer and I hurry, I would merge this PR as it is and implement my suggestion. You can always customize the default value in your projects.

Thank you very much for your contribution.

@ivantcholakov ivantcholakov merged commit c38e023 into ivantcholakov:master Nov 5, 2017
@Bankde
Copy link
Contributor Author

Bankde commented Nov 5, 2017

I completely agree with you. I forgot that codeigniter could be run on CLI. Have you start implementing your suggestion yet ?

I have a problem with modifying static default_properties.
Since PHPMailer can already detect CLI, I think I will remove default from library and let PHPMailer detects it alone. For the code in config, I will put the CLI detection there. Is that good ?

@ivantcholakov
Copy link
Owner

ivantcholakov commented Nov 5, 2017

I researched a little bit, for this particular case, I think

$config['debug_output']     = (strpos(PHP_SAPI, 'cli') !== false OR defined('STDIN')) ? 'echo' : 'html';

would be a better choice.

https://www.sitepoint.com/taking-advantage-of-phps-built-in-server/

PHP_SAPI could return 'cli' or 'cli-server', for both of these cases I would set 'echo' as PHPMailer does.

Let me do the changes and I would ask you for comments.

ivantcholakov referenced this pull request Nov 5, 2017
…mately as PHPMailer does.

Signed-off-by:Ivan Tcholakov <ivantcholakov@gmail.com>
ivantcholakov referenced this pull request Nov 5, 2017
Signed-off-by:Ivan Tcholakov <ivantcholakov@gmail.com>
@Bankde
Copy link
Contributor Author

Bankde commented Nov 5, 2017

Just a reminder, don't forget to fix debug_output from $default_properties.
We could just remove it in PHPMailer 5.2.26, otherwise we will have to set default debugoutput in the constructor.

ivantcholakov added a commit that referenced this pull request Nov 5, 2017
…brary as PHPMailer does. Tracking the option value by using the corresponding internal property.

Signed-off-by:Ivan Tcholakov <ivantcholakov@gmail.com>
@ivantcholakov
Copy link
Owner

Actually, I used the constructor to enforce the autodetection. I think this part of code is finished now.

ivantcholakov added a commit that referenced this pull request Nov 5, 2017
Signed-off-by:Ivan Tcholakov <ivantcholakov@gmail.com>
ivantcholakov added a commit that referenced this pull request Nov 5, 2017
Signed-off-by:Ivan Tcholakov <ivantcholakov@gmail.com>
ivantcholakov added a commit that referenced this pull request Nov 5, 2017
Signed-off-by:Ivan Tcholakov <ivantcholakov@gmail.com>
@ivantcholakov
Copy link
Owner

@Bankde

If you have no objections I am going to make my usual testing and to tag a new release 1.2.26.

@Bankde
Copy link
Contributor Author

Bankde commented Nov 5, 2017

The code looks great. I have no objections here.

ivantcholakov added a commit that referenced this pull request Nov 5, 2017
… be invoked by NULL or empty string value.

Signed-off-by:Ivan Tcholakov <ivantcholakov@gmail.com>
@ivantcholakov
Copy link
Owner

Done. Thank you once again. :-)

ivantcholakov added a commit that referenced this pull request Nov 5, 2017
Signed-off-by:Ivan Tcholakov <ivantcholakov@gmail.com>
ivantcholakov added a commit that referenced this pull request Nov 5, 2017
Signed-off-by:Ivan Tcholakov <ivantcholakov@gmail.com>
ivantcholakov added a commit that referenced this pull request Nov 5, 2017
Signed-off-by:Ivan Tcholakov <ivantcholakov@gmail.com>
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.

2 participants