On Wed, Jan 12, 2011 at 6:13 PM, Jeroen De Dauw jeroendedauw@gmail.comwrote:
That seems highly contrary to expectations when dealing with tag hooks.
What if there's a tag hook "display" registered by another plugin, does it get overridden? Overridden only if a "map" parameter is supplied?
That's a good point, which I did not consider when naming the hook like this. Sorting the tag extensions desc by amount of "words" in their name and then handling them in that order should prevent anything from getting overridden no? In any case, I'll have a closer look at this for the next version of the extension.
It looks like Parser::setHook() and Parser::setTransparentTagHook() don't do any validation on tag hook names. At least the search for transparent tag hooks is very naive, assuming that only valid names were passed and knowing that valid names contain only characters which carry no special meaning in regex syntax:
$elements = array_keys( $this->mTransparentTagHooks ); $text = $this->extractTagsAndParams( $elements, $text, $matches, $uniq_prefix );
... in which happens...
$taglist = implode( '|', $elements ); $start = "/<($taglist)(\s+[^>]*?|\s*?)(/?" . ">)|<(!--)/i";
This would explain why you don't get an error thrown when setting the tag hook with a space in the name (parser assumes extensions are acting correctly, and doesn't validate), and why the incorrect entry gets "parsed" as it does (the code that extracts them assumes valid input, so uses a string match that's convenient to write when those assumptions hold).
I haven't dared look at the preprocessor code in case horrible things happen there too. :)
Platonides wrote:
It is order dependant. If you install the extension providing 'display', then the one providing 'display map', the latter will never be called. If 'display map' gets registered first, both will work (your users might get annoyed, though).
Guess what I tried to do in r80025? :)
I'd recommend whitelisting legit characters rather than blacklisting a handful of illegit characters; tag hook name validation should probably sensibly match the rules of XML element naming, as that's what the syntax aims to look and act like.
It'd also be best to write the validation code only once and call it from multiple locations, rather than duplicating the code. This will make it much easier to maintain, as there will be less danger of the multiple copies getting out of sync.
-- brion