Home » HTML » HTML_Menu » Bug #5352

[2005-09-09 19:12 UTC] aaron dot hawley at uvm dot edu

Description:
------------
Menus created with DirectTreeRenderer.php, do not replace special HTML characters in to their respective entity values. I haven't checked any of the other "renderers". It's a bit of an oversight.

--- DirectTreeRenderer.php 2005/09/09 18:03:42 1.1
+++ DirectTreeRenderer.php 2005/09/09 18:16:21
@@ -115,7 +115,7 @@
foreach ($node as $k => $v) {
if ('sub' != $k) {
$keys[] = '{' . $k . '}';
- $values[] = $v;
+ $values[] = htmlspecialchars($v);
}
}
$this->_itemHtml[$level] = str_replace($keys, $values, $this->_entryTemplates[$type]);

Test script:
---------------
<?php

require_once 'HTML/Menu.php';
require_once 'HTML/Menu/DirectTreeRenderer.php';

$menu = array ( array ( 'title' => 'Home',
'url' => '/'),
array ( 'title' => 'Query',
'url' => '/?pear=rocks&sometimes=1'));

$html_menu =& new HTML_Menu($menu);
$renderer =& new HTML_Menu_DirectTreeRenderer();
$html_menu->render($renderer, 'sitemap');
print $renderer->toHtml();

?>

Expected result:
----------------
<ul><li><strong>Home</strong></li><li><a href="/?pear=rocks&sometimes=1">Query</a></li></ul>

Actual result:
--------------
<ul><li><strong>Home</strong></li><li><a href="/?pear=rocks&sometimes=1">Query</a></li></ul>

[2005-09-15 13:14 UTC] aaron dot hawley at uvm dot edu

That argument would work for any API that wasn't an HTML module, but this one is. This is upsetting.

If ignoring this is an attempt to assuage some compatability issue, most related PEAR packages (like for XML or URL data) handle it by allowing the user to specify true or false -- defaulting to the latter -- on whether the values or escaped or not.

For an example see PEAR::Net_URL::addQueryString().

Shall I proceed in making this a feature request? Shall I report a new bug?

[2005-09-15 15:23 UTC] aaron dot hawley at uvm dot edu

"... though 'title' attribute in menu structure may easily
contain HTML tags."

What?

[2005-09-15 16:51 UTC] aaron dot hawley at uvm dot edu

Sorry, in our discussing HTML-related material, I read your suggestion of the "title attribute" as the attribute of the same name used in HTML as an element attribute. I now understand you meant the "title" key of an HTML_Menu array.

I'm not familiar with all of the HTML_Menu source, but I thought adding customized HTML using the title of menu entries as you suggested would be accomplished with the API's "template" system. Adding peculiar HTML to individual entries would seem to be the exceptional usage, and should not dictate the default usage.

The case of Javascript is not different, than any other values converting to HTML. The package's apprehension about context is my motive for reporting the bug. We *do* know the contexts for the use of this package, it's always HTML. The package's purpose is *HTML menu* rendering. Other special contexts like Javascript should be required to create sub-classes, not the default usage.

I wasn't talking about URL encoding, but if the API needs to handle that situation, then I agree. I can make an improvement to the patch for you; if that is what you are asking.

[2005-09-19 14:45 UTC] aaron dot hawley at uvm dot edu

Ok. I'm still waiting for a scenario where htmlspecialchars() shouldn't be used. The example [2005-09-15 13:36 UTC] does not and instead brings up urlencode() which is not relevant to the bug report.

I don't use Javascript in my values for 'title', or 'url' in my own work, but I can't imagine using Javascript or not matters.

[2005-09-19 15:06 UTC] aaron dot hawley at uvm dot edu

I argue that the urlencode() mention is not relevant and would still like to see a better counterexample of when data should not be sent through htmlspecialchars().

My mention of "not using" Javascript was meant to express the ignorance I have of the specific situation, not my attempt to judge that it's not useful for the package to support. I still hold that Javascript source in an HTML page is HTML data.

[2005-09-20 20:12 UTC] aaron dot hawley at uvm dot edu

I investigated the other Menu renderers for HTML_Menu, and below is the only necessary change for the package. The patch is against the file DirectRenderer.php. I assume the Template Sigma subclasses don't need HTML special characters escaped, because the Template system abstracts and correctly handles the values.

--- DirectRenderer.php 18 Jan 2004 17:35:52 -0000 1.3
+++ DirectRenderer.php 20 Sep 2005 19:54:17 -0000
@@ -106,7 +106,7 @@
foreach ($node as $k => $v) {
if ('sub' != $k) {
$keys[] = '{' . $k . '}';
- $values[] = $v;
+ $values[] = htmlspecialchars($v);
}
}

$this->_rowHtml .= str_replace($keys, $values, $this->_entryTemplates[$type]);

Here's another scenario for fixing this bug:

Test script:
---------------

<?php

require_once 'HTML/Menu.php';
require_once 'HTML/Menu/DirectRenderer.php';

$menu = array ( array ( 'title' => 'Mathematics',
'url' => '/'),
array ( 'title' => 'Comparisons with the < symbol',
'url' => '/less-than.html'));

$html_menu =& new HTML_Menu($menu);
$renderer =& new HTML_Menu_DirectRenderer();
$html_menu->render($renderer, 'sitemap');
print $renderer->toHtml();

?>

Expected result:
----------------
This is valid (X)HTML:

<table border="1"><tr><td><b>Mathematics</b></td></tr><tr><td><a href="/less-than.html">Comparison with <</a></td></tr></table>

Actual result:
--------------

This is not:

<table border="1"><tr><td><b>Mathematics</b></td></tr><tr><td><a href="/less-than.html">Comparisons with <</a></td></tr></table>

[2005-10-07 20:08 UTC] aaron dot hawley at uvm dot edu

I'm still waiting for a reason HTML would be allowed in the "title" value for an HTML_Menu_DirectTreeRenderer or an HTML_Menu_DirectRenderer object.

Processing the values of nested HTML_Menu arrays an additional time before being passed to HTML_Menu (to be processed again) is not really satisfactory and makes it poor library code.