Fails validation due to incorrectly nested lists.

This code is generated by the function _site_map_taxonomy_tree(). If someone has time to patch that function to make it return valid code I will be happy to commit it.

Sooner or later I will find time to do it myself.

This should produce valid code such as;-

<ul>
<li>Top Level 1</li>
<li>Top Level 2
  <ul>
    <li>Nested Level 2.1</li>
    <li>Nested Level 2.2</li>
  </ul>
</li>
<li>Top Level 3</li>
</ul>

Would some one like to review this? I've only changed the foreach () loop but I've printed the whole function here;

function _site_map_taxonomy_tree($vid, $name = NULL, $description = NULL) {
  if ($vid == variable_get('forum_nav_vocabulary', '')) {
    $title = l($name, 'forum');
  }
  else {
    $title = $name ? check_plain($name) : '';
  }
  $title .= (module_exists('commentrss') ? ' '. theme('site_map_feed_icon', url("crss/vocab/$vid"), 'comment') : '');

  $last_depth = -1;
  $rss_depth = variable_get('site_map_rss_depth', 'all');
  if (!is_numeric($rss_depth) || $rss_depth < 0) {
    $rss_depth = 'all';
  }
  $cat_depth = variable_get('site_map_categories_depth', 'all');
  if (!is_numeric($cat_depth)) {
    $cat_depth = 'all';
  }

  $output = $description ? '<div class="description">'. check_plain($description) .'</div>' : '';

  $output .= '<div class="tree">';
  // taxonomy_get_tree() honors access controls
  $tree = taxonomy_get_tree($vid);
  foreach ($tree as $term) {
    // Adjust the depth of the <ul> based on the change
    // in $term->depth since the $last_depth.
    if (($delta = $term->depth - $last_depth) <> 0) $output .= str_repeat(($delta > 0) ? '<ul class="">': '</ul></li>', abs($delta))."\n";
    $last_depth = $term->depth;

// Display the $term.
    $term->count = taxonomy_term_count_nodes($term->tid);
    $output .= '<li>'.(($term->count) ?
      l($term->name,
        ($cat_depth < 0) ?
           taxonomy_term_path($term) :
           "taxonomy/term/$term->tid/$cat_depth",
        array('title' => $term->description)) :
      check_plain($term->name));

    if (variable_get('site_map_show_count', 1)) {
      $output .= " ($term->count)";
    }
    if (variable_get('site_map_show_rss_links', 1)) {
      $output .= ' '. theme('site_map_feed_icon', url("taxonomy/term/$term->tid/$rss_depth/feed"));
      $output .= (module_exists('commentrss') ? ' '. theme('site_map_feed_icon', url("crss/term/$term->tid"), 'comment') : '');
    }
  }
  // Bring the depth back to where it began, -1.
  $output .= str_repeat('</ul></li>', $last_depth)."</ul>\n";
  $output .= "</div>\n";
  return theme('box', $title, $output);
}

Hi Nik,

Thanks for looking at my code.

I get the same output.

I can't see the missing </li>.  You show 3 × <li>'s and 3 × </li>'s... }:-§  Please see, above the code, the html I'm aiming to produce – slightly different to what you've asked for.  I think levels 2.* should be in the same <li> as level 2.  Does it validate?

I'm only using this for taxonomy @tm as well.

Thanks for the code, I will try to test it this week.

A tips is to hand in code in the form of a patch, see http://drupal.org/patch. It makes the code a lot easier to test.

I’ve validated my html with the W3C Markup Validation Service as

• HTML 4.01 Strict
• XHTML 1.0 Strict
• XHTML 1.1

Passed all 3.
I’ve also validated this page (I’ve included my html at the end of update #5). It had eight errors, but nothing about lists.

I could only find one example of nested lists on w3c. It did have the nested list in its own <li> but the nested list is not semantically part of the preceding <li> so this is as expected.

I find that my html validates, as indicated in update #3, There is definitely not a definite error (nor is there an indefinite error).

How can you say that Basically your code is fine, but your initial "semantic" html … is not correct ? The code produces the ‘semantic’ html, so you contradict yourself.

PS Does anyone else bother to type … rather than ..., or ‘ / ’ rather than boring old '? Who knew they could?

Subscribing, will this patch be committed?
greetings, Martijn

Subscribing. Encountered this prob today as well.
Reckon a fix should be committed, yet Zeta's code gives me more errors than the original.

Cheers

I've edited the _site_map_taxonomy_tree function a bit.
This is my first try at writing a patch so comments and improvements are welcome :)

When I've had some feedback to this I will try and post it in the form of a patch!

