PHP Cron wrapper

Question

I attempt to create a reusable and scalable Cron Wrapper in PHP.

I'm at the begining of the project and I wanted have some tips/ critics / improvments to make before going too far.

The final objective is to have a complete cron wrapper which allows to lauch :

'one-shot' task (like huge DB export) from Web
manage Crontask from a web interface or from the 'default way'
Maybe more ;)

There is the main core class

<?php
/**
 * Nozdormu is a PHP5 cron wapper.
 *
 * @package    Nzdrm
 * @version    0.1
 * @author     vfo 
 */


namespace Nzdrm;
require_once('log.php');
require_once('profiler.php');
require_once('process.php');
require_once('vendors/PHPMailer/PHPMailerAutoload.php');


class Nzdrm {
    const VERSION = '0.1-dev';
    const CNFPATH = '../etc/';
    const BINPATH = '../bin/';

    public static $locale = 'C';
    public static $timezone = 'UTC';
    public static $process = '';
    public static $encoding = 'UTF-8';
    public static $profiling = false;
    public static $logging = false;
    public static $mail_on_success = true;
    public static $organisation = 'domain.tld';
    public static $mail_recipient = '[email protected]';
    public static $profile_in_mail = true;
    public static $log_threshold = '300';
    public static $log_folder_path = '../var/log/';
    public static $log_file_name = 'Nozdormu';
    public static $initialized = false;
    public static $cnf = array();
    final public function __construct() { }

    public static function init($cnf_fn = 'nzdrm.conf')
    {
        if (static::$initialized)
        {
            logger('WARNING',"Can't be init more than once",__METHOD__);
        }
        $cnf = array();
        if (!file_exists(self::CNFPATH.$cnf_fn))
        {
            logger('WARNING', "Can't find ".self::CNFPATH.$cnf_fn."! Use default parameters", __METHOD__);
        }
        else
            static::$cnf = parse_ini_file(self::CNFPATH.$cnf_fn, true);
        static::$profiling = (empty($cnf['profiler']))?false:true;
        static::$profiling and \Nzdrm\Profiler::init();
        static::$logging = (empty($cnf['logger']))?false:true;
        static::$locale = (empty($cnf['locale']))?null:$cnf['locale'];
        static::$log_folder_path = (empty($cnf['log_folder_path']))?'../var/log/':$cnf['log_folder_path'];
        static::$log_file_name = (empty($cnf['log_file_name']))?'Nozdormu':$cnf['log_file_name'];
            if (static::$locale)
            {
                setlocale(LC_ALL, static::$locale) or
                logger('WARNING', 'The configured locale '.static::$locale.' is not installed on your system.', __METHOD__);
            }
        static::$timezone = (!empty($cnf['timezone'])) ? $cnf['timezone']: date_default_timezone_get();
            if (!date_default_timezone_set(static::$timezone))
            {
                date_default_timezone_set('UTC');
                logger('WARNING', 'The configured locale '.static::$timezone.' is not valid.', __METHOD__);

            }
        static::$log_threshold = (!empty($cnf['log_threshold'])) ? $cnf['log_threshold']: '300';
        static::$initialized = true;
        if (static::$profiling)
        {
            \Nzdrm\Profiler::mark(__METHOD__.' End');
        }

    }
        public static function get_from_db()
        {
            /** @TODO
            **  Set DB connection
            **  Get Waiting Task(s)
            **  Set Pending state
            **  Lauch Task
            **  Update state not oneshot task
            **/
        }
        public static function launch($process)
        {

            static::$process = $process;
            if (static::$profiling)
                \Nzdrm\Profiler::mark(__METHOD__.' Start');
            if (!file_exists(self::BINPATH.$process.'.php'))
                logger('ERROR', "Can't find ".self::BINPATH.$process."! Shut down", __METHOD__);
            else
                if (!is_executable(self::BINPATH.$process.'.php'))
                    logger('ERROR', self::BINPATH.$process." isn't executable! Shut down", __METHOD__);
                else
                {
                    if (static::$logging)
                        logger('INFO', 'Nozdormu launches '.$process.'.php');
                    $pr = new \Nzdrm\Process($process.'.php');
                    while(true)
                        if ($pr->status === false)
                            break;
                }
            if (static::$profiling)
                \Nzdrm\Profiler::mark(__METHOD__.' End');
            if (static::$logging)
                logger('INFO', 'Nozdormu ended '.$process.'.php');
            self::shut_down();
        }
        public static function shut_down()
        {
            if (static::$logging)
                logger('INFO', 'Nozdormu is Shuting down.');
            $mail = new \PHPmailer();
            $mail->From = 'noreply@'.static::$organisation;
            $mail->FromName = 'Nozdormu';
            $mail->addAddress(static::$mail_recipient);
            $mail->addReplyTo('noreply@'.static::$organisation, 'No-Reply');

            $mail->WordWrap = 50;
            if (file_exists(static::$log_folder_path.date('Y/m/d-').static::$process.'.log'))
            $mail->addAttachment(static::$log_folder_path.date('Y/m/d-').static::$process.'.log');

            $mail->isHTML(false);
            $title = '';
            $err= false;
            if (file_exists(static::$log_folder_path.date('Y/m/d-').static::$process.'.log'))
            {
                $ftmp = file_get_contents(static::$log_folder_path.date('Y/m/d-').static::$process.'.log');
                $titlechange = array('WARNING', 'ERROR','ALERT');
                foreach ($titlechange AS $tc)
                    if (substr_count($ftmp, $tc))
                    {
                        $title = 'Cron ended with '.strtolower($tc).'(s)';
                        $err= true;
                    }
                if (!$title)
                    $title = "Cron sucessfully ended";
            }
            $mail->Subject = '[NZDRM]['.self::$process.']'.$title;
            $body= "Hi,\n";
    if ($err)
    $body .="The script '".static::$process."' generates the error/warning messages during his last execution.\nPlease read log file in attachment.\n";
    else
    $body .= "The script '".static::$process."' successfully ended";

    if (static::$profile_in_mail)
     $body .= "Profile overviews:\n" . \Nzdrm\Profiler::displayLogs();
    $body .="\n-- \n[This is an automatic message send by Nozdormu v. ".self::VERSION."]\n";

            $mail->Body = $body;
            if ($err !== true AND !static::$mail_on_success)
                if(!$mail->send()) 
                 logger('WARNING', 'Mailer Error: ' . $mail->ErrorInfo, __METHOD__);

        }
    }
    ?>

