[PEPr] +1 for HTML::PHPTAL
| From: | Michael Gauthier | Date: | Mon, 27 Apr 2009 16:21:55 +0000 |
| Subject: | [PEPr] +1 for HTML::PHPTAL | ||
| References: | 1 | Groups: | php.pear.dev |
| Request: | Send a blank email to pear-dev+get-51774@lists.php.net to get a copy of this message | ||
Michael Gauthier (http://pear.php.net/user/gauthierm) has voted +1 on the proposal for HTML::PHPTAL.
Proposal information:
http://pear.php.net/pepr/pepr-proposal-show.php?id=597
Vote information:
http://pear.php.net/pepr/pepr-vote-show.php?id=597&handle=gauthierm
This vote is conditional. The condition is:
This looks to be a comprehensive and generally well written package; however, it was rushed to the
voting stage. I think PHPTAL would be good to have in PEAR, but not in its current state. There are
several outstanding issues that will prevent its adoption in PEAR. By applying these suggestions,
the code will be more maintainable in the long term.
1.) phpcs, specifically
- line length
- whitespace, space around operators
- required docblocks
- required doc tags in docblocks
2.) immediately executable code is not allowed
- PHPTAL
- XML namespace code
3.) use require_once consistently instead of require
4.) specify optional extension dependencies in package.xml
- gettext, specifically. Other extensions may also apply.
5.) closing php tags are required
6.) use class constants consistently instead of define()
7.) procedural code in Tales.php should be in a static class
8.) @package name is inconsistent between files
9.) there should be one class/interface per file
10.) Why do you re-implement an XML parser? SAX parsing is in PHP. Does the comment in the header
still apply?
11.) SAX parser should not be named 'Dom'. DOM referrs to a different way of parsing.
12.) properties should be defined before methods in classes, not interlaced
13.) use PEAR packages to convert roman numerals PEAR::Numbers_Roman
14.) assertions may not be allowed in PEAR, a exceptions should probably be thrown instead
15.) complicated regexps like
'/^[^$&\/=\'"><\s`\pM\pC\pZ\p{Pc}\p{Sk}]+$/u' should have a comment
explaining what they do. Consider using /x and documenting the exression inline