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

On 27-04-2009 at 19:48:46 Michael Gauthier <mike@silverorange.com> wrote:

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.

I don't have such file on the server. It seems like problem on your side (temp files from previous downloads?) You can use links to specific versions: http://phptal.org/files/PHPTAL-1.2.0alpha12.tar.gz

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.

Ok, I've moved those to singletons' constructors.

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

Ok, I've changed that.

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.

PEAR itself is PHP4 software and 4 out of 5 most popular PEAR downloads violate this rule. Which packages should I take as an example then?

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.

I've used @subpackage as Greg suggested.

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.

I've split RepeatController.php, but I'm not convinced that splitting files further would be helpful to anybody (some of them would have more PEAR boilerplate than code).

* 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.

Third case certainly can, I'm not sure about others. I'd like to see first how users respond to XML parsing fixes already done in 1.2.0.

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.

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.

Then it's hard to satisfy both of your requirements at once. "Dom" in parser's name doesn't come from its interface, but from directory where the file resides. I could: 1. violate first rule and keep Dom in name of SAX-like parser class 2. violate the second rule and remove Dom from name of class that is in Dom directory 3. move parser to PHPTAL's root directory, where it doesn't belong, as it is integral part of PHPTAL's machinery that does end up building DOM-like structure. Currently I've chosen #2. I'm fine with #1. I don't like #3. -- regards, Kornel