function _site_map_taxonomy_tree($vid, $name = NULL, $description = NULL) {
  if ($vid == variable_get('forum_nav_vocabulary', '')) {
    $title = l($name, 'forum');
  }
  else {
    $title = $name ? check_plain($name) : '';
  }
  $title .= (module_exists('commentrss') ? ' '. theme('site_map_feed_icon', url("crss/vocab/$vid"), 'comment') : '');

  $last_depth = -1;
  $rss_depth = variable_get('site_map_rss_depth', 'all');
  if (!is_numeric($rss_depth) || $rss_depth < 0) {
    $rss_depth = 'all';
  }
  $cat_depth = variable_get('site_map_categories_depth', 'all');
  if (!is_numeric($cat_depth)) {
    $cat_depth = 'all';
  }

  $output = $description ? '<div class="description">'. check_plain($description) .'</div>' : '';

  $output .= '<div class="tree">';
  // taxonomy_get_tree() honors access controls
  $tree = taxonomy_get_tree($vid);
  
  foreach ($tree as $key => $term) {
    // Adjust the depth of the <ul> based on the change
    // in $term->depth since the $last_depth.
    if ($term->depth > $last_depth) {
      for ($i = 0; $i < ($term->depth - $last_depth); $i++) {
        $output .= '<ul>';
      }
    }	
	// Display the $term.
    $output .= '<li>';
    $term->count = taxonomy_term_count_nodes($term->tid);
    if ($term->count) {
      if ($cat_depth < 0) {
        $output .= l($term->name, taxonomy_term_path($term), array('title' => $term->description));
      }
      else {
        $output .= l($term->name, "taxonomy/term/$term->tid/$cat_depth", array('title' => $term->description));
      }
    }
    else {
      $output .= check_plain($term->name);
    }
    if (variable_get('site_map_show_count', 1)) {
      $output .= " ($term->count)";
    }
    if (variable_get('site_map_show_rss_links', 1)) {
      $output .= ' '. theme('site_map_feed_icon', url("taxonomy/term/$term->tid/$rss_depth/feed"));
      $output .= (module_exists('commentrss') ? ' '. theme('site_map_feed_icon', url("crss/term/$term->tid"), 'comment') : '');
    }

	if(!$tree[$key+1]->depth || $tree[$key+1]->depth == $term->depth) {
	    $output .= "</li>\n";
	}
    // Reset $last_depth in preparation for the next $term.
    $last_depth = $term->depth;
  }

  // Bring the depth back to where it began, -1.
  if ($last_depth > -1) {
    for ($i = 0; $i < ($last_depth + 1); $i++) {
      $output .= '</ul>';
	  	if(!$tree[$key+1]->depth && ($i < $last_depth) ) {
	    	$output .= "</li>\n";
		}
    }
  }
  $output .= "</div>\n";
  $output = theme('box', $title, $output);

  return $output;
}

Nevermind, just use the function in sitemap module 1.2 mine is bogus :)

Yes edited my comment.

And I know about patching but am new to this all and didn't even have a clue as to how to create a patch. Sorry to have tried and share some code in the wrong way...

I submitted a patch to fix this issue a little while ago: http://drupal.org/node/231850

I believe these three issues are all the same:

#133090: Fails validation due to incorrectly nested lists.
#231850: site_map fails to nest ul's correctly when displaying taxonomy trees
#299014: Wrong syntax in rendered source (HTML validator error) in dept menupoint

I'm marking the last 2 as duplicates of this issue (which is currently marked RTBC), although I've been using the patch I attached to #231850: site_map fails to nest ul's correctly when displaying taxonomy trees for some time, and am happy with it. It's the maintainer's call which patch gets applied, but it would be good to fix this bug one way or another, IMHO.

Sorry for the blizzard of comments - I've tried to edit the comment above with this extra info, and am getting a strange error (perhaps due to the work ongoing with the project module on d.o?)

If it makes the maintainer's life any easier, I've just checked and my patch http://drupal.org/files/issues/site_map_module_valid_nesting_of_uls_0.patch (from issue 231850) still applies to site_map-5.x-1.x-dev with no errors.

this appears to also be an issue in 6.x-1.0 - does that merit a separate ticket, or should that wait on the resolution of this one?

subscribing.

Patch by mcdruid in http://drupal.org/node/231850 in committed to 6-dev.

Thanks for applying the patch to the D6 version frjo - any chance you could commit it to the D5 branch too?

Committing the patch for 5.0 would be awesome, I just spent half an hour coding this up myself and was about to submit a patch when I found this issue :P