[PEPr] -1 for HTML::HTML_SimpleCalendar

Justin Patrin (http://pear.php.net/user/justinpatrin) has voted -1 on the proposal for HTML::HTML_SimpleCalendar. Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=322 Vote information: http://pear.php.net/pepr/pepr-vote-show.php?id=322&handle=justinpatrin Comment: This proposal was not ready for voting. It if obvious that you have either not read the PEAR Manual's section for developers or you have chosen not to follow its rules. The following is what I would have commented if I'd had time to look at this package before you called for votes. First, it would be best to either base this on the Calendar package already in PEAR. If you have compelling reasons not to do this (that it's not stable is not sufficient reason) then please explain in detail. If you're worried about the API of the calendar package you can always work with the current maintainer(s) in getting it pushed forward. (In most cases PEAR classes are fairly stable once they hit beta. It is generally discouraged to change APIs after the alpha phase. See PEAR documentation/RFCs/Group documents.) Use of trigger_error() is not allowed. Always use PEAR::raiseError for PHP4 packages. If you need errors in a constructor you should use a factory method. It also simply makes no sense to have a default value of $calname = null when null is not allowed Setting a default value means that this parameter is optional. Only use default values for truely optional parameters. Using exit() in a package is almost never ok. It is certainly not ok to call it explicitly on an error. PEAR::raiseError has a parameter to force the mode (e.g. PEAR_ERROR_DIE) but this should be used only in very very extreme cases. Most of the time you should allow the user to choose their error mode (PEAR does this for you, no need to implement anything yourself or have an option in your package.) There are no docblocks for any of your code. You have some comments, but these should be formatted as docblocks. This is not a *requirement* for having your proposal accepted, but it is very encouraged and would be needed for your first PEAR release. At the very least it would help us understand your methods. Use PEAR::Date instead of parsing your own dates. In addition, allowing the user to specify the date in a number of formats would be nice. (see Date::setDate). Using the PHP date functions isn't suggested for a package such as this as they can only deal with date in unix timestamps which cannot normally deal with dates before a certain time. Using ob_start() and ?> <?php is really not a good solution. Use a normal vairiable and simple concatenate to it. $html = '<div>'; $html .= 'Some text '.$aVar.'</div>'; This will also make your code more readable. As others have said, using a template of some kind would be a good idea in order to allow people to output their own HTML. You could even just make each piece of output a function in a renderer class which is passed in the variables to output. The user would then be able to create their own renderer. Code like this should be avoided: $querystring = "{$querystring}&"; Use: $querystring .= '&'; In addition, any code with double quotes would be best replaced by single quotes and concatenation. See: http://pear.reversefold.com/strings/ In most cases, using htmlentities() is better than using htmlspecialchars(), especially when putting variables into an attribute (use ENT_QUOTES). You generally don't need to use $_SERVER['PHP_SELF'] when outputting links. Relative links should work fine ('?var=val'). In addition, you may want to look into HTML_Common which deals with all HTML output and formatting for you. -- Sent by PEPr, the automatic proposal system at http://pear.php.net