Re: [PEPr] +1 for HTML::PHPTAL

On 27-04-2009 at 17:21:55 Michael Gauthier <mike@silverorange.com> wrote:

2.) immediately executable code is not allowed - PHPTAL

In PHPTAL.php I've got only: PHPTAL::setIncludePath() which as discussed previously, is needed to ensure that one copy of PHPTAL does not accidentally include files from another copy.

3.) use require_once consistently instead of require

I use this for files which I can guarantee that won't be included more than once (e.g. subclasses for file that is require_once'd itself) and which are not allowed to be included by PHPTAL users.

4.) specify optional extension dependencies in package.xml - gettext, specifically. Other extensions may also apply.

I've got this in package.xml: <dependencies> <required> <php> <min>5.1.0</min> </php> <pearinstaller> <min>1.4.6</min> </pearinstaller> <extension> <name>pcre</name> </extension> </required> <optional> <extension> <name>gettext</name> </extension> </optional> </dependencies> is that not enough?

6.) use class constants consistently instead of define()

OK. The only one left now is PHPTAL_VERSION, which I presume is OK, as other PEAR packages are using such defines.

7.) procedural code in Tales.php should be in a static class

I've moved bodies of these functions to a class, but functions themselves need to stay, as they've ended up part of public API. http://phptal.org/manual/en/split/custom-modifiers.html

8.) @package name is inconsistent between files

Do I have to use identical @package everywhere? Current package names give nice grouping in phpDocumentor.

9.) there should be one class/interface per file

I've split Node.php which was too crowded indeed. I've left some files with multiple classes in case when classes were very small (abstract base classes, DOM nodes for comments, doctype, Exception subclasses) or were coupled pairs of classes (helper for grouping in repeat controller, file source + file resolver).

10.) Why do you re-implement an XML parser? SAX parsing is in PHP. Does the comment in the header still apply?

I haven't tested SAX parsers, but compared to PHP's DOM parser: * PHPTAL preserves entites. Users like their &nbsp;, but hate getting &oacute;. Exact format of special characters does matter for some horribly broken mobile phones :( * PHPTAL micromanages CDATA and needs to know where they are in source document to handle some edge-cases properly. * Does not require namespace declarations for tal/metal/i18n/phptal prefixes. I know it's against well-formedness, but it's also very convenient and probably most PHPTAL templates rely on that. I've already started refactorings needed to add other parsers as an option, and perhaps getting rid of PHPTAL's own parser in the future. However, because of compatibility with existing templates, I can't switch just yet (PHPTAL 1.2.0 breaks enough ill-formed templates already :)

11.) SAX parser should not be named 'Dom'. DOM referrs to a different way of parsing.

It's got this name because it's part of PHPTAL's DOM builder (which ends up with DOM-like structure). Should I keep names of classes and directories consistent? (PHPTAL/Dom/XmlParser.php => PHPTAL_Dom_XmlParser) ---- I've updated proposed PHPTAL package with changes mentioned above: http://phptal.org/latest-pear.tar.gz -- regards, Kornel