Full repo here

Your code contains: waay to many static's, syntax errors (else static::$cnf = parse_ini_file(self::CNFPATH.$cnf_fn, true); static::$profiling = (empty($cnf['profiler']))?false:true; wont produce the desired result) , (ab)use of ternary operators, doesn't use a proper autoloader, and will cause loss of hair, sleep, sanity and faith in humanity for whoever ends up debugging/maintaining it a couple of months from now. — Elias Van Ootegem, Nov 27 '13 at 13:57
@EliasVanOotegem there is no syntax error(s) maybe a bad indentation ;). Why did you say tatic::$profiling = (empty($cnf['profiler']))?false:true; wont produce the desired result? You make a point for the autoloader i'm working on it. — Foobar, Nov 27 '13 at 14:06
The else will only execute the first statement, the other statements will be executed regardless. You're logging an error if the ini file can't be found, else you're parsing it into $cnf and continue processing what may well be an empty array. Also: you're using empty, which is woefully unreliable. If you can't find the config file, stop, that's a fatal error, don't try to blunder along, you might end up doing more harm than good. — Elias Van Ootegem, Nov 27 '13 at 14:13
@EliasVanOotegem the app can run with default parameters, so a missing conf file isn't a fatal error, but you're right, if the file can't be find, i should skip the test and directly run with defaults values. — Foobar, Nov 27 '13 at 14:19

Foobar · Answer 1 · 2013-11-29 13:52:53Z

Going to try to answer my own question. So I should use :

Less statics, and implement an interface to deal with config parameters, and just keep the most useful in attributs (logging, profilling)
Move the mailing functionnality to her own method.
In \Nzdrm::init just keep the init for logging, profilling and locale configuration.
Use Exceptions
(Minor) Comment code while coding, otherwise it's going to never be done.

Based on Elias comments :

Use an autoloader
Don't try to deal without a conf file and just exit with a FATAL ERROR, so delete all default values.

Finally I realize that is the approach may be a bit outdated, and a puppet based solution would be more suitable, and this project is going to go in study case archive.

Malachi · Answer 2 · 2013-11-29 14:14:14Z

in one of your functions you have two different coding styles.

public static function init($cnf_fn = 'nzdrm.conf')
{
    if (static::$initialized)
    {
        logger('WARNING',"Can't be init more than once",__METHOD__);
    }
    $cnf = array();
    if (!file_exists(self::CNFPATH.$cnf_fn))
    {
        logger('WARNING', "Can't find ".self::CNFPATH.$cnf_fn."! Use default parameters", __METHOD__);
    }
    else
        static::$cnf = parse_ini_file(self::CNFPATH.$cnf_fn, true);
    static::$profiling = (empty($cnf['profiler']))?false:true;
    static::$profiling and \Nzdrm\Profiler::init();
    static::$logging = (empty($cnf['logger']))?false:true;
    static::$locale = (empty($cnf['locale']))?null:$cnf['locale'];
    static::$log_folder_path = (empty($cnf['log_folder_path']))?'../var/log/':$cnf['log_folder_path'];
    static::$log_file_name = (empty($cnf['log_file_name']))?'Nozdormu':$cnf['log_file_name'];

in your if statements you consistently use brackets, even for one liners, but then in the else statement you don't use any brackets, this gets confusing and can lead to you not knowing where the statement really ends, it is better coding practice to use the brackets (in my opinion).

then the next if statements that you come to are indented again, and they really shouldn't be, this makes them look like they should be inside of another code block.

I keep going and find a Function indented where it shouldn't be.

 public static function get_from_db()

this is indented too far.

it really looks like there are two people writing this code.

my suggestion is that if you borrow code from somewhere else that you style it the way that you would write it so that it is easier for you to read in a year, or two or more. you don't want to look back at your code and think to yourself "what was a I doing here?" you want to be able to read the code and be able to tell what is going on, even if you have learned more about the language and would do things differently, you should still be able to read the code easily.

Agreed with you on all coding style point. In fact I havea lot of bad habits while coding (wrong/mixing indents...). I'm trying to lose those quick&dirty habits to have a good and reusable coding style for this kind of project, but isn't easy to lose years of bad habits. — Foobar, Nov 29 '13 at 14:26
I know what you mean, I am always learning new techniques and such, different styles of coding (where to put brackets) stuff like that — Malachi, Nov 29 '13 at 14:56

asked	1 year ago
viewed	168 times
active	1 year ago

current community

your communities

more stack exchange communities

PHP Cron wrapper

2 Answers 2

Your Answer

Not the answer you're looking for? Browse other questions tagged php oop or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

PHP Cron wrapper

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged php oop or ask your own question.

Related

Hot Network Questions