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

Hi Kornel, The latest version you uploaded has the tar file named as latest-pear.tar-3. It will be easier to open for review if it is just .tar. On Mon, 2009-04-27 at 18:38 +0100, Kornel Lesiński wrote: > 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. > Lots of files also have PHPTAL_Dom_Defs::getInstance()->registerNamespace(new PHPTAL_Namespace_METAL()); or something similar. Requiring files should not cause any action in PEAR besides defining classes. > > 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. > You should still use require_once in other files as it is required by PEAR. See http://pear.php.net/manual/en/standards.including.php > > 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? > That's perfect. I missed that when I read the package.xml. > > 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. > New PEAR packages should not use any define() constants. Any packages using such constants are from the PHP4 era. > > 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 > That sounds reasonable to me. > > 8.) @package name is inconsistent between files > > Do I have to use identical @package everywhere? > > Current package names give nice grouping in phpDocumentor. > I'm not exactly sure of the rule here. I'd say you can use sub-packages. I was confused because I saw PHPTAL and PHPTAL.php and thought that PHPTAL.php package name was accidentally copied from a filename. > > 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). Classes and interfaces should still be one to a file. There is an exception to the rule for Exception classes, which may go in a single file. Keep in mind that this rule is changing for PEAR2 where the class-per-file will be stricter. > > 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. > Could those special cases not be handled by a pre-processing and post-processing the XML? Then you could use a real XML library to do the heavy lifting. > 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). > The XML DocumentBuilder has similar class names to some DOM classes but the interface is not similar. Calling it 'DOM' will be confusing to developers as the DOM refers to a specific, well documented interface. The parser API in PHPTAL is similar to the SAX model where parsing is handled by event listeners. Strangely, you use this parser to build a tree model in memory. If you're storing the document tree in memory, you're probably better off just using the DOM. SAX is handy if you want to stream XML. > Should I keep names of classes and directories consistent? > > (PHPTAL/Dom/XmlParser.php => PHPTAL_Dom_XmlParser) > Yep, file names should map directly to class names. Cheers, Mike